Memory management and realloc

Question

I'm going through my program with valgrind to hunt down memory leaks. Here's one that I'm not sure what to do with.

==15634== 500 (224 direct, 276 indirect) bytes in 2 blocks are definitely lost in loss record 73 of 392
==15634==    at 0x4007070: realloc (vg_replace_malloc.c:429)
==15634==    by 0x807D5C2: hash_set_column(HASH*, int, char const*) (Hash.cpp:243)
==15634==    by 0x807BB15: LCD::PluginDiskstats::PluginDiskstats() (PluginDiskstats.cpp:102)
==15634==    by 0x806E021: LCD::Evaluator::Evaluator() (Evaluator.cpp:27)
==15634==    by 0x8066A87: LCD::LCDControl::LCDControl() (LCDControl.h:16)
==15634==    by 0x80667F5: main (Main.cpp:8)

Here's the code:

/* add an entry to the column header table */
void hash_set_column(HASH * Hash, const int number, const char *column)
{
    if (Hash == NULL)
        return;

    Hash->nColumns++;
    Hash->Columns = (HASH_COLUMN *)realloc(Hash->Columns, Hash->nColumns * sizeof(HASH_COLUMN)); // line 243
    Hash->Columns[Hash->nColumns - 1].key = strdup(column);
    Hash->Columns[Hash->nColumns - 1].val = number;

    qsort(Hash->Columns, Hash->nColumns, sizeof(HASH_COLUMN), hash_sort_column);

}

Should I be doing something here in regards to memory management?

Answer 1

If realloc() fails it returns null and the original block is not freed. This line:

Hash->Columns = (HASH_COLUMN *)realloc(Hash->Columns, Hash->nColumns * sizeof(HASH_COLUMN)); // line 243

doesn't check the return value. So if realloc() fails null is written into Hash->Columns and the original block is leaked.

Answer 2

The problem is that if realloc() fails the function will return NULL but the original block will still be allocated. However, you've just overwritten the pointer to that block and can't free (or use) it anymore.

Answer 3

Not sure this is the issue, but it is potentially problematic. From the manpage for realloc():

RETURN VALUE

Upon successful completion with a size not equal to 0, realloc() returns a pointer to the (possibly moved) allocated space. If size is 0, either a null pointer or a unique pointer that can be successfully passed to free() is returned. If there is not enough available memory, realloc() returns a null pointer and sets errno to [ENOMEM].

What will happen is, if there isn't enough room for the expanded object, the old object is still valid and isn't freed, but realloc() returns NULL. So you should store the return result of realloc() in a separate variable, check that variable for NULL, and if it isn't, assign it to Hash->Columns.

Answer 4

Aha - asveikau 's comment:

but if that were the leak he's seeing in valgrind, there'd be a null dereference and he'd crash. my guess is there's some code he's not providing that has a leak.

has led me to another problem - the data structure you're resizeing contains pointers to strings allocated with strdup(). if your realloc() call is making the allocation smaller, you will lose those pointers without properly freeing them first. I believe that asveikau is correct that this is what valgrind is complaining about.

Answer 5

On Mac OS X and FreeBSD et al. you also have the reallocf() function:

man 3 malloc | less -p reallocf 
... 
     The reallocf() function is identical to the realloc() function, except
 that it will free the passed pointer when the requested memory cannot be
 allocated.  This is a FreeBSD specific API designed to ease the problems
 with traditional coding styles for realloc causing memory leaks in
 libraries.

Answer 6

Valgrind isn't saying that the leak is happening at the realloc line - it's saying that the memory that was allocated by that realloc line is the memory that's getting leaked, eventually. Valgrind doesn't know where though - it just knows that you don't have a reference to that memory anymore, so it'd be impossible to free it. (The OP may know this, but it's clear that many of the answerers don't!)

In short, the code you've pasted isn't causing the issue (although the issue that Michael Burr raises is definitely real, but since you're not even checking for a NULL returned from realloc...)

Somewhere in your code, there should be a free(Hash->Columns) which isn't there now. Find this place - probably just before the Hash itself is freed - and add it.

Answer 7

Here is a code that can assist you in finding errors:
debuglogger
debugalloc