On the value of consistent API design

Raymond Chen writes in “We’re using a smart pointer, so we can’t possibly be the source of the leak“.  The most immediate cause is a subtle misuse of CComPtr, using operator= which performs an AddRef on a return value that has already been AddRef’d, leading to one AddRef too many.

The less immediate failure was a poorly designed API.

Well, not poorly designed.  Unfortunately designed.

The ATL library class template CComPtr is a smart pointer designed to automate lifetime management of COM interface pointers.  IStream is a COM interface.  The general pattern in COM is to only return interface pointers in an ‘out parameter’ position.  CComPtr is designed for this, and when its operator& is used to capture an out parameter value, it does not perform the errant extra AddRef.

 

CComPtr<IStream>  s;
CreateSomething(&s, ...); // Callee performs AddRef, and CComPtr
                          //  will Release.  Balanced.

But this code sample in question is using SHCreateMemStream, and SHCreateMemStream does something a little different. Rather, it returns an interface pointer in C’s return value position. This is not unusual for shell functions or Win32 in general, but it is inconsistent with COM.

 

CComPtr<IStream> s;
s = SHCreateMemStream(...); // oops, too many AddRefs

In part 2, Raymond Chen comments
“The CComPtr::operator=(T*) method is definitely one of the more dangerous methods in the CComPtr repertoire, because it’s so easy to assign a pointer to a smart pointer without giving it a moment’s thought.” It might be also fair to point out that “The SHCreateMemStream(..) function is definitely one of the more dangerous COM functions to use, because it’s so easy to leak it without giving it a moment’s thought”.

See, CComPtr::operator= is following the COM rules — it was given an in parameter, which it AddRef’d and later Released, a balanced operation.

If you don’t believe me, consider this equally naive code, which experiences the same leak without invoking ATL:

 

HRESULT GetXmlReaderFromStream(IStream*, IXmlReader**);
...
CComPtr$lt;IXmlReader> pReader;
UINT nDepth = 0;

//Open read-only input stream
IFC(GetXmlReaderFromStream(
    ::SHCreateMemStream(utf8Xml, cbUtf8Xml),
    &pReader);

pReader is used properly in an output position, so it’s clear to the caller that it will be assigned a value that the caller must Release — this is the COM convention. However, SHCreateMemStream is able to appear in an input position. Leak of memory stream ensues.

Lastly, a fix — just treat SHCreateMemStream’s return value as though it were in an output position:

 

#define SHCreateMemStreamFix(pInit,cbInit,out)
    ((out) = ::SHCreateMemStreamFix(pInit, cbInit))

SHCreateMemStreamFix(some, args, &pStream);

Problem solved.