What is the correct way to cast when using ATL and IUnknownPtr?

Question

During the modification of an existing ATL COM object I came across an article from the "The Old New Thing" blog called "The ways people mess up IUnknown::QueryInterface" and there was a discussion in the comments section that started when one of the respondents (Norman Diamond) pointed out that that in one of the article's examples that the cast to void** was incorrect.

However when I try and correct my code to do the casting properly I end up with a memory leak.

The example was as follows:

IShellFolder *psf = some object;
IUnknown *punk = NULL;
psf->QueryInterface(IID_IUnknown, (void**)&punk);

Norman said

punk is not a void*. punk is an IUnknown*.

void** is not a universal pointer type. void* is a universal pointer type, and char* and relatives are grandparented in to be equivalent in that way, but void** is not.

If you want to obey the calling convention and avoid horrible deaths, you have to do this: IUnknown *punk; void *punkvoid; psf->QueryInterface(IID_IUnknown, &punkvoid); punk = (IUnknown *)punkvoid;

Lots of other MSDN contributors made the same identical mistake.... some people might say that it works in all VC++ implementations to date, but that doesn't make it correct code, and it's still violating the calling convention.

In light of this I went to change my old code - which was as follows:

#include <comdef.h>

...

HRESULT FinalConstruct()
{ 
    if (m_dwROTCookie != 0)
        return E_FAIL;

    //Check whether there already is an instance of the Object
    IUnknownPtr pUnk = NULL;
    if (GetActiveObject(CLSID_Object, NULL, &pUnk) == S_OK)
    {
        TRACE_WARNING("An instance of Object already exists in the current context");
        return S_OK;
    }
    HRESULT hr = QueryInterface(IID_IUnknown, reinterpret_cast<void **>(&pUnk));

    hr = RegisterActiveObject(pUnk, CLSID_Object, ACTIVEOBJECT_WEAK, m_dwROTCookie);        
    if (FAILED(hr))
        return hr;

    hr = CoLockObjectExternal(pUnk, TRUE, TRUE);
    pUnk = NULL;
    ATLASSERT(m_dwRef == 2);
    return hr;
}

I then changed it as follows:

HRESULT FinalConstruct()
{ 
    if (m_dwROTCookie != 0)
        return E_FAIL;

    //Check whether there already is an instance of the Object
    IUnknownPtr pUnk = NULL;
    if (GetActiveObject(CLSID_Object, NULL, &pUnk) == S_OK)
    {
        TRACE_WARNING("An instance of Object already exists in the current context");
        return S_OK;
    }
    void* pUnkVoid = NULL;
    HRESULT hr = QueryInterface(IID_IUnknown, &pUnkVoid);

    if (SUCCEEDED(hr)
    {
        pUnk = reinterpret_cast<IUnknown*>(pUnkVoid);
        hr = RegisterActiveObject(pUnk, CLSID_Object, ACTIVEOBJECT_WEAK, m_dwROTCookie);        
        if (FAILED(hr))
            return hr;

        hr = CoLockObjectExternal(pUnk, TRUE, TRUE);
        pUnk = NULL;
    }
    ATLASSERT(m_dwRef == 2);

    return hr;

However now my application has a memory leak from this the COM Object

Answer 1

Mmm, I think that rather than assigning the void* to pUnk I should be using:

pUnk.Attach(reinterpret_cast<IUnknown*>(pUnkVoid));

Answer 2

You likely have a memory leak because you call GetActiveObject() and QueryInterface() which upon success increment the reference count on the object, but don't call Release() later to decrement the reference count.