0

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);
AppDeveloper
  • 1,816
  • 7
  • 24
  • 49
  • 1
    `sizeof(char *) * size + 1` is not `sizeof(char *) * (size + 1)` – KamilCuk May 11 '20 at 11:35
  • oh, that fixed the problems, i hope this is ok as well -> `(strlen(*list) + 1)` – AppDeveloper May 11 '20 at 11:37
  • Also do you see any other problem with the code, is it well written or do you see some improvements? – AppDeveloper May 11 '20 at 11:38
  • 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 May 11 '20 at 11:39
  • 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 May 11 '20 at 11:41
  • Maybe it's part of the exercise, but I would never clone the names to the heap unless (1) I intend to read them from an external data source, or (2) I intend to make direct modifications to the names (e.g. using `strtok`). – Ruud Helderman May 11 '20 at 11:48
  • Ruud, yes caller is not modifying, but I though I shouldn't return an array without allocation (malloc). The error is fixed but can someone please update it for a better optimum solution? – AppDeveloper May 11 '20 at 11:51
  • @KamilCuk I've added an answer based on the suggestions, can you please review. I didn't get you on the `strlen + memcpy`, need help with the code example please. – AppDeveloper May 11 '20 at 12:31
  • @AppDeveloper In your program, the strings `"John"` and `"Jane"` have a boundless lifetime. This explains why: https://stackoverflow.com/questions/30533439/string-literals-vs-array-of-char-when-initializing-a-pointer – Ruud Helderman May 11 '20 at 12:40
  • Ruud, so you mean I can just return `list` to the caller without allocation (that will be the address of stack memory), I'm not well versed with this, please guide more. I also posted an answer, please feel free to edit it and post as yours with amendments. I'll accept the answer. – AppDeveloper May 11 '20 at 12:57
  • @RuudHelderman, I've edited my answer with version2, is this is what you recommend? – AppDeveloper May 11 '20 at 13:04

2 Answers2

1

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;
}
AppDeveloper
  • 1,816
  • 7
  • 24
  • 49
  • `ptr = (char **) list` ... use proper type`const char * const * ptr = list;`, too many `const` can get confusing. With `strlen + memcpy` I meant to cache the result of `strlen` so that `strcpy` won't have to iterate over the string again. `const size_t cache = strlen(*ptr) + 1; ... malloc(cache); ... memcpy(data[0], *ptr, cache);` – KamilCuk May 11 '20 at 12:36
  • Updated, please review if everything's ok. – AppDeveloper May 11 '20 at 12:43
  • you mean `memcpy(data[i]`, not `data[0]`.? – AppDeveloper May 11 '20 at 12:47
  • @KamilCuk do you agree with version2? – AppDeveloper May 11 '20 at 13:07
  • Yes, I think I would merge it ; ) – KamilCuk May 11 '20 at 13:10
  • @KamilCuk, I even tried to do a one-liner (no declaration, return inline), simply return the array as it is with a cast but doesn't seem like i've a way, any ideas? – AppDeveloper May 11 '20 at 13:15
  • 1
    Yes, for a hard-coded, read-only list, version 2 will do fine. And if you'd like it to be a one-liner, then just get rid of `getList` and let the caller directly refer to `list`. BTW, in version 2, please do not `free`. – Ruud Helderman May 11 '20 at 13:29
  • Thanks, getting rid of the function is not possible (because it has multiple callers), although i confirm, list is readonly and hard coded. was wondering if i could shorten the the function to one line, if possible. – AppDeveloper May 11 '20 at 14:03
0

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;
    

    }

Paul Floyd
  • 5,530
  • 5
  • 29
  • 43