Dynamically-resizable array implementation, for use in a game-dev-related library

Question

I've been working on a library that includes a list-like structure, which I use internally multiple times to store different types of structs. However, I'm not entirely confident with my ability to safely manage memory in C yet, so I'd appreciate if someone could let me know if there's anything wrong with my code (it seems to work just fine.)

This is the code for the list structure (with some bits from the associated header put at the top for simplicity.)

// Put this at the top of structs to make it possible to use them in a cg_mem_list #define CG_ITEMIZABLE int cg_index; // Free extra spaces after this many exist #define CG_MEM_EXTRAS 10 typedef struct cg_mem_item { CG_ITEMIZABLE } cg_mem_item; typedef struct cg_mem_list { cg_mem_item** items; int num, capacity, itemsize; } cg_mem_list; void cg_new_mem_list (cg_mem_list* list, int itemsize) { list->items = 0; list->num = 0; list->capacity = 0; list->itemsize = itemsize; } cg_mem_item* cg_mem_list_add (cg_mem_list* list) { cg_mem_item* mitem = malloc (list->itemsize); if (list->capacity - list->num < 1) { list->capacity++; list->items = realloc (list->items, list->capacity * sizeof (cg_mem_item*)); } mitem->cg_index = list->num; list->items[list->num++] = mitem; return mitem; } void cg_mem_list_remove (cg_mem_list *list, cg_mem_item *item) { list->num--; int index = item->cg_index; if (index < list->num) { list->items[index] = list->items[list->num]; list->items[index]->cg_index = index; } if (list->capacity - list->num > CG_MEM_EXTRAS) { list->capacity = list->num; list->items = realloc (list->items, list->capacity * sizeof (cg_mem_item*)); } free (item); } void cg_destroy_mem_list (cg_mem_list* list) { free (list->items); } 

This is how I use it:

// The struct being stored in the list typedef struct some_data_type { CG_ITEMIZABLE int random_data; } some_data_type; int main (void) { // Declare and define the list cg_mem_list data_list; cg_new_mem_list (&data_list, sizeof(some_data_type)); // Allocates a new item and sets it up some_data_type* first_item = (some_data_type*)cg_mem_list_add(&data_list); first_item->random_data = 12; // <More additions and operations> // This frees the item cg_mem_list_remove(&data_list, (cg_mem_item*)&first_item); } 

I'd also like to note that I didn't implement any checks malloc or realloc because this is in a game-dev-related library, and I figure if it can't allocate any new memory it's probably gonna crash and burn either way. If you have any suggestions for how to gracefully handle this (other than just straight-up throwing an error and aborting, which is pretty much the same as crashing anyways for this sort of application honestly) I'd appreciate it.

Answer 1

Use a memory checker

There's a clear bug, identified by running the test program under Valgrind:

==21190== Invalid free() ==21190== at 0x48369AB: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==21190== by 0x109320: cg_mem_list_remove (215759.c:64) ==21190== by 0x109388: main (215759.c:94) ==21190== Address 0x1fff0006a8 is on thread 1's stack ==21190== in frame #2, created by main (215759.c:82) 

This is where a local variable, not allocated with malloc() has been passed to free().

There's also a leak:

==21190== 16 (8 direct, 8 indirect) bytes in 1 blocks are definitely lost in loss record 2 of 2 ==21190== at 0x48356AF: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==21190== by 0x4837DE7: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so) ==21190== by 0x1091F8: cg_mem_list_add (215759.c:36) ==21190== by 0x109366: main (215759.c:88) 

This is only partially mitigated if we add a call to cg_destroy_mem_list, because the individual items still don't get reclaimed (that's what the "8 indirect" refers to).

Other issues

We need to include <stdlib.h>, to use malloc() and family.

We're completely missing the (necessary) error checking when we call malloc() and realloc(), both of which can return null pointers. Remember to check the result of realloc()before overwriting the old pointer (which is still valid if realloc() failed).