1 Yozshuzshura

Ccomptr Release On Assignment Nursing

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 to a smart pointer instead of attaching it.

The 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 , and the responsibility is placed upon the recipient to call . The assignment operator for is a copy operation: It s 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 to a s the incoming pointer and s the old pointer (if any). When the is destructed, it will release the pointer, undoing the 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 is with the operator. This is a transfer operation:

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

Observe that there is no here. When the is destructed, it will perform the , which doesn't undo any operation performed by the . 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:

OperationBehaviorSemantics
Attach()Takes ownershipTransfer semantics
operator=()Creates a new referenceCopy semantics

You use the 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 method which is the opposite of : Detaching a pointer from the means "I am taking over responsibility for releasing this pointer." The 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 method is definitely one of the more dangerous methods in the 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 , 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 .

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 , causing the output integer to overwrite the input , resulting in the leak of the . 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 function itself?

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

Like most smart pointers, indicates ownership and uses RAII semantics to ensure that an owned resource is properly cleaned up. The methods and are used to transfer ownership, in part because the reference count bookkeeping can be relatively expensive. So the question you need to answer in your context is what the pattern of ownership is.

When transferring ownership between two objects, there will be a tendency to match and calls, but they will be on the separate objects:

Consider the following snippets and what they indicate about ownership of the object. Both and are intended to be values:

Here's my take on these snippets.

  • The first is a very usual way to return an to a caller, following the usual semantics wherein the caller receives a copy with an incremented reference count.
  • The second is okay if ownership is being passed to the caller; the caller takes the copy that the object previously had.
  • The third is probably returning a or already (the non-reference case can lead to extra reference count bookkeeping) and we are indicating further shared ownership; that is, we want to keep it around for longer than that statement.
  • The fourth indicates unusual semantics in , as it must have provided a reference count on a raw that we don't want to further increment.

Leave a Comment

(0 Comments)

Your email address will not be published. Required fields are marked *