CODEWITHSUNDEEP
cpointerssegmentation-fault
waynemystir
waynemystir

Reputation: 343

Why does this code usually work but sometimes produce Segmentation Fault?

Here is my code

#include <stdlib.h>
#include <string.h>
#include <stdio.h>

/**************************************************
a is a pointer to an array of strings
b is a string
This function appends b to *a
n is the number of strings currently "held" by *a
**************************************************/
int append(char ***a, char *b, unsigned long n) {
    if (a && *a && **a) {
        char** tmp = realloc(*a, (n + 1) * sizeof(**a));
        if (tmp) {
            tmp[n] = b;
            *a = tmp;
            return 0;
        }
    }
    return -1;
}

void test() {
    char *words[7] = { "food", "is", "good", "to", "eat,", "like", "pizza" };
    char** a = malloc(1 * sizeof(*a));

    for (int i = 0; i < 7; i++) {
        append(&a, words[i], i);
        int j = 0; 
        while (j <= i) 
            printf("%s ", a[j++]); 
        printf("\n");
    }
}

int main() {
    test();
    return 0;
}

The code always compiles fine and without warnings. And the executable runs as expected about 95% of the time. But about 5% of the time, I get a Segmentation Fault. I know the fault occurs at a[j++] but I don't understand why.

Upvotes: 3

Views: 101

Answers (1)

templatetypedef
templatetypedef

Reputation: 372972

Take a look at this line:

if (a && *a && **a)

When you malloc space for one element initially pointed at by a, you never actually initialize that memory. As a result, **a is uninitialized, so reading it is considered undefined behavior. In practice, I suspect that sometimes the memory that's allocated to you is a null pointer in some cases but not others, which accounts for the flakiness.

I don't actually think you even need the checks on *a and **a. As long as the pointer a itself isn't a null pointer, you can modify the pointer that it points at (*a). Moreover, knowing whether the first element of the array pointed at by *a is null (**a) isn't actually needed here. So you can just replace this check with

if (a)

I would go further and not even allocate an initial array for a, since you're never actually going to read the value that's stored there.

Other things to do: the function append returns a status code that informs whether the operation succeeded or failed. It would be a good idea to check that value whenever calling append just in case it fails. You might also want to change the outer if statement into an assert so that if someone calls it with bad parameters, it halts and reports a violation of the preconditions rather than failing with an error code. After all, if the issue is "you gave me arguments that couldn't possibly be right," it means there's a logic error somewhere in the code.

Hope this helps!

Upvotes: 4

Related Questions