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

You may have seen C/C++ code that used something like this before:

#define ALLOC_MEMORY(dwBytes)               \
    ( HeapAlloc( GetProcessHeap(),          \
                 HEAP_ZERO_MEMORY,          \
                 dwBytes ) )

#define FREE_MEMORY(hMem)                   \
    ( (hMem) ? (HeapFree( GetProcessHeap(), \
                          0,                \
                          (PVOID)hMem ))    \
             : 0 )

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.

Doesn't this kind of "simplification" just obfuscate bugs?

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

And it just seems like a lazy and a potentially really bad practice to me. Sure, add the simplifying macro if you want -- but make sure the macro guards against side effects, and don't simplify it so much that you actually hide bugs.

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

# Maurits [MSFT] on 2 Mar 2006 8:16 PM:

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.

# Maurits [MSFT] on 2 Mar 2006 8:17 PM:

Someday I'll learn how to count. ALLOC_MEMORY is fine in that respect, it just uses dwBytes once.

# Maurits [MSFT] on 2 Mar 2006 8:31 PM:

/* 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);

# Maurits [MSFT] on 2 Mar 2006 8:35 PM:

*ahem* actually the next-to-last-block should be...

/* and so would this */
int i = 0;
do { FREE_MEMORY(lotsa_memory[i]); } while (++i < NUM_BLOCKS);
/* note pre-increment, not post-increment... stupid off-by-one errors */

# Thomas Witt on 3 Mar 2006 12:08 AM:

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.

# Michael S. Kaplan on 3 Mar 2006 1:43 AM:

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

go to newer or older post, or back to index or month or day