memory free throws segFault

Question

I am implementing symbol table using link list, The code works fine but there is memory leak in code,

I have following structure

struct node
{
  char* pcKey;
  void* pvValue;
  struct node *next;
};

struct _Sym
{
  int totalBindings;
  struct node *node;
};

add I have sym_new method to allocate memory for sym instance

sym Sym_new (void)
{
    _Sym *m_SymTable_t = (_Sym*) malloc (sizeof(_Sym));

    if(m_SymTable_t == NULL)
    {
        return NULL;
    }
    else
    {
        m_SymTable_t->totalBindings = 0;
        m_SymTable_t->node = NULL;
        return m_SymTable_t;
    }//endif
}

I am allocating memory for key and value in other function based on the string length.

The free method is

typedef struct _Sym *Sym;

void Sym_free (Sym m_SymTable_t)
{
    assert(m_SymTable_t != NULL);

    struct node* temp = m_SymTable_t->node;

    struct node *currentBinding = NULL;
    while(temp != NULL)
    {
        currentBinding = temp;
        temp = temp -> next;

            //Removing comment for the below line throws segfault
        //free(currentBinding -> pcKey);
        //free(currentBinding -> pvValue);

        free(currentBinding);
    }

    free(m_SymTable_t);
}

What is proper way to free the sym completely?

I have uploaded my symTable_Link.cpp file at link

It looks like you didn't initialize pcKey and pvValue to NULL in the allocator function? — Jarhmander, Feb 06 '14 at 17:01
Also useful may be: http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc - why not to cast return of `malloc`. It's not what's wrong with the code... just a comment — Jimbo, Feb 06 '14 at 17:02
Where do you allocate memory for pcKey and pvValue? Also, *please* don't use Hungarian notation to encode primitive type information in the variable name; that's not what it's meant for. — John Bode, Feb 06 '14 at 17:02
I am allocating memory for pcKey and PcVal in another function called sym_put — Meluha, Feb 06 '14 at 17:03
Do you always call `sym_put()` for each entry? If not you should initialise the pointers to `null` in the `Sym_new` function. `free()` will ignore `null` pointers but will segfault on dangling pointers or pointers that contain junk - the `malloc`ed memory is not necessarily zero. — Jimbo, Feb 06 '14 at 17:04
Is it possible that you deallocate your _Sym before allocating memory for `pcKey` and `pcVal`? — Jarhmander, Feb 06 '14 at 17:04
@Sagar, could you check? Initialise the pointers with say, 0xdeadbeaf, and then in your `sym_free` function check for this magic number... if it is found it indicates the a `sym_put` got missed... — Jimbo, Feb 06 '14 at 17:09
@Jarhmander No, _sym is freed only in Sym_free that to at last. — Meluha, Feb 06 '14 at 17:09
I have uploaded my symTable_Link.cpp file at [link](http://tempsend.com/62CCDBF79A) — Meluha, Feb 06 '14 at 17:15
When I free a pointer, I usually set it explicitly to NULL afterwards. That way you don't accidentally free it twice. Note also in your linked code you do `malloc(sizeof(char)*strlen(pcKey))` - you need `malloc(sizeof(char) *(strlen(pcKey) + 1))` to account for terminating `'\0'`. And I think `sizeof(char)` is `1` by definition, but that doesn't mean it's wrong to be explicit about it... — Floris, Feb 06 '14 at 17:49

score 1 · Accepted Answer · edited May 23 '17 at 10:31

The variables pcKey and pvValue should probably be initialised to null in the Sym_new() function. Otherwise they may contain any old value. This is because malloc doesn't necessarily zero the memory allocated: it just allocates a chunk of memory and the memory could therefore be filled with junk.

So, if for some reason sym_put() is not called for the newly created object these pointers could point to invalid memory and upon your call to free() segfault. If you initialise them to null free() will just ignore them and won't try to free the memory.

A "hacky" DEBUG-only technique you could use to check that the pcKey and pvValue variables are definitely allocated by a sym_put call would be to initialise them in sym_new with a dummy value, for example 0xCDCDCDCD (careful about pointer-widths here... this is why I'm calling this a hacky technique). Then in sym_free check for this magic constant before freeing pcKey and pvValue. If you find it, there's the problem...

Also of interest may be the thread Do I cast the result of malloc?

EDIT:

Looked at the code linked and you appear to be discarding const!

The function id defined as:

int SymTable_put (SymTable_t m_SymTable_t, const char *pcKey, const void *pvValue)

But then does this cast...

temp->pcKey = (char*)pcKey;
temp->pvValue = (char*)pvValue;

This is a bad idea. You're "fooling" the compiler into invalidating your const promise.

THE BUG: Ok, so you allocate as follows

temp->pcKey = (char*) malloc (sizeof(char) * strlen (pcKey));

But then you overwrite this pointer using

temp->pcKey = (char*)pcKey;

So you a) have a memory leak and b) have just stashed the wrong pointer, which is probs why you get the segfault. You you probably meant to do instead is (strdup is useful here)...

temp->pcKey = strdup(pcKey);

This will allocate new memory for the string in pcKey and COPY the string into the new memory.

I would hazzard a guess you called the function like this...

SymTable_put (xxx, "KEY string", "VALUE string");

Then your code did this

temp->pcKey = (char*)malloc (sizeof(char) * strlen (pcKey));
...
temp->pcKey = (char*)pcKey;

So now temp->pcKey points to "KEY string" itself and not a copy of it. So when you try to free the string constant, your program complains. What you want to do is copy the string from pcKey into temp->pcKey instead of overwriting the pointer.

EDIT: As per comments the mallocs need space + 1 to include the null terminator. Also sizeof(char) is always 1, so is redundant. Try strdup instread.

i can see valid addresses for pcKey and pvValue, the exact implementation http://tempsend.com/62CCDBF79A — Meluha, Feb 06 '14 at 17:20
You need to `malloc` for `strlen + 1` - don't forget the `'\0'` terminator. — Floris, Feb 06 '14 at 17:43
Yep good point, I didn't spot that. +1 `strdup` will solve that problem by doing the allocate and copy in one go... — Jimbo, Feb 06 '14 at 17:48

memory free throws segFault

1 Answers1