C Programming free() Function Error

Question

So here's a beginner question about free() function in C Programming, when I try to free the deallocate the memory in the end using free() function I keep getting double free or corruption error or the loop won't end, I had the same issue with the other similiar function I wrote in the same program. When did I do wrong?

int addnode(struct node *add, struct node *p,int dep)
{
    int count=0;
    dep=1;
    struct node *roo=(struct node*)malloc(sizeof(struct node*));
    roo=p;
    while(roo){
        if(add->data >roo->data&&roo->right!=NULL)
        {
            roo=roo->right;
            dep++;
        }
        if(add->data <roo->data&&roo->left!=NULL)
        {
            roo=roo->left;
            dep++;
        }
        if(add->data >roo->data&&roo->right==NULL)
        {
            roo->right=add;
            dep++;
            break;
        }
        if(add->data <roo->data&&roo->left==NULL)
        {
            roo->left=add;
            dep++;
            break;
        }
        free(roo);
    }
    return dep;
}

`struct node *roo=...` causes a memory leak, because on the next line `roo` is overwritten with `p`. — DYZ, Feb 05 '17 at 04:49
You allocate `roo` but then immediately assign `roo = p;`. So, now the memory you allocated has _leaked_. — Chad, Feb 05 '17 at 04:49
@JohnKugelman Hi, Iam getting a double free or corruption error and I also edit the code format — woshidashen, Feb 05 '17 at 04:49
Probably because you free `p` elsewhere, while `roo` since it was overwritten with `p` is leaked. — Chad, Feb 05 '17 at 04:50
So a strict can evaluate to true if it is allocated, and you are expecting it to evaluate to false after first iteration of the while loop? — marshal craft, Feb 05 '17 at 04:50
It should also be `sizeof(struct node)`, not `sizeof(struct node*)`. Also also, don’t cast the return value of malloc. — Ry-, Feb 05 '17 at 04:58
[don't cast the result of `malloc` in C](http://stackoverflow.com/q/605845/995714) — phuclv, Feb 05 '17 at 05:08
thanks everyone for the feedback. I'm very new to programming in general so all the comments and answers have been super helpful to me, since I just noticed how amatuer the mistake i made was lol. — woshidashen, Feb 05 '17 at 05:15

score 2 · Accepted Answer · answered Feb 05 '17 at 04:50

On one line you allocate the memory and assign to roo, and on next line you immediately assign it another value:

struct node *roo=(struct node*)malloc(sizeof(struct node*));
roo=p;

The variable is reassigned several times later again. Therefore at the end it tries to free memory completely different than was allocated.

The algorithm seems to be quite unclear, I suggest properly naming the variables and specifying purpose of input and output. So far it seems that the node to be added is already allocated when the function is called so there shouldn't be any reason for any additional allocation within this function.

score 2 · Answer 2 · answered Feb 05 '17 at 04:50

Have you considered using a memory debugger such as valgrind? It took me thirty seconds of analysis to notice a common memory leak:

struct node *roo=(struct node*)malloc(sizeof(struct node*));
roo=p;

This leak itself won't cause what's ailing you, but you might want to fix them for performance benefits, otherwise you might end up with a program hogging all of your memory.

Following this, after you free(roo); without modifying roo, your loop test while(roo) makes use of a pointer that's invalid (as you've freed it); this is undefined behaviour, and what follows it (dereferencing that invalid pointer) is likely to cause the symptoms you describe. Valgrind would've picked this up.

C Programming free() Function Error

2 Answers2