c - 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.