How can i correct this memory leak?

Question

This is my code to allocate memory using malloc for a 2 dimensional array in C, and to free it. N is a number given by the user

R = malloc(N * sizeof(int *)); // start of memory allocating for **R
if (R == NULL) { 
  return -1;
}

for (i = 0; i <= N; i++) {
  *(R+i) = malloc(N * sizeof(int));
  if (*(R + i) == NULL) {
    return -1;
  }
} // end of memory allocating for **R 

for (i = 0 ; i < N; i++){
  free(R[i]);
  free(R);
}

After the program finishes, I get this error: double free or corruption (out) Aborted (core dumped)

By using valgrind for debugging, I get this message: 12 bytes in 1 blocks are definitely lost in loss record 1 of 1

This leak is detected at this line of code (according to valgrind): *(C+i) = malloc(N * sizeof(int));

Any help on how to fix it?

This code is incomplete. You allocate `R`, then you free `C` ... — Damien, Jan 04 '21 at 16:14
Please post a [mcve], and perhaps indent it properly while you are at it. — n. m. could be an AI, Jan 04 '21 at 16:14

bruno · Answer 1 · 2021-01-04T17:10:04.843

2

i get this error: double free or corruption (out) Aborted (core dumped)

first notice that error is not a memory leak, the double free comes from that code

for (i=0 ; i <N ; i++){
  free(R[i]);
  free(R);
}

which must be must be

for (i=0 ; i <=N ; i++){
  free(R[i]);
}
free(R);

else you free R in the first loop rather than after the loops

Because of that from the second loop the value of R[i] if undefined and the behavior of the free is undefined too, and you call again free on R, both reasons compatible with the message produced by valgrind.

Note I also changed the test to be i<=N to be the identical of the test used to do the allocations, but both suppose your array is sized N+1 (at least), else you miss a free producing a memory leak.

If the allocations step and free step are not consecutive in the code and the free step is done even you cannot allocate the memory at a given time, the code must be modified to be :

if (R != NULL)
{
  for (i=0 ; i <=N ; i++){
    if (R[i] == NULL) /* allocation failed */
      break;
    free(R[i]);
  }
  free(R);
}

to not dereference R if it is NULL, and to not attempts to free the entries of the array not set in the allocation loop because (*(R + i) == NULL) was true breaking the loop

edited Jan 04 '21 at 17:10

answered Jan 04 '21 at 16:15

bruno

32,421
7
25
37

Your assumption is correct ,however that seems to not be the solution to my subproblem – David Brawl Jan 04 '21 at 16:23
@DavidBrawl why ? did you see the remark about the size ? – bruno Jan 04 '21 at 16:24
There's one other issue, although it won't show up unless the allocation stage partially failed: either an early return should itself free `R` and all the allocated `R[0]..R[i]`, or the early return branch should make sure the subsequent `R[i+1]..R[N]` are set to NULL. – Useless Jan 04 '21 at 16:24
@Useless yes, but in the given code there is a *return* so none free are done at all – bruno Jan 04 '21 at 16:26
I was assuming that was a snippet from a different function - it's hard to be sure without a proper [minimal reproducible example](https://stackoverflow.com/help/minimal-reproducible-example) – Useless Jan 04 '21 at 16:27
@Useless yes, and I edited my answer to follow your advice – bruno Jan 04 '21 at 16:30
It's OK to `free(NULL)`, right? The issue is that the uninitialized `R[i]` (or `C[i]` or whatever) entries may _not_ be `NULL`. – Useless Jan 04 '21 at 16:32
@Useless yes it is ok, but the next elements are not initialized, so it is not possible to call *free* on them, and because it is useless to call *free* on NULL I check the NULL case before to call *free* – bruno Jan 04 '21 at 16:33
@Useless it is like you do not see the *break* in my code, I do not do `if (C[i] != NULL) free(C[i]);` or something like that – bruno Jan 04 '21 at 16:38
@DavidBrawl please can you answer to my remark about the size of the array (also in my answer) ? – bruno Jan 04 '21 at 16:46
@bruno Sorry to the late response,it seems that my problem steemed from me,not calculating correctly the lines of the 2d array – David Brawl Jan 04 '21 at 16:58
1

Ah you mean because the single failed allocation stops the `free` loop. Yes, you're perfectly right - I was fixated on the partially-successful allocation itself. – Useless Jan 04 '21 at 17:00

Basile Starynkevitch · Answer 2 · 2021-01-04T17:19:19.920

This is my code to allocate memory using malloc for a 2 dimensional array in C, and to free it

An alternative approach is to make your matrix some abstract data type implemented by some (pointer to) struct ending with a flexible array member. In such a case, your matrix creation routine makes one allocation (using malloc), and your matrix deallacation routine can be a simple (casted) free. Don't forget to handle failure of malloc(3) (it could happen when you want a gigantic matrix which don't fit in your computer).

Details have been given in this answer.

Read also of course Modern C and some C reference website.

IMHO two-dimensional arrays don't exist in C, you can just have arrays of arrays of some known type. Check by reading some C standard, like n2176.

How can i correct this memory leak?

2 Answers2