List in C Error - Stack around the variable 'memory' was corrupted

Question

I am trying to implement a dynamic List in C and I do not get why I get the "Stack around the variable 'memory' was corrupted." Error when I am trying to add a second Item to the list.

struct LIST {
    unsigned int size;
    unsigned int count;
    void** items;
};

struct LIST* list_init(unsigned int size)
{
    struct LIST* createList = (struct LIST*)calloc(1, sizeof(struct LIST));
    void* memory = calloc(size, sizeof(void*));
    if (memory == 0 || createList == 0) return 0;

    createList->items = &memory;
    createList->size = size;

    return createList;
}

void list_add(struct LIST* list, void* item)
{
    if (list->count + 1 > list->size)
    {
        void* memory = realloc(list->items, (sizeof(void*)) * list->count + 1);
        list->items = &memory;
        return;
    }
    else
    {
        list->items[list->count] = item;
        list->count = list->count + 1;
    }
}

int main()
{
    const char* str = "Hello";
    struct LIST* list = list_init(2);

    list_add(list, (void*)str);
    list_add(list, (void*)str);
    list_add(list, (void*)str);
    list_add(list, (void*)str);

    printf("Items in List: %d \n\n", list_count(list));

    list_clear(list);

    free(list);
    return 0;
}

The code never goes into the if block that could do anything with the memory variable, however in the second list_add Call it crashes when exiting the function (Note I am using the VS2022 Debugger to step through it).

I expect to have 2 items added to the list before realloc is used to extend the list by one.

Answer 1

A few issues ...

1. In LIST, items should be just void * and not void **
2. In list_add, memory is a stack local variable that goes out of scope. Change list->items = &memory to list->items = memory
3. You just use list->count + 1 in the realloc. We want to increment list->size instead and use that in the call.
4. The actual store code (in the else block) should be executed unconditionally
5. The storing code tries to dereference a void * pointer
6. Because the list is storing strings (i.e. char * arrays), we need to duplicate the string when storing. So, we should use strdup
7. Because of the new strdup, just doing free(list) at the end is insufficient. We need to loop through all elements and free the strings we duplicated in list_add.
8. list_init is replicating code in list_add. It should just do the first calloc only.
9. Because we can pass in string literals to list_add (e.g. list_add(list,"Hello");, the 2nd arg should have const.

---

Here is the refactored code. It is annotated with the bugs and fixes:

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

struct LIST {
    unsigned int size;
    unsigned int count;
#if 0
    void **items;
#else
    void *items;
#endif
};

struct LIST *
list_init(unsigned int size)
{
#if 0
// NOTE/BUG: do _not cast the return of calloc (or malloc/realloc)
    struct LIST *createList = (struct LIST *) calloc(1, sizeof(struct LIST));
#else
    struct LIST *createList = calloc(1, sizeof(*createList));
#endif

#if 0
// NOTE/BUG: this replicates code in list_add
    void *memory = calloc(size, sizeof(void *));

    if (memory == 0 || createList == 0)
        return 0;

    createList->items = &memory;
    createList->size = size;
#endif

    return createList;
}

#if 0
// NOTE/BUG: this should be a const pointer because we can pass in a string
// literal
void
list_add(struct LIST *list, void *item)
#else
void
list_add(struct LIST *list, const void *item)
#endif
{

    if (list->count + 1 > list->size) {
#if 0
// NOTE/BUG: we need to increment list->size
        void *memory = realloc(list->items, (sizeof(void *)) * list->count + 1);
#else
        list->size += 3;
        void *memory = realloc(list->items, (sizeof(void *)) * list->size);
#endif

#if 0
// NOTE/BUG: the variable 'memory' is on the stack and goes out of scope
        list->items = &memory;
        return;
#else
        list->items = memory;
#endif
    }

#if 0
// NOTE/BUG: this should be executed unconditionally
// NOTE/BUG: we need to duplicate the 'item' string
// NOTE/BUG: we can _not_ dereference a void * pointer
    else {
        list->items[list->count] = item;
        list->count = list->count + 1;
    }
#else
    char **items = list->items;
    items[list->count] = strdup(item);
    list->count = list->count + 1;
#endif
}

#if 1
void
list_print(struct LIST *list)
{

    printf("Items in List: %d\n", list->count);

    const char **items = list->items;
    if (items != NULL) {
        for (unsigned int idx = 0;  idx < list->count;  ++idx, ++items)
            printf("Item: %s\n",*items);
    }
}

void
list_free(struct LIST *list)
{

    char **items = list->items;

    if (items != NULL) {
        for (unsigned int idx = 0;  idx < list->count;  ++idx, ++items)
            free(*items);
    }

    free(list);
}

#endif

int
main(void)
{
    const char *str = "Hello";
    struct LIST *list = list_init(2);

#if 0
// NOTE/BUG: no need to cast str and can be detrimental as in masks the
// const nature of a string literal as below
    list_add(list, (void *) str);
    list_add(list, (void *) str);
    list_add(list, (void *) str);
    list_add(list, (void *) str);
#else
    list_add(list, str);
    list_add(list, "abc");
    list_add(list, "def");
    list_add(list, "ghi");
    list_add(list, "jkl");
#endif

#if 0
    printf("Items in List: %d \n\n", list_count(list));

    list_clear(list);
#else
    list_print(list);
#endif

#if 0
    free(list);
#else
    list_free(list);
#endif

    return 0;
}

---

In the code above, I've used cpp conditionals to denote old vs. new code:

#if 0
// old code
#else
// new code
#endif

#if 1
// new code
#endif

Note: this can be cleaned up by running the file through unifdef -k

---

Here is the program output:

Items in List: 5
Item: Hello
Item: abc
Item: def
Item: ghi
Item: jkl

---

Here is the cleaned up code:

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

struct LIST {
    unsigned int size;
    unsigned int count;
    void *items;
};

struct LIST *
list_init(unsigned int size)
{
    struct LIST *createList = calloc(1, sizeof(*createList));

    return createList;
}

void
list_add(struct LIST *list, const void *item)
{

    if (list->count + 1 > list->size) {
        list->size += 3;
        void *memory = realloc(list->items, (sizeof(void *)) * list->size);

        list->items = memory;
    }

    char **items = list->items;
    items[list->count] = strdup(item);
    list->count = list->count + 1;
}

void
list_print(struct LIST *list)
{

    printf("Items in List: %d\n", list->count);

    const char **items = list->items;
    if (items != NULL) {
        for (unsigned int idx = 0;  idx < list->count;  ++idx, ++items)
            printf("Item: %s\n",*items);
    }
}

void
list_free(struct LIST *list)
{

    char **items = list->items;

    if (items != NULL) {
        for (unsigned int idx = 0;  idx < list->count;  ++idx, ++items)
            free(*items);
    }

    free(list);
}

int
main(void)
{
    const char *str = "Hello";
    struct LIST *list = list_init(2);

    list_add(list, str);
    list_add(list, "abc");
    list_add(list, "def");
    list_add(list, "ghi");
    list_add(list, "jkl");

    list_print(list);

    list_free(list);

    return 0;
}