The difference between assignment and attachment with ATL smart pointers


Last time, I presented a puzzle regarding a memory leak. Here's the relevant code fragment:

CComPtr<IStream> pMemoryStream;
CComPtr<IXmlReader> pReader;
UINT nDepth = 0;

//Open read-only input stream
pMemoryStream = ::SHCreateMemStream(utf8Xml, cbUtf8Xml);

The problem here is assigning the return value of SHCreateMemStream to a smart pointer instead of attaching it.

The SHCreateMemStream function creates a memory stream and returns a pointer to it. That pointer has a reference count of one, in accordance with COM rules that a function which produces a reference calls AddRef, and the responsibility is placed upon the recipient to call Release. The assignment operator for CComPtr<T> is a copy operation: It AddRefs the pointer and saves it. You're still on the hook for the reference count of the original pointer.

ATLINLINE ATLAPI_(IUnknown*) AtlComPtrAssign(IUnknown** pp, IUnknown* lp)
{
        if (lp != NULL)
                lp->AddRef();
        if (*pp)
                (*pp)->Release();
        *pp = lp;
        return lp;
}

template <class T>
class CComPtr
{
public:
        ...

        T* operator=(T* lp)
        {
                return (T*)AtlComPtrAssign((IUnknown**)&p, lp);
        }

Observe that assigning a T* to a CComPtr<T> AddRefs the incoming pointer and Releases the old pointer (if any). When the CComPtr<T> is destructed, it will release the pointer, undoing the AddRef that was performed by the assignment operator. In other words, assignment followed by destruction has a net effect of zero on the pointer you assigned. The operation behaves like a copy.

Another way of putting a pointer into a CComPtr<T> is with the Attach operator. This is a transfer operation:

