Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

C memory leak despite free

In debugging my program with Valgrind, I have discovered a memory leak despite what I thought were effective calls to free. First, the code that is allocating the memory and storing it:

    row = malloc(sizeof(Row));
    row->columns = malloc(sizeof(char*) * headcnt);
    row->numcol  = 0;

    ...

    row->numcol    = colcnt;
    rows           = realloc(rows, (rowcnt+1) * sizeof(Row));
    rows[rowcnt++] = *row;

The code responsible for attempting to free the memory:

void cleanUp(){
    int i = 0;
    int j = 0;

    for (i = 0; i < rowcnt; i++){
        for (j = 0; j < rows[i].numcols; j++){
            free(rows[i].columns[j]);
        }
        free(&rows[i]);
    }
    free(rows); 
    exit(0);
}

The declaration of Row:

typedef struct {
    char** columns;
    unsigned short int numcol;
} Row;

Row* rows = NULL;

Worse still, this program sometimes causes a glibc error at free(&rows[i]) that complains of a double free. I am new to C, and would appreciate any pointers (ahem) someone might have.

like image 311
zchtodd Avatar asked Feb 08 '11 14:02

zchtodd


2 Answers

Doing rows[rowcnt++] = *row; effectively makes a copy of the memory you allocated. Your array rows should be an array of pointers. Also like Oli Chalesworth pointed out, you free for columns should be a single free for all the columns.

rows = malloc(count * sizeof(Row*)); // This is probably done somewhere

row->columns = malloc(sizeof(char*) * headcnt);
row->numcol  = 0;

...

row->numcol    = colcnt;
rows           = realloc(rows, (rowcnt+1) * sizeof(Row*));
rows[rowcnt++] = row;

Now if your cleanup

void cleanUp(){
    int i = 0;
    int j = 0;

    for (i = 0; i < rowcnt; i++){
        free(rows[i]->columns);
    }
    free(rows); 
    exit(0);
}
like image 191
Rod Avatar answered Sep 23 '22 08:09

Rod


Every call to malloc (or realloc) must be matched with a corresponding call to free. If you dynamically allocate an array thus:

int *p = malloc(sizeof(int) * NUM);

You free it like this:

free(p);

Not like this:

for (int i = 0; i < NUM; i++)
{
    free(p[i]);
}

It appears that you are doing this incorrectly. I suspect that your cleanup code ought to be:

void cleanUp(){
    int i = 0;
    int j = 0;

    for (i = 0; i < rowcnt; i++){
        for (j = 0; j < rows[i].numcols; j++){
            free(rows[i].columns[j]); // Free whatever rows[i].columns[j] points to
        }
        free(rows[i].columns); // Matches row->columns = malloc(sizeof(char*) * headcnt);
    }
    free(rows);  // Matches rows = realloc(rows, (rowcnt+1) * sizeof(Row));
    exit(0);
}

Also, there is no way to match the row = malloc(sizeof(Row));. I suspect that your allocation code ought to be:

row->numcol    = colcnt;
rows           = realloc(rows, (rowcnt+1) * sizeof(Row));
rows[rowcnt].columns = malloc(sizeof(char*) * headcnt);
rows[rowcnt].numcol = 0;
rowcnt++;
like image 39
Oliver Charlesworth Avatar answered Sep 25 '22 08:09

Oliver Charlesworth