1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ BTW, if you're not confident of memory-management, it's wise to exercise your functions under Valgrind or some other checker - have you done that? \$\endgroup\$ – Toby Speight Mar 19 at 15:48
  • \$\begingroup\$ Welcome to Code Review! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Please check that I haven't misrepresented your code, and correct it if I have. \$\endgroup\$ – Toby Speight Mar 19 at 15:49
  • \$\begingroup\$ @TobySpeight I've never looked into it, honestly. I've only relatively recently started getting serious about working with compiled languages. Will do. \$\endgroup\$ – Ethan McTague Mar 19 at 15:49
3
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ Oh, that first invalid free() is just a typo in my example usage, I should be passing first_item instead of &first_item. Thank you, though! I've fixed the leak. What should I do about my realloc checks? I figure if it fails the program'll have to terminate either way at some point so it may as well do it just by tripping up on an unexpected NULL value - is there a way to recover from a failed realloc and still obtain some memory? \$\endgroup\$ – Ethan McTague Mar 19 at 16:12
  • \$\begingroup\$ No, don't do that - for two reasons. Firstly, if you're implementing a library, you don't know the calling program's recovery strategy; you want to return an error indication and do nothing else. Secondly, dereferencing a null pointer is Undefined Behaviour; there's no guarantee that your program will be terminated. It will just be wrong. \$\endgroup\$ – Toby Speight Mar 19 at 16:15
  • \$\begingroup\$ BTW, this is just a very quick review, as I'm departing for a few days; you might want to hang on for some more considered reviews before accepting (unless you plan to post a follow-up question with improved/fixed code). \$\endgroup\$ – Toby Speight Mar 19 at 16:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.