Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What’s wrong with this code, Part 23..

What’s wrong with this code, Part 23..

Rate This
  • Comments 26

I recently tracked down a bug that was causing problems in my code.  Once I figured out the bug, I realized it made a good “what’s wrong with this code”…

#include "stdafx.h"
#include <mmdeviceapi.h>


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))
            {
                UINT deviceCount;
                hr = deviceCollection->GetCount(&deviceCount);
                if (SUCCEEDED(hr))
                {
                    printf("There are %d audio endpoints on the machine\n", deviceCount);
                }
            }
        }
    }
    CoUninitialize();
    return 0;
}

Simple code, right?  But there’s a nasty bug hidden in there, and it was NOT obvious to me what the bug was.

As always, kudos to the person who gets the bug first.  And of course mea culpa's for bugs I accidentally included.

  • It's not clear from the documentation, but I'm betting that CComPtr::CoCreateInstance doesn't modify its interface pointer member on failure.

    Always initialize CComPtr's with NULL.

  • Memory Leak on the IMMDeviceEnumerator object.

    The function GetDeviceEnumerator() will add a reference count to the object. Then the constructor of the of the smart pointer in _tmain(...) will add a second at the following line:

    CComPtr<IMMDeviceEnumerator> deviceEnumerator(GetDeviceEnumerator());

    The object will be forever at reference count 1.

  • On second thought, the default ctor for CComPtr initializes the member interface pointer (if not, then that's a bad design for CComPtr).

    Cue Emily Litella..

  • Memory Leak on the IMMDeviceEnumerator object.

    The function GetDeviceEnumerator() will add a reference count to the object. Then the constructor of the of the smart pointer in _tmain(...) will add a second at the following line:

    CComPtr<IMMDeviceEnumerator> deviceEnumerator(GetDeviceEnumerator());

    The object will be forever at reference count 1.

  • James got it (clearly this was too easy if the 2nd post had the answer :)).

  • Harder question... how to fix it?  I'd suggest making these changes:

    -IMMDeviceEnumerator *GetDeviceEnumerator()

    +HRESULT GetDeviceEnumerator(IMMDeviceEnumerator **ppDeviceEnumerator)

    {

    ...

       if (FAILED(hr))

       {

    -        return NULL;

    +        return hr;

       }

    -   return deviceEnumerator.Detach();

    +   *ppDeviceEnumerator = deviceEnumerator;

    +   deviceEnumerator->AddRef();

    +   return S_OK;

    }

    ...

       {

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

    +       CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    +       GetDeviceEnumerator(&deviceEnumerator);

           CComPtr<IMMDeviceCollection> deviceCollection;

  • <pedant>

    printf("There are %d audio endpoints on the machine\n", deviceCount);

    This can print "There are 1 audio endpoints on the machine", which looks silly.

    Possible fixes:

    printf("Audio endpoints on the machine: %d\n", deviceCount);

    printf(

       "There %s %d audio %s on the machine\n",

       (deviceCount == 1 ? "is" : "are"),

       deviceCount,

       (deviceCount == 1 ? "endpoint" : "endpoints"),

    );

    </pedant>

  • Maurits, I think you'd be better off just returning a smart pointer from GetDeviceEnumerator() and getting rid of the all-so-commonly-disastrous Detach() call. Mixing smart pointers with manual reference counting is a common way to introduce bugs. If you're going to use smart pointers, use them like they're actually smart pointers. :)

  • Curt: Thanks.  That was going to be one of my points in the response - the .Detach/.Attach thingy can be very tricky to get right.

  • Maurits: For your pedant comment: That is a localization nightmare :).

  • Perhaps a simpler fix (from the original post:)

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

    +        CComPtr<IMMDeviceEnumerator> deviceEnumerator;

    +        deviceEnumerator.Attach(GetDeviceEnumerator());

    CComPtr::Attach doesn't AddRef() the way initialization or assignment does.

  • Maurits: While your second solution will not leak, it relies on the knowledge of how the function GetDeviceEnumerator() is implemented, i.e. that it has allocated a reference count for the calling function.

    The reason I saw the problem so quickly is because I saw the function GetDeviceEnumerator() returning a raw pointer. It was immediately a case of "Danger Will Robinson!".

    The previously suggested solutions are far safer. My vote goes for your first solution. It's more efficient (though only marginally) and is constant with how most ATL APIs work.

  • Another pedantic comment: %d requires int, use %u for UINT.

  • References and smart pointers beat the crap out of pointer references.   If you're using C++, take advantage of it and stop trying to write C code.

  • It's funny but I haven't ever tried to write a function returning a raw COM interface. I guess it's just ingrained to pass it through an argument and return HRESULT. I've still managed to find lots of *other* ways to hose myself though.

    My favorite post about localization horror stories:

    http://interglacial.com/~sburke/tpj/as_html/tpj13.html

Page 1 of 2 (26 items) 12