Error in using free with 2D arrays in C

Question

I am using C for this project to create a bunch of dynamically created arrays. They are generated as explained [here][1]This works fine. ' However, when I try the code below to free up the array(s), I get a "segmentation error( Core Duped)". I am using the listing below to create a "my_struct".

typedef struct
{
    uint32_t**  block;         
    uint32_t**  valid;
    uint8_t     block_size;     //Bytes per block

    uint8_t     level;
}my_struct;

my_struct L1, L2;

Thereafter, at a later point, the pointers "block" and "valid" are allocated dynamic memory using the function below where they are successively passed as parameters (arr_ptr):

void Generate2DArray (uint32_t** arr_ptr, uint32_t row, uint32_t column)
{
uint32_t* temp;
uint32_t i = 0;
uint32_t j = 0;

arr_ptr = (uint32_t**)malloc(row* sizeof(uint32_t*));
if(arr_ptr == NULL)
{
    printf("MALLOC 1 FAILS \n ");
}
temp = (uint32_t*)malloc(row* column* sizeof(uint32_t));
if(temp == NULL)
{
    printf("MALLOC 2 FAILS \n ");
}
for (i = 0; i < row; i++)
{
  arr_ptr[i] = temp + (i * column);
}
}

All this works fine so far.

Now, when I try to "free" the memory near the end of the code, using the listing below, I get an error message saying "Segmentation Fault (Core dumped)"

void FreeMemory(uint32_t** arr_ptr, uint32_t rows)
{
    uint32_t i = 0;

    for ( i = 0; i < rows; i++)
    {
        free(arr_ptr[i]);
    }   
    free(arr_ptr);
}

Please provide any suggestions as to where am I going wrong. I have gone through this post as well and my code seems to be compliant with it.

Thanks!!

I might have said it before, but you're new so maybe you haven't heard: [please don't cast the return value of `malloc()` in C](http://stackoverflow.com/a/605858/28169). — unwind, Oct 04 '13 at 14:20
Suggestion. Don't leak memory. In Generate2DArray. none of the allocated memory in that function makes it out because you're value-assigning the array parameter pointer rather than passing a pointer by-address. Alternatively, you can forego the array parameter *entirely* and simply using the function's *return value* to bring the allocation back to the caller. As is, it leaks like a sieve. — WhozCraig, Oct 04 '13 at 14:23
Why would someone still want to implement a 2D array as an array of pointers in 2013 is completely beyond me. If STL/Boost solutions do not appeal to you, at least allocate it as one block and save on indirect access/continuity/paging and what not. I would label this as an "antipattern" if there was such an option. — AlexK, Oct 04 '13 at 14:45
Please don't call and treat double pointers as if they were 2D arrays, they aren't. If you need a 2D array, allocate a 2D array: `int (*arr)[ROWS] = malloc(COLS * sizeof *arr);` — , Oct 04 '13 at 14:45
@AlexK For one, because neither STL nor Boost are available in C. — WhozCraig, Oct 04 '13 at 15:19
@WhozCraig, So true, not that there aren't any reusable open source C-only solutions. — AlexK, Oct 04 '13 at 17:29

score 2 · Answer 1 · answered Oct 04 '13 at 15:09

Hope this helps.:)

       arr_ptr: row*sizeof(uint32_t*)
          ||
          ||
        __\/__       0   1   2   ...   column-1   column
  0    | temp ==>  |   |   |   |     |          |       |: column*sizeof(uint32_t)
  1    | temp             
  2    | temp                
  .    |  ..                 
  .    |  ..                 
row-1  | temp                
 row   | temp            
        ------

P0W · Accepted Answer · 2013-10-04T14:37:06.277

1

Fix you Generate2DArray, you aren't achieving what you're thinking.

void Generate2DArray  (uint32_t*** arr_ptr, uint32_t row, uint32_t column)
{
    int **array= malloc (row * sizeof (uint32_t*));

    for (int i = 0; i < row; i++)
        array[i] = malloc(column * sizeof(uint32_t));

    *arr_ptr = array;

}

Use it using :

int **arr_ptr;
Generate2DArray(&arr_ptr, rows, cols);

You can use your way of single allocation of row buffers, but the main point was the ***, the address of the final arr_ptr. Also, I think this is bit clearer.

edited Oct 04 '13 at 14:37

answered Oct 04 '13 at 14:31

P0W

46,614
9
72
119

The OP is purposely using a single allocation for the row buffers, then partitioning that through the loop. You should do the same in this answer, then help the guy out by fixing his free-array (which works with this, not with his, even if his was done right, which it isn't). +1 btw. – WhozCraig Oct 04 '13 at 14:33
@POW :Could you also please explain why is it so? – mes mart Oct 04 '13 at 14:38
And how would this help in resolving the issue with "free" ? – mes mart Oct 04 '13 at 14:44
@mesmart your free routine is correct, beside you were using it for freeing memory that wan't allocated. And for the above `***` is only necessary cause you need to change the value of the pointer, within the function, you need to pass the pointer by its address. Also in your free `***` is only necessary if you intend to change the value of the pointer, else not – P0W Oct 04 '13 at 14:50

Werner Henze · Answer 3 · 2013-10-04T14:45:56.960

You must call free() for each successfull call to malloc(). You are doing two mallocs, so you need to call free two times. Once for arr_ptr and once for temp (which is equivalent to arr_ptr[0]).

If you want to keep your implementation of FreeMemory you must change Generate2DArray so that you call malloc for each row and store the returned pointer in arr_ptr[i]. You should decide if you think it's better to have one larger block of memory or a lot of smaller blocks of memory and then choose the corresponding implementation.

Also: as WhozCraig said you are not returning the allocated buffer to the caller of Generate2DArray. You should change the function signature to one of these:

// Return pointer
uint32_t** Generate2DArray (uint32_t row, uint32_t column);
// Pass pointer to the pointer variable and store it there
void Generate2DArray (uint32_t*** arr_ptr, uint32_t row, uint32_t column);

Error in using free with 2D arrays in C

3 Answers3