        void Attach(T* p2)
        {
                if (p)
                        p->Release();
                p = p2;
        }

Observe that there is no AddRef here. When the CComPtr<T> is destructed, it will perform the Release, which doesn't undo any operation performed by the Attach. Instead, it releases the reference count held by the original pointer you attached.

Let's put this in a table, since people seem to like tables:

Operation Behavior Semantics
Attach() Takes ownership Transfer semantics
operator=() Creates a new reference Copy semantics

You use the Attach method when you want to assume responsibility for releasing the pointer (ownership transfer). You use the assignment operator when you want the original pointer to continue to be responsible for its own release (no ownership transfer).

There is also a Detach method which is the opposite of Attach: Detaching a pointer from the CComPtr<T> means "I am taking over responsibility for releasing this pointer." The CComPtr<T> gives you its pointer and then forgets about it; you're now on your own.

The memory leak in the code fragment above occurs because the assignment operator has copy semantics, but we wanted transfer semantics, since we want the smart pointer to take the responsibility for releasing the pointer when it is destructed.

pMemoryStream.Attach(::SHCreateMemStream(utf8Xml, cbUtf8Xml));

The CComPtr<T>::operator=(T*) method is definitely one of the more dangerous methods in the CComPtr<T> repertoire, because it's so easy to assign a pointer to a smart pointer without giving it a moment's thought. (Another dangerous method is the T** CComPtr<T>::operator&(), but at least that has an assertion to try to catch the bad usages. Even nastier is the secret QI'ing assignment operator.) I have to say that there is merit to Ben Hutchings' recommendation simply not to allow a simple pointer to be assigned to a smart pointer, precisely because the semantics are easily misunderstood. (The boost library, for example, follows Ben's recommendation.)

Here's another exercise based on what you've learned:

Application Verifier told us that we have a memory leak, and we traced it back to the function GetTextAsInteger.

BSTR GetInnerText(IXMLDOMNode *node)
{
    BSTR bstrText = NULL;
    node->get_text(&bstrText);
    return bstrText;
}

DWORD GetTextAsInteger(IXMLDOMNode *node)
{
    DWORD value = 0;

    CComVariant innerText = GetInnerText(node);
    hr = VariantChangeType(&innerText, &innerText, 0, VT_UI4);
    if (SUCCEEDED(hr))
    {
        value = V_UI4(&innerText);
    }

    return value;
}

Obviously, the problem is that we passed the same input and output pointers to VariantChangeType, causing the output integer to overwrite the input BSTR, resulting in the leak of the BSTR. But when we fixed the function, we still got the leak:

DWORD GetTextAsInteger(IXMLDOMNode *node)
{
    DWORD value = 0;

    CComVariant innerText = GetInnerText(node);
    CComVariant textAsValue;
    hr = VariantChangeType(&innerText, &textAsValue, 0, VT_UI4);
    if (SUCCEEDED(hr))
    {
        value = V_UI4(&textAsValue);
    }

    return value;
}

Is there a leak in the VariantChangeType function itself?

Hint: It is in fact explicitly documented that the output parameter to VariantChangeType can be equal to the input parameter, which results in an in-place conversion. There was nothing wrong with the original call to VariantChangeType.

Comments (15)
  1. John says:

    The answer to this is right there in the docs (http://msdn.microsoft.com/en-us/library/5b5cad6s%28VS.80%29.aspx):

    CComVariant& operator =(LPCOLESTR lpszSrc);

    lpszSrc [in] The character string to be assigned to the CComVariant object. You can pass a zero-terminated wide (Unicode) character string to the LPCOLESTR version of the operator or an ANSI string to the LPCSTR version. In either case, the string is converted to a Unicode BSTR allocated using SysAllocString. The type of the CComVariant object will be VT_BSTR.

  2. Sam Holloway says:

    This is one of the reasons why we prefer to use Microsoft’s _com_ptr_t instead – the semantics of the operator= are more ‘natural’ and it’s more difficult for the novice to get it wrong.

  3. Exact same problem the hint to yesterday’s puzzle (http://blogs.msdn.com/oldnewthing/archive/2008/06/23/8640472.aspx): the BSTR decays into an LPCOLESTR, so CComVariant::operator= makes a copy of the BSTR and the original BSTR is leaked.

  4. Hal says:

    One of my favorite uses for the new move semantics in C++0x is getting to write:

    spFoo1(std::move(spFoo2));

    instead of

    spFoo1.Attach(spFoo2.Detach());

  5. Cooney says:

    One of my favorite uses for the new move semantics in C++0x is getting to write:

    this looks interesting – got a link?

  6. Kyle says:

    > One of my favorite uses for the new move semantics in C++0x is getting to write:

    this looks interesting – got a link?

    See http://en.wikipedia.org/wiki/C%2B%2B0x#Rvalue_reference_and_move_semantics for details…it’s kind of a mind twister, tho.

  7. Ivo says:

    How do you look for memory leaks using Application Verifier? I have a simple program that leaks:

    CoInitialize(NULL);

    void *ptr=CoTaskMemAlloc(100);

    CoUninitialize();

    but the Verfier doesn’t say anything.

  8. Yuhong Bao says:

    ”Even nastier is the secret QI’ing assignment operator.“

    Yea, it is like late-binding in Visual Basic, only that VB makes errors more obvious.

  9. Marvin says:

    Your are wrong about Boost. It allows conversion from the raw pointer. You still can convert but just have to type the pointer class name again. Take a look at

    http://www.boost.org/doc/libs/1_40_0/libs/smart_ptr/intrusive_ptr.html

    This is Boost’s smart pointer that would work with COM. It doesn’t address the real issue that there is no single way to "convert" to begin with. For a COM smart pointer there should not be any direct conversion from the raw one.

    If you were talking about shared_ptr then this is a different beast that has very different semantics from the COM case. The whole issue of whether to attach or copy never arises there since count is external to the object. In other words the pointer always has the reference and "Attach" is impossible to begin with.

  10. Neil says:

    AtlComPtrAssign could crash if releasing the old value causes the destruction of the CComPtr object.

  11. pm100 says:

    thank heavens for CLR / c# / .net

    I dont have to worry about this stuff anymore, or at least almost :-)

  12. JohnQPublic says:

    <i>

    thank heavens for CLR / c# / .net

    I dont have to worry about this stuff anymore, or at least almost :-)

    </i>

    Now it’s buried deeply enough that when something goes wrong, you’ve not got the tools to find it :(

  13. B.Y. says:

    This is the problem with features like smart pointers. You have to spent so much time to figure out tips and tricks and who knows what, you’re much better off if you just keep things simple and be diligent and use regular pointers. Well, I guess if people did that memory leaks wouldn’t be a big problem to begin with.

Comments are closed.

Skip to main content