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 removedAnonymous
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 removedAnonymous
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 removedAnonymous
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.htmlAnonymous
September 05, 2008
The comment has been removedAnonymous
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, PascalAnonymous
September 05, 2008
The comment has been removedAnonymous
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.