A COM-mon memory leak

27 12 2009

One of the memory leaks I find most often in our code base comes from people being insufficiently lazy when constructing empty BSTRs. That and not understanding/thinking about the low level implementations of using ATL’s wrapper classes.

When using CComBSTRs there are two obvious ways to create empty strings:

CComBSTR empty = L"";
CComBSTR null;

Since there should be no semantic difference between a zero length BSTR and a NULL BSTR I always prefer the second option for two reasons:

  1. It’s 4 less characters to type (six if you use a sane number of spaces)
  2. The first option allocates a new BSTR which includes a count DWORD and a null terminator, not only is this allocation redundant, it may also lead to memory fragmentation.

These reasons can be seen as nitpicking but the real fun starts when CComBSTR’s operator & is used in order to pass the CComBSTR as an out parameter. What operator& does is simply return a raw pointer to the inner BSTR field, this means that when we enter a function that accepts a BSTR* no knowledge of ownership follows the pointer. If this function specified that the BSTR* parameter is an out parameter it will overwrite it without freeing its current value (since the current value may be garbage). If the CComBSTR contained a valid BSTR (e.g. an empty one) this BSTR will be leaked!

A Google code search shows that 5 out of 22 places in which a CComBSTR was initialized with the empty string (L””) go on to use it as an out parameter thus leaking it, most of the other places go on to either assign another value or use Append (which is safe to use on NULL BSTRs so again the empty BSTR is redundant).

ATL’s authors knew this could be a problem and added an assert in operator& that fires if the member BSTR isn’t NULL, however this assert is protected by a macro (ATL_NO_CCOMBSTR_ADDRESS_OF_ASSERT) which isn’t set by default. Why this differs from CComPtr’s operator & (which always asserts) is beyond me.

The root of the problem is that all ATL’s wrapper classes are leaky abstractions, they’re great for saving you some typing here and there and for scoping resource ownership (using RAII) but they don’t save you from thinking about what’s going on behind the scenes. Forget that and the only thing you’ll gain from CComBSTR, CComPtr and friends will be the ability to add bugs faster than before.




One response

5 05 2010

yeah, it’s quite often that I find myself looking into ATL sources in order to understand what is really happening inside, especially with safearrays/variants.

I would also add the following to your collection of bugs:
suppose we have a function with the following prototype:
CComBSTR doSomething();

Then the following code compiles, but…
BSTR bstr = doSomething();

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: