C - Unable to free memory allocated within a linked list structure

Question

Consider the following code snippet

  struct node {
    char *name;
    int m1;
    struct node *next;
    };

    struct node* head = 0; //start with NULL list

    void addRecord(const char *pName, int ms1)
    {   
        struct node* newNode = (struct node*) malloc(sizeof(struct node)); // allocate node

        int nameLength = tStrlen(pName);
        newNode->name = (char *) malloc(nameLength);
        tStrcpy(newNode->name, pName);

        newNode->m1 = ms1;
        newNode->next = head; // link the old list off the new node
        head = newNode;
    }

    void clear(void)
    {
        struct node* current = head;
        struct node* next;
        while (current != 0) 
        {
            next = current->next; // note the next pointer
    /*      if(current->name !=0)
            {
                free(current->name);
            }
    */
            if(current !=0 )
            {
                free(current); // delete the node
            }
            current = next; // advance to the next node
        }
        head = 0;
    }

Question: I am not able to free current->name, only when i comment the freeing of name, program works. If I uncomment the free part of current->name, I get Heap corruption error in my visual studio window. How can I free name ?

Reply:

@all,YES, there were typos in struct declaration. Should have been char* name, and struct node* next. Looks like the stackoverflow editor took away those two stars.

The issue was resolved by doing a malloc(nameLength + 1). However,If I try running the old code (malloc(namelength)) on command prompt and not on visual studio, it runs fine. Looks like, there are certain compilers doing strict checking.

One thing that I still do not understand is , that free does not need a NULL termination pointer, and chances to overwrite the allocated pointer is very minimal here.

user2531639 aka Neeraj

[Don't cast the return value of `malloc()` in C](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — unwind, Jun 28 '13 at 11:35

hmjd · Accepted Answer · 2013-06-28T11:45:41.303

7

This is writing beyond the end of the allocated memory as there is no space for the null terminating character, causing undefined behaviour:

newNode->name = (char *) malloc(nameLength);
tStrcpy(newNode->name, pName);

To correct:

newNode->name = malloc(nameLength + 1);
if (newNode->name)
{
    tStrcpy(newNode->name, pName);
}

Note calling free() with a NULL pointer is safe so checking for NULL prior to invoking it is superfluous:

free(current->name);
free(current);

Additionally, I assume there are typos in the posted struct definition (as types of name and next should be pointers):

struct node {
    char* name;
    int m1;
    struct node* next;
};

edited Jun 28 '13 at 11:45

answered Jun 28 '13 at 11:32

hmjd

120,187
20
207
252

I agree that the allocation leaves no room for its proper NULL, however I don't think it's highly likely that the buffer is actually being overwritten in this case. The true buffer size malloc carves out is almost always larger by at least the one byte that gets overrun here. Bad code, sure, but it doesn't seem reasonable to me that this is the cause of the OP's failure. That being said, I'm assuming that `name` is a `char *`, not a `char` as provided above. – mah Jun 28 '13 at 11:35
It may carve out extra space, but that doesn't mean `free` won't still check that extra space for modifications. – Drew McGowen Jun 28 '13 at 12:35
If `char name;` wasn't a typo it would strongly participate in keeping `free(current->name` from working. However I'm not sure whether `malloc()`ing to `name` would even compile. – alk Jun 28 '13 at 16:03
@alk, I am fairly confident it is a typo as it would definitely not compile if `struct node next;` was used and not `struct node* next;` as `struct node` is not fully defined. – hmjd Jun 28 '13 at 16:12
@hmjd: fully agree for `struct node next;`, however although the latter could by a typo and the former could very well be a bug. – alk Jun 28 '13 at 16:17

C - Unable to free memory allocated within a linked list structure

1 Answers1

Linked