1

Another C question:

let's say I have a struct that has a pointer member of char* type.

When I want to initialize an instance of the struct I call malloc:

MyStruct* ptr = (MyStruct*)malloc(sizeof(MyStruct)

And then allocate 256 bytes of memory for the char* member:

ptr->mem = (char*)malloc(sizeof(char)*256);

what happens to the pointer member and the memory it points to when I call free(ptr);? when I check the program with valgrind I see that I have a memory leak, but when I explicitly call free(ptr->member); I still have a memory leak and valgrind shows an "Invalid free" error

What's the proper way the manage the memory pointed by the member?

Oliver Charlesworth
  • 267,707
  • 33
  • 569
  • 680
Bob
  • 101
  • 1
  • 3
  • 8
  • 1
    You shouldn't cast the return value from `malloc` in C. – Oliver Charlesworth Apr 09 '12 at 21:07
  • 1
    @stefanbachert: See http://stackoverflow.com/a/605858/129570. – Oliver Charlesworth Apr 09 '12 at 21:10
  • @stefanbachert: it is a point of contention. It isn't strictly necessary, and if you aren't compiling with options to warn you about missing function declarations, could lead to errors because the compiler assumed `malloc()` returns `int` and it doesn't. OTOH, in C++, the cast is necessary. I often put the cast in; I understand why people leave it out, and don't get het up about it either way. – Jonathan Leffler Apr 09 '12 at 21:12
  • The problem, in part, I think, is that lots of people use the cast because they believe they _have_ to cast. They register it is used in some code and include it without really knowing why. Further I think that in many cases if one need to cast to clarify, then there is something wrong with the structure of the code and or naming of variables etc. Further down the line I also notice some people casting malloc also cast to suppress errors and the cast becomes some kin of "magic wand" - only problem being it is black magic ;) – Morpfh Apr 09 '12 at 21:40

3 Answers3

3

You have to free ptr->member first, then the struct

free(ptr->member);
free(ptr);
stefan bachert
  • 9,413
  • 4
  • 33
  • 40
3

As soon as you call free(ptr), none of the members in ptr are valid any more. You can't do anything with them. But the memory that was pointed to be ptr->mem still needs to be freed. So you must either free(ptr->mem) first, or have otherwise copied that pointer somewhere so have a valid pointer to free.

The general pattern of allocating and freeing compound structures is something like (and it is helpful to wrap them up in nice clean functions that do this):

MyStruct* MakeMyStruct() {
    MyStruct* ptr = malloc(sizeof(MyStruct)); //N.B. don't need cast if it's C
    ptr->mem = malloc(sizeof(char)*256);
    //initialise other members
    return ptr;
}

void DestroyMyStruct(MyStruct *ptr) {
    //Free members first, then the struct
    free(ptr->mem);
    free(ptr);
}

If some of the members are complicated structs themselves, they would in turn be allocated/freed with MakeWhatever and DestroyWhatever instead of malloc and free in the above two functions.

Edmund
  • 10,533
  • 3
  • 39
  • 57
  • +1 for nice clean functions. It could be argued that adding `ptr->mem = NULL;` to `DestroyMyStruct` would help catch bugs elsewhere. – Oliver Charlesworth Apr 09 '12 at 21:19
  • That's what I implemented, but I get a double free error for some reason.. The valgrind output shows that the double free is from the method that destroys the struct.. – Bob Apr 10 '12 at 11:54
  • Could be that `ptr->mem` equals `ptr` somehow. Do you want to update the question with your new code? – Edmund Apr 10 '12 at 12:50
2

The rule of thumb is that you need one free for every (successful) call to malloc (and generally, these occur in the reverse order).

If you only free(ptr), then you have a memory leak (because there's no way to access the memory allocated for ptr->mem). If you only free(ptr->mem), then you haven't cleared up completely (not quite as bad as a memory leak).

Oliver Charlesworth
  • 267,707
  • 33
  • 569
  • 680