Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

Rate This
  • Comments 21

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.

  • @Maurits, but then it would rather fail in it's purpose. i.e. to be a bit of example code demonstrating the problem.

  • @Jared

    "It's easy to criticize but it's a lot more effective if you have specific problems to point out."

    I did just that in the rest of my comment.

    Digging through my links, here is another longish discussion of problems with CComPtr and other similar classes:

    http://www.gershnik.com/articles/refcnt_ptr.asp

    It also gives a better smart pointer class that would have prevented Larry's problem by forcing him to stop and think what exactly he wants to accomplish when assigning from raw pointer.

  • @Marvin,

    I don't feel like you did.  You made one comment about ref_counted_smart_ptr.  I'm not sure if you were refering to CComPtr<> or not but comment lacked a good example.  Mind you I'm not saying CComPtr<> is perfect (I've made several posts to the contrary).  

    I enjoyed the article wanting to add a parameter to the constructor of the CComPtr class indicating whether or not an add ref was requested.  I highly dislike the use of a bool though.  Take the following

    CComPtr<IFoo> spFoo(p,true)

    To an un-educated user what does true mean?  If you say they'll look at the documentation that's a bad answer.  Because if looking at the documentation was an answer then there should be no problem having a single argument constructor that will AddRef (after all if everyone read the documentation, there would be no ambiguity)

    On the other hand if you made in an enum it becomes much clearer.

    enum SmartPointerInit { SmartPointerInit_AddRef; SmartPointerInit_DontAddRef}

    ...

    CComPtr<IFoo> spFoo(p,SmartPointerInit_AddRef)

  • @Myself

    Accidentally navigated to submit trying to dismiss an annoying dialog.

    The author of the article didn't believe that a non-loud name could be found for an enum.  The problem he's trying to solve is a subtle issue.  Making the issue loud removes the subtle problem :)

  • @Jared

    I don't think you've read the article through since it makes exact same argument about enum vs. bool.

    But at the end it doesn't want to use a second parameter at all, be it bool or enum. The syntax it forces is something like

    p_smart = ref(p_raw);

    p_smart = noref(p_raw);

    which to me looks much better than two parameter constructor. Especially b/c you don't need to repeat the pointer type.

  • "Kudos to James Skimming"

    Don't you spell that QDOS?

Page 2 of 2 (21 items) 12