Casting COM Interfaces

Question

I came across a problem in my code today where an access violation was being caused, AFAICT, by casting a COM object of mine to an IUnknown**. The function it was passed into executed without a problem but when calling one of my object's functions it would execute some random function and corrupt the stack then die.

Indicative code (just ignore why it's done this way - I know it's bad and I know how to fix it but this is a question of why problems like this may occur):

void MyClass2::func(IMyInterface* pMyObj)
{
    CComPtr<IMyInterface2> pMyObj2;
    HRESULT hRes = pMyObj->GetInternalObject((IUnknown**)&pMyObj2);

    if (SUCCEEDED(hRes))
        pMyObj2->Function(); // corrupt stack
}

void MyClass::GetInternalObject(IUnknown** lpUnknown)
{
    pInternalObject->QueryInterface(IID_IMyInterface2, (void**)lpUnknown);
}

I have always been a bit suspicious of using C/C++ casts on COM objects but I've never encountered (possibly through undefined behaviour) any problems until now.

I had a quick look and from what I can tell casting to IUnknown is technically valid so long as there is no multiple interitance in the inheritance chain, however it is not considered best practice - I should really pass an IUnknown to MyClass::GetInternalObject(IUnknown** lpUnknown) and then query the return value for the interface I want.

My question is, are there rules as to when C/C++ casts can be used on COM objects, and aside from multiple inheritance and the adjustor thunks they bring, how can casting COM objects result in surprises like access violations? Please be detailed.

Edit: They're all good examples of how it should be done properly but what I was hoping for was a technical explanation of why you shouldn't cast COM objects (assuming one exists) e.g. casting will return pMyObj2-4 in situation x but QueryInterface will return pMyObj2-8 because of y...or is casting COM objects simply a matter of bad practice/style?

TIA

Answer 1

I'd just use CComPtr and CComQIPtr to manage COM interfaces, instead of writing code with C-style casts that to me seem inappropriate in the context of COM:

void MyClass2::Func(IMyInterface* pMyObj)
{
    // Assuming:
    //   HRESULT IMyInterface::GetInternalObject( /* [out] */ IUnknown** )
    CComPtr<IUnknown> spUnk;       
    HRESULT hr = pMyObj->GetInternalObject(&spUnk);
    if (SUCCEEDED(hr))
    {
        // Get IMyInterface2 via proper QueryInterface() call.
        CComQIPtr<IMyInterface2> spMyObj2( spUnk );
        if ( spMyObj2 )
        {
            // QueryInterface() succeeded

            spMyObj2->Function();
        }
    }
}

Moreover, I'm not a COM expert, but I see with suspicion your code:

void MyClass::GetInternalObject(IUnknown** lpUnknown)
{
    pInternalObject->QueryInterface(IID_IMyInterface2, (void**)lpUnknown);
}

If you are QueryInterface()'ing IID_MyInterface2, you should store that in an IMyInterface2*, not in an IUnknown*. If your method returns an IUnknown*, then I'd QueryInterface() an IID_IUnknown:

// NOTE on naming convention: your "lpUnknown" is confusing.
// Since it's a double indirection pointer, you may want to use "ppUnknown".
//
void MyClass::GetInternalObject(IUnknown** ppUnknown)
{
    pInternalObject->QueryInterface(IID_IUnknown, (void**)ppUnknown);
}

or better use IID_PPV_ARGS macro:

void MyClass::GetInternalObject(IUnknown** ppUnknown)
{
    IUnknown* pUnk = NULL;
    HRESULT hr = pInternalObject->QueryInterface(IID_PPV_ARGS(&pUnk));
    // Check hr...

    // Write output parameter
    *ppUnknown = pUnk;
}

COM style casts have a specific name: QueryInterface().

Answer 2

I think the issue is that because a cast from IMyInterface* to IUnknown* is OK (in COM everything inherits from IUknown right?) you think that a cast from IMyInterface** to IUnknown** is also OK. But that's not true in C++, and I doubt it's true in COM either.

To me the following looks more logical, apologies if this isn't strictly correct, my COM is very rusty, but hopefully you get the idea.

CComPtr<IUnknown> pMyObj2;
HRESULT hRes = pMyObj->GetInternalObject(&pMyObj2);

if (SUCCEEDED(hRes))
{
    CComPtr<IMyInterface> pMyObj3 = (IMyInterface*)pMyObj2;
    pMyObj3->Function();
}

I.e. get an IUnknown object first, and then down cast that to your actual type.

Answer 3

I don't see any issues in your code snippets, the stack corruption perhaps has its cause but its somewhere else.

I don't think it is your actual code because GetInternalObject should be of HRESULT type and yours is not, so you lost something during copy/pasting.

To stay safer, just avoid direct QueryInterface calls because together with casting they might misinterpret interfaces. Casting to and from IUnknown* might be inevitable though. If the callee cannot be trusted to return proper interface casted to IUnknown, on the caller side you might prefer to QI once again to make sure you hold the interface of your interest.

Provided that GetInternalObject is a COM interface method on its own, you could have it like this:

void MyClass2::func(IMyInterface* pMyObj)
{
    CComPtr<IUnknown> pMyObj2Unknown;
    pMyObj->GetInternalObject((IUnknown**)&pMyObj2Unknown);
    CComQIPtr<IMyInterface2> pMyObj2 = pMyObj2Unknown; // This is only needed if callee is not trusted to return you a correct pointer
    if (pMyObj2)
        pMyObj2->Function(); // corrupt stack
}

STDMETHODIMP MyClass::GetInternalObject(IUnknown** lpUnknown) // COM method is typically both HRESULT and __stdcall
{
    CComQIPtr<IMyInterface2> pMyInterface2 = pInternalObject;
    if(!pMyInterface2)
        return E_NOINTERFACE;
    *lpUnknown = pMyInterface2.Detach(); // *lpUnknown will have to me IMyInterface2 this way
    return S_OK;
}

PS If GetInternalObject was a native method, not COM, you would avoid casting to IUnknown* at all.