When macros obfuscate in a way that encourages bugs....

by Michael S. Kaplan, published on 2006/03/02 20:01 -05:00, original URI: http://blogs.msdn.com/b/michkap/archive/2006/03/02/542489.aspx

This is not a specifically international issue but it is one that annoys me, which makes me feel like blogging about it....

The intent of the macros is rather obvious -- avoid the minutae of how things are allocated in every call.

The problem for me is in this FREE_MEMORY macro, which is doing a couple of things that annoy me. First of all, it is checking for a NULL pointer to know whether it should free, without even so much as an ASSERT to try to track down these people who are trying to free something that does not have to be freed.

But more importantly, it is not ZEROING OUT THE BUFFER it just freed (the very thing it checks for!), which means that any type of double free bug is not found immediately, since if they attempt to free a block twice then it will just try and free it again.

Now I have seen plenty of code that does this, and which adds in an extra FREE_MEMORY type call at the end of a function to "make sure resources are released."

Ok, I will get off my soapbox now, back to your regularly scheduled blog.... :-)

Also both of those macros use their argument twice. Any time a macro uses an argument twice, you run into DO_SOMETHING(*p++) issues where p is incremented twice.

If a macro can't do what it needs to do without referencing the argument only once, it's safer to use a function.

/* let's define some constants */
#define NUM_BLOCKS 20
#define BLOCK_SIZE 1024

/* a nice static array here */
void* lotsa_memory[NUM_BLOCKS];

/* let's grab 20K of memory in bite-sized chunks */
for (int i = 0; i < NUM_BLOCKS; i++)
{
__ lotsa_memory[i] = ALLOC_MEMORY(BLOCK_SIZE);
}

/* let's do some stuff... */
/* ... */
/* ... */
/* ok, done. Now how to free the memory? */

/* this would work */
for (int i = 0; i < NUM_BLOCKS; i++)
{
__ FREE_MEMORY(lotsa_memory[i]);
}

/* and so would this */
int i = 0;
do { FREE_MEMORY(lotsa_memory[i]); } while (i++ < NUM_BLOCKS);

/* but this is a big hairy BUG!! */
int i = 0;
do { FREE_MEMORY(lotsa_memory[i++]); } while (i < NUM_BLOCKS);

While I do agree that there might be reasons to complain about FREE_MEMORY I think your complaint is ill founded. FREE_MEMORY's semantics with respect to null pointers are in line with the std library facillities in both C99 and C++03. free() in C as well as delete in C++ are both defined to be a no-op for null pointers. Given this I think FREE_MEMORY's behaviour is more than reasonable.

Furthermore AFAICS zeroing out the pointer would be exactly the wrong thing to do with respect to hiding bugs. It would in fact hide double free, what is by the way diagnosed by the underlying HeapFree call in MS's C library (the memory debugging facillities are pretty powerfull).

In my experience there is a widespread misconception on what the non nullness of a pointer conveys. There is practicly no information about the memory it points to as far as allocation state goes. It could be on the heap or on the stack and if on the heap it could be free'd already. Given this coding standards that require zeroing out pointers to heap memory that has been free'd add a false sense of safety. And are likely to hide issues that can be easily diagnosed by modern C-libraries.

Hi Thomas,

All reasonable, though the part I had the most trouble with was combining that FREE_MEMORY with the idea of "cleaning up at the end" something that may have already been cleaned up. I am pretty sure that no one is in favor of architecting a double free....