CODEWITHSUNDEEP
c
user2805460
user2805460

Reputation: 13

Program freezes when freeing memory

I have a program with a function to free memory of a struct. It usually works fine but sometimes it just freezes.

This is the struct which I am trying to free the memory for:

struct Image {
   unsigned int height;
   unsigned int width;
   unsigned int maxvalue;
   unsigned int *imgdata[];
};

This is the function which frees the memory( the printfs are only there to check where it is freezing)

void    Image_Delete (Image *img)
{
    printf("1");
    free(img->imgdata);
    printf("1");
    free(img);
    printf("1");
}

Sometimes this works fine but often the program freezes at free(img);. Are there any errors in my Image_Delete function?

Here are my malloc lines for img and img->imgdata

Image *img= (Image*)malloc(sizeof(Image*));
img->imgdata[height*width]= (unsigned int*)malloc(height*width*sizeof(unsigned int*));

Upvotes: 1

Views: 2460

Answers (1)

rici
rici

Reputation: 241901

Your first malloc doesn't allocate enough space:

Image *img= (Image*)malloc(sizeof(Image*));

That allocates enough space for a "pointer to Image" but you are about to use it as an "Image", which is 3 ints and a pointer. So when you start modifying your newly allocate "Image", you'll end up overwriting arbitrary memory. Also, your free(img->imgdata) call uses a pointer which is not contained in img's allocated space, and hence might have been modified by some other part of your program; calling free on a value which was not returned by malloc could result in arbitrary corruption of malloc's internal state, so pretty well anything is possible.

Here's a hint. Best style is to always write your mallocs like this:

Image *img = malloc(sizeof *img);

Using *variable rather than repeating the type is much less error prone, although you still end up repeating the variable name, so it's not perfect. And there's not point casting the void* which malloc returns.

Also, to answer the question you raised in a comment: it's always safe to call free on NULL, but otherwise you can only call it on a pointer which was returned by malloc. So if you're not going to fill in img->imgdata right away, you should clear it to 0. In fact, using calloc instead of malloc would be even better; in the case of small objects like Image, the overhead is negligible.

Upvotes: 2

Related Questions