Memory leaking in C, malloc inside function

Question

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;
}

`atom_t`, `grid_t`.. Are we supposed to know what those are? Post a [mcve]. — StoryTeller - Unslander Monica, Apr 06 '17 at 10:28
[Please see this discussion on why not to cast the return value of `malloc()` and family in `C`.](http://stackoverflow.com/q/605845/2173917). — Sourav Ghosh, Apr 06 '17 at 10:28
@SouravGhosh How do you manage to religiously paste that link on every post? Do you have like a hotkey combination set to paste that link? — Ajay Brahmakshatriya, Apr 06 '17 at 10:41
@SouravGhosh Couldn't have guessed that. It would be worth while to just write a bot that scans the questions for (T*) malloc and pastes the link in the comments :p — Ajay Brahmakshatriya, Apr 06 '17 at 10:49
`ATOM -> grid` is _not_ a 2D array. It is a pointer to a pointer of `grid_t`. [example](https://cdecl.org/?q=int+**bar%3B). Code cannot create an _array_ by using `malloc()`. Code can allocate memory and get a pointer that is used like `ptr[i][j]`, but `ptr` is not an array. — chux - Reinstate Monica, Apr 06 '17 at 14:43

score 3 · Accepted Answer · answered Apr 06 '17 at 10:47

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.

Why not simply `(ATOM -> grid)[i] = calloc(HEIGHT, sizeof(*((ATOM->grid)[i])));`? Now the loop to set it to zero isn't needed anymore (all problems gone). — Paul Ogilvie, Apr 06 '17 at 10:51
The OP was using `malloc()` and initialising that way, not I. So I simply explained how to clean up. My tips are actually about preventing bugs. There are plenty of other improvements possible in how the OP set things up - using `calloc()` is only one - but the question was not about that. — Peter, Apr 06 '17 at 10:56
Thank you. Have removed casts, used calloc, and after a bit of fiddling have some working code — Esteban Mora, Apr 09 '17 at 05:51

score 0 · Answer 2 · answered Apr 06 '17 at 10:34

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;

   ...
}

score 0 · Answer 3 · answered Apr 06 '17 at 11:25

0

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().

answered Apr 06 '17 at 11:25

NoirDelire

59
5

Memory leaking in C, malloc inside function

3 Answers3