C freeing multi-dimensional array inside a struct seems incorrect

Question

I've seen another question for allocating and freeing multi-dimensional arrays, but I suspect that it does not free correctly. For testing I made this small code extracted from my main code. I compiled it under MacOS X.10 with XCode or gcc 4.9 with same results:

Faulty code

It runs 200000 times and the memory consumption grows up to 20GB!:

#include <stdlib.h>
typedef struct{
    int lonSize;
    int latSize;
    double **grid;
}raf09_grid_t;
static raf09_grid_t raf09_grid;

void free_raf09_grid() {
    if (raf09_grid.grid != NULL) {
            int i;
                for (i = 0; i < raf09_grid.lonSize; ++i) {
                    free(raf09_grid.grid[i]);
                }
                free(raf09_grid.grid);
    }
    raf09_grid.latSize = 0;
    raf09_grid.lonSize = 0;
}

void get_raf09_grid() {
    int nbElLat=381;
    int nbElLon=421;
    raf09_grid.grid = malloc(nbElLon*sizeof(double*));
    int it;
    for(it=0;it<nbElLon;it++)
          raf09_grid.grid[it] = malloc(nbElLat*sizeof(double));
    int i,j;
    for(i=0;i<420;i++) {
        for(j=0;j<380;j++) {
           raf09_grid.grid[i][j]=0.0;
        }
    }
}

int main (int argc, char *argv[]) {
   int i=0;
   for (i=0;i<20000;i++) {
       get_raf09_grid();
       free_raf09_grid();
   }
   return 0;
}

I'm newbie so I suspect that my freeing is not correct...

Corrected code

With your help I corrected my code, it is now corrects and takes only 10M in ram:

#include <stdlib.h>
typedef struct{
    int lonSize;
    int latSize;
    double **grid;
}raf09_grid_t;
static raf09_grid_t raf09_grid;

void free_raf09_grid() {
    if (raf09_grid.grid != NULL) {
            int i;
                for (i = 0; i < raf09_grid.lonSize; ++i) {
                    free(raf09_grid.grid[i]);
                }
                free(raf09_grid.grid);
    }
    raf09_grid.latSize = 0;
    raf09_grid.lonSize = 0;
}

void get_raf09_grid() {
    raf09_grid.latSize=381;
    raf09_grid.lonSize=421;
    raf09_grid.grid = malloc(raf09_grid.lonSize*sizeof(double*));
    int it;
    for(it=0;it<raf09_grid.lonSize;it++)
          raf09_grid.grid[it] = malloc(raf09_grid.latSize*sizeof(double));
    int i,j;
    for(i=0;i<420;i++) {
        for(j=0;j<380;j++) {
           raf09_grid.grid[i][j]=0.0;
        }
    }
}

int main (int argc, char *argv[]) {
   int i=0;
   for (i=0;i<20000;i++) {
       get_raf09_grid();
       free_raf09_grid();
   }
   return 0;
}

You're allocating `nbElLat*sizeof(double)` but just freeing `nbElLon*sizeof(double*)`. You need to loop through latSize and free those elements aswell (eg. each of grid[0 to nbElLon-1][0 to nbEllat-1]). — ccKep, Aug 10 '14 at 11:04
Purely as a matter of interest - how are you measuring the memory usage? — roelofs, Aug 10 '14 at 11:08
Note that you fail to initialize the last column and row (you have 421 rows but you loop 420 times, etc.). To avoid bugs, instead of `i<420` do `i — M.M, Aug 10 '14 at 11:26

roelofs · Answer 1 · 2014-08-10T11:38:50.567

Valgrind is an invaluable tool in tracing memory leaks. Compiling your source code with debug information, and running it with:

valgrind --leak-check=full -v ./a.out

will give the following summary:

==7033== HEAP SUMMARY:
==7033==     in use at exit: 1,283,208,000 bytes in 421,000 blocks
==7033==   total heap usage: 422,000 allocs, 1,000 frees, 1,286,576,000 bytes allocated
==7033== 
==7033== Searching for pointers to 421,000 not-freed blocks
==7033== Checked 92,040 bytes
==7033== 
==7033== 18,288 bytes in 6 blocks are possibly lost in loss record 1 of 2
==7033==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7033==    by 0x400611: get_raf09_grid (grid.c:27)
==7033==    by 0x4006A8: main (grid.c:39)
==7033== 
==7033== 1,283,189,712 bytes in 420,994 blocks are definitely lost in loss record 2 of 2
==7033==    at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7033==    by 0x400611: get_raf09_grid (grid.c:27)
==7033==    by 0x4006A8: main (grid.c:39)
==7033== 
==7033== LEAK SUMMARY:
==7033==    definitely lost: 1,283,189,712 bytes in 420,994 blocks
==7033==    indirectly lost: 0 bytes in 0 blocks
==7033==      possibly lost: 18,288 bytes in 6 blocks
==7033==    still reachable: 0 bytes in 0 blocks
==7033==         suppressed: 0 bytes in 0 blocks
==7033== 
==7033== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 2 from 2)
--7033-- 
--7033-- used_suppression:      2 dl-hack3-cond-1
==7033== 
==7033== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 2 from 2)

The link to line 27 shows that there is a problem with this statement:

raf09_grid.grid[it] = malloc(nbElLat*sizeof(double));

You are allocating more memory here than is being freed later in the program.

Update the raf09_grid.lonSize variable in get_raf09_grid() to equal nbElLon, and it fixes the problem.

In general, every malloc should have a corresponding free. Once you know which malloc is leaking, then you can find the code that is supposed to free that variable, and debug from there.

Note: I reduced the loop from 20000 to 1000, but it will give you the same information.

score 2 · Answer 2 · answered Aug 10 '14 at 11:51

After reading through yoiur question, here are a few suggestions:

In the code:

for(i=0;i<420;i++) {
    for(j=0;j<380;j++) {
       raf09_grid.grid[i][j]=0.0;
    }

you are looping i and j for 420 and 380, while you've defined :

int nbElLat=381;
int nbElLon=421;

So, you never process the 420th iteration for the Longitudes. The same with missing out on the 380th loop for Latitude. If that's a desirable thing, its fine, else you should fix it to something like this:

for(i=0;i<nbElLon;i++) { //better use macros or variable names than just plain magic numbers
    for(j=0;j<nbElLat;j++) {
       raf09_grid.grid[i][j]=0.0;
    }

Second, in your free_raf09_grid() function, you use:

for (i = 0; i < raf09_grid.lonSize; ++i) {

but you haven't initialized that variable anywhere.

Perhaps in function get_raf09_grid() just before the declaration of int i,j do this:

raf09_grid.lonSize = nbElLon; 
raf09_grid.latSize = nbElLat;

The third important one. In the lines below:

raf09_grid.grid = malloc(nbElLon*sizeof(double*));
int it;
for(it=0;it<nbElLon;it++)
      raf09_grid.grid[it] = malloc(nbElLat*sizeof(double));

you should check if the mallocs have returned success or not aka check for NULL.

score 1 · Accepted Answer · answered Aug 10 '14 at 11:06

1

your for loop uses lonSize as a parameter, but it doesn't get updated anywhere

answered Aug 10 '14 at 11:06

Shlomi Agiv

1,183
7
17

3

Please expand on this to make it a useful answer, otherwise it should probably just be a comment. – Paul R Aug 10 '14 at 11:09

C freeing multi-dimensional array inside a struct seems incorrect

Faulty code

Corrected code

3 Answers3