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[])
        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);
    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 (26)

  1. Rick Schaut says:

    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.

  2. James Skimming says:

    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.

  3. Rick Schaut says:

    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..

  4. James Skimming says:

    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.

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

  6. 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;

  7. <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);


       "There %s %d audio %s on the machinen",

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


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



  8. Curt Nichols says:

    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. 🙂

  9. 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.

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

  11. 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.

  12. james@skimming.net says:

    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.

  13. Igor Tandetnik says:

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

  14. Alex says:

    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.

  15. Dave says:

    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:


  16. Shog9 says:

    It’s been a while since i last used CComPtr<> (something about it pissed me off and drove me to _com_ptr_t<>) but couldn’t you just drop the Detach() and switch the return value to CComPtr<IMMDeviceEnumerator>?

    Adds an unnecessary set of addref/release calls i suppose, but avoids this sort of dodgy "does the call add a reference" guessing.

  17. 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);

  18. Nathan_works says:

    Too bad the generic answer of "you’re using COM" doesn’t count 😉

  19. Thales says:

    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.

  20. Pascal says:

    CComPtr<IMMDeviceEnumerator> deviceEnumerator;




  21. Looks like a memory leak.  The documentation for CComPtr::CoCreateInstance doesn’t explicitly say so, but it must increment the ref count.  Otherwise CComPtr::~CComPtr() would call Release on a zero count object.  

    That being the case the return of the function is a com pointer with ref count 1.  The CComPtr constructor will increment the ref count to 2 instead of one and eventually leak.  

    Fix is to

    1) CComPtr::Attach the result of GetDeviceEnumerator()

    2) Have GetDeviceEnumerator() return a CComPtr<> to make the ref count logic explicit (and don’t detach)

  22. Frederik Slijkerman says:

    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?

  23. Frederik Slijkerman says:

    Forget it, I now see how this is handled by CComPtrBase…

  24. piyo says:

    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.

  25. Wound says:

    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.

  26. martin says:

    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.