valgrind conditional jump or move depends on uninitialized

0
votes

Valgrind is throwing two warning for two lines of codes, mentioned in the comments next to each.

Warning 1:

invalid write of size 8, Address ... is 8 bytes inside a block of size 9 alloc'd

data[size] = NULL;

Warning 2:

Conditional jump or move depends on uninitialized values(s)

for (char **ptr = data; *ptr; ptr++) { // warning -> Conditional jump or move depends on uninitialized values(s)
    free(*ptr);
}

Here's a complete code,

Callee

char **getList() {
    char **list = (char *[]) {"John", "Jane", NULL};

    int size = 0;
    for (char **ptr = list; *ptr; ptr++) {
        size++;
    }

    char **data = malloc(sizeof(char *) * size + 1);
    if (data == NULL) goto exception;

    for (int i = 0; *list; list++, i++) {
        data[i] = malloc(sizeof(char) * (strlen(*list) + 1));
        if (data[i] == NULL) goto exception;
        strcpy(data[i], *list);
    }

    data[size] = NULL; // this line gives warning
    // warning -> invalid write of size 8, Address ... is 8 bytes inside a block of size 9 alloc'd
    return data;

    exception:
    fprintf(stderr, "data allocation failed.\n");
    return NULL;
}

Caller different file/scope

char **data = getList();

for (char **ptr = data; *ptr; ptr++) { // warning -> Conditional jump or move depends on uninitialized values(s)
    free(*ptr);
}
free(data);
2
cvalgrind
sizeof(char *) * size + 1 is not sizeof(char *) * (size + 1) - KamilCuk
oh, that fixed the problems, i hope this is ok as well -> (strlen(*list) + 1) - AppDeveloper
Also do you see any other problem with the code, is it well written or do you see some improvements? - AppDeveloper
I would change char **getList() to char **getList(void) and char **list = (char *[]){.. to const char * const list[] = {.... I would explicit test against NULL so *list != NULL and *ptr != NULL. And strlen + strcpy can be optimized to strlen + memcpy. Both size and i can't be negative - should use unsigned. - KamilCuk
it gives me some error, because i'm incrementing list++ in the second for loop, I'd love you to please review this code. Thanks. since I think it has bunch of hoops. - AppDeveloper

2 Answers

1
votes

Writing based on the suggestions on a new slate.

Edit updated based on the comments.

char **getList(void) {
    const char * const list[] = {"John", "Jane", NULL};

    unsigned int size = 0;
    const char * const *ptr;
    for (ptr = list; *ptr != NULL; ptr++) {
        size++;
    }

    char **data = malloc(sizeof(char *) * (size + 1));
    if (data == NULL) goto exception;

    ptr = list;
    for (unsigned int i = 0; *ptr != NULL; ptr++, i++) {
        const size_t cache = strlen(*ptr) + 1;
        data[i] = malloc(cache);
        if (data[i] == NULL) goto exception;
        memcpy(data[i], *ptr, cache);
    }

    data[size] = NULL;
    return data;

    exception:
    fprintf(stderr, "Allocation failed.\n");
    return NULL;
}

VERSION 2

With the help of Using C-string: "Address of stack memory associated with local variable returned"

const char * const *getList(void) {
    static const char * const list[] = {"John", "Jane", NULL};
    return list;
}
0
votes

The code below is not tested. My recommendations are

  • Use strdup
  • Use calloc

  • Try to avoid using goto

    char **getList() { char **list = (char *[]) {"John", "Jane", NULL};

    // this size is a compile time constant, no need to
calculate it at runtime
size_t size = sizeof(list)/sizeof(list[0]);

// using calloc initializes everything to 0/NULL
// -> no need to set the last element then
char **data = calloc(sizeof(char *), size);

// don't use goto
if (data != NULL)
{
    for (size_t i = 0U; i < size; i++)
    {
        if (list[i] != NULL)
        {
            // don't re-invent the wheel with strlen/memcpy
            // just use strdup
            data[i] = strdup(list[i]);
            if (data[i] == NULL) goto exception;
        }
    }
    return data;
}

exception:
// you need to do more cleanup here
// for instance if the 'data' calloc succeeds, the "John" strdup succeeds
// but the "Jane" strdup fails then you have two leaks. That said fixing
// small leaks when you are out of memory usually isn't your biggest issue.
fprintf(stderr, "data allocation failed.\n");
return NULL;

    }