c string array with null fails

Question

A method that returns a string array. For some reason this method fails while accessing the string values specifically for the last NULL value.

static char **getList() {
    char **list = (char *[]){"ABC", "DEF", NULL};
    char **interests = malloc(sizeof(char*) * 3);
    int i = 0;
    while(*list) {
        interests[i] = malloc(sizeof(char) * strlen(*list));
        strcpy(interests[i++], *list);
        list++;
    }
    return interests;
}

Using

char **interests = getList();
while(*interests) {
    puts(*interests);
    interests++;
}

Attached screenshot.

Why create list the way you do? Why not a plain simple array of pointers, as in char *list[] = { "ABC", "DEF", NULL };? And if the array of strings will always be fixed, the strings never modified, you could use static const char *list[] = ...; and return that directly. — Some programmer dude
– Some programmer dude, Commented May 4, 2020 at 8:04
Also, don't forget that the length returned by strlen does not include the string null-terminator. Which means your strcpy calls will write out of bounds of your allocated memory, and lead to undefined behavior. — Some programmer dude
– Some programmer dude, Commented May 4, 2020 at 8:06
And lastly note that you allocate interests as an array of three elements, and use it (when returned) as a null-terminated array, but you never actually copy the terminating NULL pointer. — Some programmer dude
– Some programmer dude, Commented May 4, 2020 at 8:09
Can you please put that as an answer, i read that it would cause issue because the array would be allocated on stack, fyi I'm new to this. — AppDeveloper
– AppDeveloper, Commented May 4, 2020 at 8:36

MikeCAT · Accepted Answer · 2020-05-04 08:06:34Z

1

Using strlen(), the allocation lack for space for terminating null-character.
The last element of what is pointed at by interestes, which should be NULL, is not initialized.

fixed version:

static char **getList() {
    char **list = (char *[]){"ABC", "DEF", NULL};
    char **interests = malloc(sizeof(char*) * 3);
    int i = 0;
    while(*list) {
        interests[i] = malloc(sizeof(char) * (strlen(*list) + 1)); /* allocate one more */
        strcpy(interests[i++], *list);
        list++;
    }
    interests[i] = NULL; /* initialize the last element */
    return interests;
}

answered May 4, 2020 at 8:06

MikeCAT

75.1k11 gold badges50 silver badges71 bronze badges

Sign up to request clarification or add additional context in comments.

8 Comments

AppDeveloper Over a year ago

strlen(*list) + 1, this one more automatically puts null character, just a question, it was working without this extra character, mind help me understand this better. Thanks.

MikeCAT Over a year ago

@AppDeveloper Accessing out-of-bounds invokes undefined behavior. You are unlucky because it happened to seem working well and they didn't gave you a chance to notice the bug.

AppDeveloper Over a year ago

Thanks for the explanation. in comments above, it was suggested that since the array contents are not modified (it's true), I should use plain simple array of pointers, do you agree?

MikeCAT Over a year ago

It depends on situration. Even if the contents are not modified, it is not guaranteed that the array or its contents will not be freed. You must not pass pointers that are not returned by malloc() family to free().

MikeCAT Over a year ago

Doing trial and error is good. If it is supported in your environment, AddressSanitizer is helpful to detecting memory management errors.

|

Some programmer dude · Accepted Answer · 2020-05-04 08:57:09Z

You do some... weird things in the code you show. To begin with we have the list variable, which should be a simple and plain array instead.

And if the array or its contents should not be modified then you could even create an array whose life-time last the full run-time of the program, using static:

static const char * const *getList() {
    static const char * const list[] = { "ABC", "DEF", NULL };
    return list;
}

The declaration const char * const list[] declares list as an array (whose size will be deduced from the initialization) of constant pointers to constant characters. That is, the pointers inside list can't be modified, and the string contents itself can't be modified.

If you have other requirements, like being able to modify the strings, then you could still use the definition shown above in my answer, but create a new array to return.

There are a few improvements that can be made to the allocation and copying code as well. For example you could programatically get the size of the array list. I also recommend that you use a simple index loop instead of the pointer arithmetic loop you have now, and with the correct size of the list array the loop will include copying the terminating NULL as well.

All in all it could look something like

static char **getList() {
    static const char * const list[] = { "ABC", "DEF", NULL };

    // Get the number of elements in the array
    size_t list_size = sizeof list / sizeof list[0];

    // sizeof *interests is the size of each element we want to allocate
    // multiply it by the number of elements we need
    char **interests = malloc(sizeof *interests * list_size);

    // Do a deep copy of the strings from list
    for (size_t i = 0; i < list_size ++i) {
        if (list[i] == NULL) {
            interests[i] = NULL;
        } else {
            interests[i] = malloc(strlen(list[i]) + 1);  // +1 for the string terminator
            strcpy(interests[i], list[i]);

            // Or if your system have the strdup function (which is quite common)
            // interests[i] = strdup(list[i]);
        }
    }

    return interests;
}

If you don't need a deep copy, then the copying loop could be:

for (size_t i = 0; i < list_size ++i) {
    interests[i] = list[i];
}

Collectives™ on Stack Overflow

c string array with null fails

2 Answers 2

8 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

8 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related