Dynamic allocation of char** in C

Question

I have a date type defined as typedef char* DateTime; the format is "dd/mm/yyyy-hh:mm" e.g. "08/08/2012-12:00"

and I want to allocate n string that are "dates". What is wrong with the following?

    DateTime*  dates = (DateTime* ) malloc(sizeof(char*) * n);  
for (int i = 0; i <= n; i++) {
    dates[i] =  malloc(sizeof(char)*16);
    if (dates[i] == NULL) {
        free(dates);

        return NULL;
    }
}

To make the code more readable, you should use `sizeof(DateTime *)`in the first call to `malloc`. Or even better, since they are strings, don't create the `typedef` at all, but go with plain `char *` everywhere. — Some programmer dude, Aug 09 '12 at 13:02
Also, the two big things that's wrong that I can see is that you loop one to many, and you allocate one to little for the actual date string. If there's anything else that wrong you have to be more specific than just asking "whats wrong". — Some programmer dude, Aug 09 '12 at 13:05
Writing `malloc(sizeof(char*))` instead of `malloc(sizeof(*dates))` is not a good idea. Casting the return of malloc is wrong too. http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc — Eregrith, Aug 09 '12 at 13:23

score 7 · Answer 1 · answered Aug 09 '12 at 13:00

7

for (int i = 0; i <= n; i++) {
                   ^

In C arrays start from 0 so dates[n] cannot be accessed. Drop the =.

answered Aug 09 '12 at 13:00

cnicutar

178,505
25
365
392

score 3 · Answer 2 · answered Aug 09 '12 at 13:28

Besides responses by @Dan and @cnicutar (both of which are spot on), note that the string literal "08/08/2012-12:00" contains 17 characters (not 16). While it's string length is 16, it contains the 16 characters that you see PLUS the '\0' character at the end that serves as the terminator. Also, sizeof(char) is one by definition. Finally, the idiomatic way to allocate memory using malloc would be -

DateTime *dates = malloc(n * sizeof *dates);

score 2 · Answer 3 · answered Aug 09 '12 at 13:12

In addition to cnicutar's answer there is also this:

In the event that this condition is true:

if ( dates[i] == NULL )

you are only calling free on the overall array, and not freeing the elements before i in the array. This can lead to a significant memory leak

score 0 · Answer 4 · answered Aug 10 '12 at 05:24

As been pointed out be both me in a comment, and by others, the loop

for (int i = 0; i <= n; i++) {

is looping once to many.

Another problem is this:

dates[i] =  malloc(sizeof(char)*16);

The actual string is 16 characters, but since strings in C needs an extra terminator character ('\0') you need to allocate one character more, which means you should multiply by 17.

And as noted by Dan F you also have a potential memory leak.

The two biggest problems is the looping and allocation, as that will cause so called undefined behavior when overwriting unallocated memory.

Dynamic allocation of char** in C

4 Answers4