Is this a good way to free memory?

Question

The function for freeing an instance of struct Foo is given below:

void DestroyFoo(Foo* foo) {     if (foo) free(foo); } 

A colleague of mine suggested the following instead:

void DestroyFoo(Foo** foo) {     if (!(*foo)) return;     Foo *tmpFoo = *foo;     *foo = NULL; // prevents future concurrency problems     memset(tmpFoo, 0, sizeof(Foo));  // problems show up immediately if referred to free memory     free(tmpFoo); } 

I see that setting the pointer to NULL after freeing is better, but I'm not sure about the following:

1. Do we really need to assign the pointer to a temporary one? Does it help in terms of concurrency and shared memory?

2. Is it really a good idea to set the whole block to 0 to force the program to crash or at least to output results with significant discrepancy?

Answer 1

Do we really need to assign the pointer to a temporary one? Does it help in terms of concurrency and shared memory?

It has nothing to do concurrency or shared memory. It's pointless.

Is it really a good idea to set the whole block to 0 to force the program to crash or at least to output results with significant discrepancy?

No. Not at all.

The solution suggested by your colleague is terrible. Here's why:

• Setting whole block to 0 achieves nothing either. Because someone is using a free()'ed block accidentally, they wouldn't know that based on the values at the block. That's the kind of block calloc() returns. So it's impossible to know whether it's freshly allocated memory (calloc() or malloc()+memset()) or the one that's been free()'ed by your code earlier. If anything it's extra work for your program to zero out every block of memory that's being free()'ed.

• free(NULL); is well-defined and is a no-op, so the if condition in if(ptr) {free(ptr);} achieves nothing.

• Since free(NULL); is no-op, setting the pointer to NULL would actually hide that bug, because if some function is actually calling free() on an already free()'ed pointer, then they wouldn't know that.

• most user functions would have a NULL check at the start and may not consider passing NULL to it as error condition:

void do_some_work(void *ptr) {     if (!ptr) {         return;      }     /*Do something with ptr here */ } 

So the all those extra checks and zero'ing out gives a fake sense of "robustness" while it didn't really improve anything. It just replaced one problem with another the additional cost of performance and code bloat.

So just calling free(ptr); without any wrapper function is both simple and robust (most malloc() implementations would crash immediately on double free, which is a good thing).

There's no easy way around "accidentally" calling free() twice or more. It's the programmer's responsibility to keep track of all memory allocated and free() it appropriately. If someone find this hard to handle then C is probably not the right language for them.

Answer 2

What your collegue suggests will make the code "safer" in case the function is called twice (see sleske comment...as "safer" may not mean the same for everybody...;-).

With your code, this will most likely crash:

Foo* foo = malloc( sizeof(Foo) ); DestroyFoo(foo); DestroyFoo(foo); // will call free on memory already freed 

With your collegues's version of the code, this will not crash:

Foo* foo = malloc( sizeof(Foo) ); DestroyFoo(&foo); DestroyFoo(&foo); // will have no effect 

Now, for this specific scenario, doing tmpFoo = 0; (within DestroyFoo) is enough. memset(tmpFoo, 0, sizeof(Foo)); will prevent crash if Foo has extra attributes that could be wrongly accessed after memory is released.

So I would say yes, it may be a good practice to do so....but it's only a kind of security against developers who have bad practices (because there's definitely no reason to call DestroyFoo twice without reallocating it)...in the end, you make DestroyFoo "safer" but slower (it does more stuff to prevent bad usage of it).