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

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.

Comments

• Anonymous
  September 05, 2008
  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.

• Anonymous
  September 05, 2008
  The comment has been removed

• Anonymous
  September 05, 2008
  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..

• Anonymous
  September 05, 2008
  The comment has been removed

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

• Anonymous
  September 05, 2008
  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;

• Anonymous
  September 05, 2008
  <pedant> printf("There are %d audio endpoints on the machinen", deviceCount); This can print "There are 1 audio endpoints on the machine", which looks silly. Possible fixes: printf("Audio endpoints on the machine: %dn", deviceCount); printf(    "There %s %d audio %s on the machinen",    (deviceCount == 1 ? "is" : "are"),    deviceCount,    (deviceCount == 1 ? "endpoint" : "endpoints"), ); </pedant>

• Anonymous
  September 05, 2008
  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. :)

• Anonymous
  September 05, 2008
  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.

• Anonymous
  September 05, 2008
  Maurits: For your pedant comment: That is a localization nightmare :).

• Anonymous
  September 05, 2008
  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.

• Anonymous
  September 05, 2008
  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.

• Anonymous
  September 05, 2008
  Another pedantic comment: %d requires int, use %u for UINT.

• Anonymous
  September 05, 2008
  The comment has been removed

• Anonymous
  September 05, 2008
  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

• Anonymous
  September 05, 2008
  The comment has been removed

• Anonymous
  September 05, 2008
  While the digression on localization (or is that localisation? compromize on l10n) is interesting, in my own defense (or is that defence?) I must point out that the first alternative I proposed is eminently l6able: printf("Audio endpoints on the machine: %dn", deviceCount);

• Anonymous
  September 05, 2008
  Too bad the generic answer of "you're using COM" doesn't count ;)

• Anonymous
  September 05, 2008
  Usually with this kind of idiom you want to return a smart pointer rather than a raw pointer to remove the ambiguity and save the user from having to inspect the function implementation - they just have to look at the declaration to understand what happens. Also, you can still avoid unecessary addref/releases with:    deviceEnumerator.Attach( GetDeviceEnumerator().Detach() ); I don't think that Attach/Detach is tricky, on the contrary, we just have to write code that follows some elementary basic rules.

• Anonymous
  September 05, 2008
  CComPtr<IMMDeviceEnumerator> deviceEnumerator; deviceEnumerator.Attach(GetDeviceEnumerator()); Cheers, Pascal

• Anonymous
  September 05, 2008
  The comment has been removed

• Anonymous
  September 07, 2008
  How does this work? HRESULT hr = deviceEnumerator->EnumAudioEndpoints(eAll, DEVICE_STATE_ACTIVE, &deviceCollection); &deviceCollection is a pointer to a CComPtr object, not a pointer to IMMDeviceCollection, right? So this shouldn't even compile. Or am I mistaken?

• Anonymous
  September 07, 2008
  Forget it, I now see how this is handled by CComPtrBase...

• Anonymous
  September 07, 2008
  So the rule is, if given an initialized COM pointer "from somewhere else" and you wanna stick it in a ATL::CComPtr, use ATL::CComPtr::Attach(T *p)  instead of the ctor ATL::CComPtr::CComPtr(T *p). But then how do you know after ATL::CComPtr::~CComPtr() is called that you are Releasing the right amount of times? Is 1 time enough? Do you have to give it back to the someone you got it from? I think that the return type of GetDeviceEnumerator should be changed to the same suggestion as the others previously posted, that is, CComPtr<IMMDeviceEnumerator>. Contemporary C++ recommends the smart pointer concept to automate resource control and ensure exception safety. Except this forces people to use ATL, which is probably a trivial constraint, but a constraint nonetheless.

• Anonymous
  September 07, 2008
  I would always implement this as HRESULT GetDeviceEnumerator(IMMDeviceEnumerator **ppDeviceEnumerator) because that is the way COM implements this kind of thing. Look at QueryInterface, get__NewEnum and the countless other COM methods that return interface pointers. In fact, the new property wizard will always magic up a "get" property of this format.

• Anonymous
  September 08, 2008
  A bug like this is a good example why it's better to just stick with a nice "HRESULT Func(ISomething** ppSomething)" function where normal COM "conventions" will mean there's never a doubt in a long time COM user's mind that a reference has already been added and they can't accidentally add another one (that won't get released) with a simple assignment to a CCom(QI)Ptr<T> variable.