CODEWITHSUNDEEP
cmemory-leaksmalloc
Esteban Mora
Esteban Mora

Reputation: 13

Memory leaking in C, malloc inside function

I am trying to create a 2d array of struct grid_t, and am getting memory leak warnings via address sanitiser, and eventually a seg fault in certain conditions.

There may be various points in my code causing this, but I thought knowing what was going wrong here would point me in the right direction to fixing the rest.

I am new to C and thus to memory management, so all feedback is welcome and appreciated!

void createGridArray(atom_t* ATOM) {
  ATOM -> grid = (grid_t**) malloc(WIDTH * sizeof(grid_t*));

  grid_t *nullGrid = malloc(sizeof(grid_t));
  grid_t temp = {NULL, 0};
  *nullGrid = temp;


  for (int i = 0; i < WIDTH; i++) {
    (ATOM -> grid)[i] = malloc(HEIGHT * sizeof(grid_t));
    for (int j = 0; j < HEIGHT; j++) {
        (ATOM -> grid)[i][j] = *nullGrid;
    }
  }
  //free(nullGrid); <- do I do this now?
  return;
}

Upvotes: 1

Views: 1763

Answers (3)

NoirDelire
NoirDelire

Reputation: 49

The C memory allocation is easier when you think it as a "loan".

Somehow, the system loan you some memory when you call malloc(), and you have to give it back with free().

Upvotes: 0

Peter
Peter

Reputation: 36637

Firstly, don't cast the return from malloc(). It is not required in C, and can obscure serious errors.

Second, don't hard-code the type into the malloc() call. For example,

ATOM->grid = (grid_t**) malloc(WIDTH * sizeof(grid_t*));

would be replaced by 

ATOM->grid = malloc(WIDTH * sizeof(*(ATOM->grid)));

This ensures the memory allocated is of the required size, regardless of what ATOM->grid is a pointer to.

To answer your question, to release all memory, you need to pass every nonNULL pointer returned by malloc() to free(). Exactly once.

So, if you allocate like this

ATOM->grid = malloc(WIDTH * sizeof(*(ATOM->grid)));

grid_t *nullGrid = malloc(sizeof(*nullGrid));
grid_t temp = {NULL, 0};
*nullGrid = temp;

for (int i = 0; i < WIDTH; i++)
{
    (ATOM -> grid)[i] = malloc(HEIGHT * sizeof(*((ATOM->grid)[i])));
    for (int j = 0; j < HEIGHT; j++) {
        (ATOM -> grid)[i][j] = *nullGrid;
}

the one way of deallocating would be

for (int i = 0; i < WIDTH; i++)
{
    free((ATOM -> grid)[i]);
}
free(ATOM->grid);
free(nullGrid);

In this case, you cannot safely free(ATOM->grid) before any free((ATOM -> grid)[i]) (unless you store all of the (ATOM->grid)[i] somewhere else, which sort of defeats the point). The individual (ATOM->grid)[i] may be freed in any order (as long as each is released exactly once).

Lastly, check the pointers returned by malloc(). It returns NULL when it fails, and dereferencing a NULL pointer gives undefined behaviour.

Upvotes: 3

Jabberwocky
Jabberwocky

Reputation: 50912

Yes, you need to free(nullGrid); here to avoid a memory leak.

But actually you can simplify your code to this:

void createGridArray(atom_t* ATOM) {
  ATOM -> grid = (grid_t**) malloc(WIDTH * sizeof(grid_t*));

  grid_t nullGrid = {NULL, 0};

  for (int i = 0; i < WIDTH; i++) {
    (ATOM -> grid)[i] = malloc(HEIGHT * sizeof(grid_t));
    for (int j = 0; j < HEIGHT; j++) {
        (ATOM -> grid)[i][j] = nullGrid;
    }
  }
}

There is need for mallocing nullGrid here at all. BTW the return at the end of a void function is implicit.

You wouldn't do this either:

void Foo()
{
   ...
   int *temp = malloc(sizeof(int));
   *temp = ...;
   ...
      bar = *temp;

   ...
   free(temp);
}

but rather:

void Foo()
{
   ...
   int temp
   temp = ...;
   ...
      bar = temp;

   ...
}

Upvotes: 0

Related Questions