What’s wrong with this code, Part 23 – The Answers

My last post was all about a problem with what appeared to be some really simple ATL code.

It turns out that the problem was easier than I had expected.  James Skimming came up with the answer on the second comment.  The problem here was that the following code (snipped a bit)

 IMMDeviceEnumerator *GetDeviceEnumerator()
{
    CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));
    if (FAILED(hr))
    {
        return NULL;
    }
    return deviceEnumerator.Detach();

}
int _tmain(int argc, _TCHAR* argv[])
{
    CoInitialize(NULL);
    {
        CComPtr<IMMDeviceEnumerator> deviceEnumerator(GetDeviceEnumerator());
        CComPtr<IMMDeviceCollection> deviceCollection;

        if (deviceEnumerator != NULL)
        {
            HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection);
            if (SUCCEEDED(hr))
            {

That’s because the CComPtr<T> constructor takes a reference to the input T* object.  As a result, the code leaks a reference to the MMDeviceEnumerator object.  The correct fix for the problem generated a fair amount of discussion in the comments and on email internally.

The root cause of the problem  is that the code in question mixes raw interface pointers and smart pointers.  In this case there’s an impedance mismatch between the GetDeviceEnumerator (which returns a raw pointer) and it’s caller (which is expecting a smart pointer).

My preferred solution to the problem is:

 CComPtr<IMMDeviceEnumerator> GetDeviceEnumerator()
{
    CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));
    if (FAILED(hr))
    {
        return NULL;
    }
    return deviceEnumerator;

}

The reason I prefer this solution is that it consistently uses the smart pointer class.  There are two downsides to it.  The first is that it loses the result of the call to CoCreateInstance.  The other downside is that it constructs a temporary object on the stack and has two additional calls to AddRef()/Release().

 

There are other possible solutions.  The second possible solution changes the GetDeviceEnumerator function:

 HRESULT GetDeviceEnumerator(IMMDeviceEnumerator *Enumerator)
{
    CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    if (Enumerator == NULL)
    {
        return E_POINTER;
    }
    HRESULT hr = deviceEnumerator.CoCreateInstance(__uuidof(MMDeviceEnumerator));
    if (FAILED(hr))
    {
        *Enumerator = NULL;
        return hr;
    }
    *Enumerator = deviceEnumerator.Detach();

    return S_OK;
}

With this change, the caller looks like:

     {
        CComPtr<IMMDeviceEnumerator> deviceEnumerator;
        if (SUCCEEDED(GetDeviceEnumerator(&deviceEnumerator))
        {
            CComPtr<IMMDeviceCollection> deviceCollection;
            HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection);
            if (SUCCEEDED(hr))

This solution fits closely with the COM usage pattern so it is quite attractive.  You also don’t lose the result of the CoCreateInstance API call, which can be extremely useful for diagnostics.  The major negative with this is that it depends on the fact that the CComPtr<T> operator& returns a raw pointer to the underlying object.

 

The third possible solution keeps the “return a value” idea of the original code but works around the additional reference applied in the constructor.  Keep the original implementation of GetDeviceEnumerator and change the tmain function as follows:

 int _tmain(int argc, _TCHAR* argv[])
{
    CoInitialize(NULL);
    {
        CComPtr<IMMDeviceEnumerator> deviceEnumerator;
        deviceEnumerator.Attach(GetDeviceEnumerator());
        CComPtr<IMMDeviceCollection> deviceCollection;

The big problem with this solution is that to me it’s “unnatural” – the whole point of using smart pointers is that their use is supposed to be intuitive, however in this case there’s nothing intuitive about the use – it certainly fixes the problem but it relies on internal behaviors of the smart pointer class.  To me this is the least attractive solution to the problem.

 

As I mentioned, Kudos to James Skimming for figuring the problem out quickly.