How to free memory allocated on double pointer struct

Question

How can I free memory allocated in this struct

struct image_t {
    char type[3];
    int **ptr;
    int width;
    int height;
};

In the first function I made these allocations:

    struct image_t *struktura = (struct image_t *)malloc(sizeof(struct image_t));
    int **ptr = (int**)malloc(struktura->height * sizeof(int*));

    for (i = 0; i < struktura->height; i++) {
        *(ptr + i) = (int *)malloc(struktura->width * sizeof(int));
        if (*(ptr + i) == NULL) {
            break;
        }
    }

In the second function I must free allocated memory so I tried to free memory something like this but it does not work

void destroy_image(struct image_t **m) {
    if (m != NULL) {
        if (*m != NULL) {
            if ((*m)->ptr != NULL) {
                for (int i = 0; i < (*m)->height; i++) {
                    free(*((*m)->ptr + i));
                }
                free((*m)->ptr);
                free(*m);
            }
        }
    }
}

I can't change declaration of the destroy function so there must be double pointer on struct.

In order to create a `**` of anything (to simulate a 2D array) it is a two-step process (1) allocate a block of `rows` number of pointers, (2) loop `rows` times allocating a block holding `cols` number of whatever type you need assigning the address to each of your pointers in turn. To free all memory, simply reverse the process, e.g. (2) loop `rows` number of times freeing each block assigned to a each pointer and then (1) make a single-call to free the pointers. (you can substitute `height` and `width` for `rows` and `cols`) — David C. Rankin, May 18 '21 at 06:26
In C, there is no need to cast the return of `malloc`, it is unnecessary. See: [Do I cast the result of malloc?](http://stackoverflow.com/q/605845/995714) — David C. Rankin, May 18 '21 at 06:27
Advice: If your `ptr` is a pixeldata array, I'd suggest to make it one contiguous array of data and not a two dimensional array. — Raildex, May 18 '21 at 06:33

Support Ukraine · Answer 1 · 2021-05-18T07:54:02.440

To start with...

Don't cast the value returned by malloc
Don't use *(ptr + i) Use the equivalent but much more readable version, i.e. ptr[i]

Doing that changes your allocation to:

struct image_t *struktura = malloc(sizeof(struct image_t));
int **ptr = malloc(struktura->height * sizeof(int*));

for (i = 0; i < struktura->height; i++) {
    ptr[i] = malloc(struktura->width * sizeof(int));
    if (ptr[i] == NULL) {
        break;
    }
}

Here is the first problem... you never assign ptr to anything. In the end of the code block you need to add:

struktura->ptr = ptr;

Another problem is that both struktura->height and struktura->width are uninitialized when used. They must be assigned a value before use.

For freeing the allocated memory: Your current code is too complex. A statement like free(*((*m)->ptr + i)); contains 3 pointer dereferences!!. That's hard to read. I'll recommend that you use a few local variables to simplify the code.

void destroy_image(struct image_t **m) {
    if (m == NULL) return;
    if (*m == NULL) return;
    struct image_t *t = *m;
    int **ptr = t->ptr;
    if (ptr != NULL)
    {
        for (int i = 0; i < t->height; i++) 
        {
            free(ptr[i]);
        }
        free(ptr);
    }
    free(t);
    *m = NULL;  // The only reason to pass a double pointer to this
                // function is to be able to change *m. I guess
                // the prototype authoe wants the function to set
                // *m to NULL
}

By using these local variables the code is much easier to read than stuff like free(*((*m)->ptr + i));

And for the break in the allocation code...

It's kind of strange (a bug?) that you you don't check for NULL after the first two malloc and then do it inside the loop. Further it's kind of strange to use break as it will leave the rest of the pointers uninitialized.

Anyway - If you really want that break in the allocation code, you also need to take that into account when freeing memory. Something like:

void destroy_image(struct image_t **m) {
    if (m == NULL) return;
    if (*m == NULL) return;
    struct image_t *t = *m;
    int **ptr = t->ptr;
    if (ptr != NULL)
    {
        for (int i = 0; i < t->height && ptr[i] != NULL; i++) 
        {
            free(ptr[i]);
        }
        free(ptr);
    }
    free(t);
    *m = NULL;
}

chqrlie · Accepted Answer · 2021-05-18T07:07:22.957

2

In order for your destroy function to work properly, all pointers in the pointer array must be either valid or null. Memory returned by malloc() is uninitialized so breaking out of the loop in the allocation function leaves the rest of the pointer array uninitialized, hence should not be passed to free().

Also note the you should test for allocation failure of the structure pointer and the pointer array. Furthermore the width and height members of the newly allocated structure are unintialized: you should use function arguments to initialize them.

The destroy_image function should probably set *m to NULL after deallocation and must free(*m); even if (*m)->ptr is a null pointer.

Here are solutions for this issue:

allocate the array with calloc() (a good idea in all cases) or
set height to the number of successfully allocated pointers.
explicitly set the remaining pointers in the array to NULL
free the allocated block upon allocation failure and return NULL.

Here is a modified version:

#include <stdlib.h>

struct image_t {
    char type[3];
    int **ptr;
    int width;
    int height;
};

struct image_t *allocate_image(int width, int height) {
    struct image_t *struktura = calloc(1, sizeof(*struktura));

    if (struktura == NULL)
        return NULL;

    // should initialize struktura->type too
    struktura->width = width;
    struktura->height = height
    struktura->ptr = calloc(height, sizeof(*struktura->ptr));
    if (struktura->ptr == NULL) {
        free(struktura);
        return NULL;
    }

    for (int i = 0; i < height; i++) {
        struktura->ptr[i] = calloc(sizeof(*struktura->ptr[i]), width);
        if (struktura->ptr[i] == NULL) {
            // Iterate downwards on index values of allocated rows
            // (i --> 0) is parsed as (i-- > 0)
            // this test works on signed and unsigned index types, unlike (--i >= 0)
            while (i --> 0) {
                free(struktura->ptr[i]);
            }
            free(struktura->ptr);
            free(struktura);
            return NULL;
        }
    }
    return struktura;
}

void destroy_image(struct image_t **m) {
    if (m != NULL) {
        struct image_t *p = *m;
        
        if (p != NULL) {
            if (p->ptr != NULL) {
                for (int i = 0; i < p->height; i++) {
                    free(p->ptr[i]);
                }
                free(p->ptr);
            }
            free(p);
            *m = NULL;
        }
    }
}

edited May 18 '21 at 07:07

answered May 18 '21 at 06:44

chqrlie

131,814
10
121
189

I don't really agree with these overly pedantic clean-ups upon malloc failure. If you are out of heap memory your program is likely toast and the only sensible thing to do is to stop it as gracefully as possible. In which case manual heap clean-up isn't important. – Lundin May 18 '21 at 06:48
@DavidC.Rankin: `*m` is freed before `*m = NULL;`. The very purpose of `*m = NULL;` is to clear the dangling pointer in the caller's space. Not a perfect solution as this pointer could have been duplicated elsewhere, but a reasonable one. – chqrlie May 18 '21 at 06:59
@Lundin: if the structure cannot be fully allocated, clean up and return `NULL`. If some cases, using a wrapper that aborts upon failure is fine, but in other cases it is a life threatening approach. – chqrlie May 18 '21 at 07:03
@chqrlie - yep, it was the use of `p` and the comparison with the original that prompted the question. Thanks. – David C. Rankin May 18 '21 at 07:03
@TruthSeeker: thanks for the edit, but your test fix was incorrect and I happen to like the elegant [*downto* operator](https://stackoverflow.com/questions/1642028/what-is-the-operator-in-c-c). – chqrlie May 18 '21 at 07:10
But it's pointless to clean-up if you were about to exit anyway. The only reason for calling free() upon exit is to catch run-time bugs related to the heap and pointers. But you only need to do that when the program terminates without errors. – Lundin May 18 '21 at 07:32
@Lundin: I agree that cleaning up is vain if the program exits abruptly on error. In my example, the allocation function returns `NULL` on failure. The caller may exit or try a smaller matrix size or free some memory or ask to user... The allocation function should just try to allocate and fail gracefully. What would you say if `fopen()` leaked memory when opening the file fails? – chqrlie May 18 '21 at 07:36
fopen is another story. If you are running out of heap memory then you should usually just exit. If the malloc failure comes from a user asking for unreasonable amounts, then that should have been caught by sanitizing user input before calling the function. I guess it is somewhat application-specific. – Lundin May 18 '21 at 07:59
@Lundin: *I guess it is somewhat application-specific.* yes indeed we agree on this. The caller knows better. – chqrlie May 18 '21 at 08:21

Lundin · Answer 3 · 2021-05-18T06:53:43.873

1

This is needlessly complicated and inefficient. Have a look at Correctly allocating multi-dimensional arrays.

You can change the struct like this:

struct image_t {
    char type[3];
    int* ptr; // placeholder pointer
    int width;
    int height;
};

Then malloc like this:

struct image_t* s = malloc(sizeof *s);
s->width  = width;
s->height = height;
s->ptr    = malloc( sizeof(int[width][height]) );

(Remember to check the result of each malloc and stop the program if it returns NULL.)

Then use it like this:

int (*arr)[s->height] = (void*) s->ptr; // convert to a temporary 2D array pointer
...
arr[i][j] = whatever; // this enables 2D array access syntax

And free it like this:

free(s->ptr);
free(s);

edited May 18 '21 at 06:53

answered May 18 '21 at 06:42

Lundin

195,001
40
254
396

Alternatively you could use old style "mangled arrays" `s->ptr = malloc(width*height*sizeof(int));` ... `(s->ptr)[i*s->width + j]` but that's less readable. – Lundin May 18 '21 at 06:45
Using a real 2D array is good advice, but 2D arrays with both dimensions variable is a C11 optional feature and cannot be specified as a structure member. Furthermore I doubt the OP can change the structure definition nor the allocation strategy: allocating a 2D array and initializing the pointer array to point to each of its rows seems more efficient but would fail if some other code reallocates individual rows. – chqrlie May 18 '21 at 06:49
@chqrlie No that doesn't make sense either. In that case they would need to store the individual sizes somewhere, not use two members called height and width. The structure you describe sounds like an array of structs, each struct storing data and size. – Lundin May 18 '21 at 06:52
@DavidC.Rankin I don't understand what you mean with "technical bounds violation". – Lundin May 18 '21 at 06:53
@Lundin: by *reallocating*, I meant changing the row pointers. Even if no call to `free` is performed, just swapping the first and second pointers would make the 2D array no longer freeable as its base address is no longer known. – chqrlie May 18 '21 at 06:54
@DavidC.Rankin The object that `p` points at has no effective type until you write access it. The type of the object then becomes the type used for the access. Which is actually just an `int`. So the compiler cannot make any out of bounds assumptions about `p[i][j]` for the same reason as it can't make any such assumptions from `int* p = malloc(n*sizeof *p); ... p[i] = ...`. – Lundin May 18 '21 at 06:58
@Lundin -- thank you. Coffee wore off a long time ago and I fell into my own sleepy trap -- there is no array involved... – David C. Rankin May 18 '21 at 07:01

How to free memory allocated on double pointer struct

3 Answers3