Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

"Memory Leak" when using the Vista Audio API notification routines

"Memory Leak" when using the Vista Audio API notification routines

  • Comments 17

We recently got an internal report from someone using the internal audio notification APIs that they were leaking memory and they wanted to help from us debugging the problem.

I took a look and discovered that the problem was a circular reference that was created when they called:

CFoo::Initialize()
{
    <SNIP>

    hr = CoCreateInstance(__uuidof(MMDeviceEnumerator), NULL, CLSCTX_INPROC_SERVER, __uuidof(IMMDeviceEnumerator), (void**)&m_pEnumerator);
    if (FAILED(hr))
        return hr;

    if (FAILED(m_pEnumerator->RegisterEndpointNotificationCallback(this)));
    <SNIP>
}

CFoo::~CFoo()
{
    <SNIP>
    m_pEnumerator->UnregisterEndpointNotificationCallback(this);
    <SNIP>
}

The root cause of the problem is that the IMMDeviceEnumerator::RegisterEndpointNotificationCallback takes a reference to the IMMNotificationClient object passed in.  This shouldn't be a surprise, because the Counted Pointer design pattern requires that every time you save a pointer to an object, you take a reference to that object (and every interface that derives from IUnknown implements the Counted Pointer design pattern).  Since the RegisterEndpointNotificationCallback saves it's input pointer for later consumption (when it generates the notification), it has to take a reference to the object.

At the heart of the problem is the fact that CFoo object only calls UnregisterEndpointNotificationCallback in its destructor (which will never be called).  If the CFoo object had a "Shutdown()" or other form of finalizer, the call to UnregisterEndpointNotificationCallback could be moved to the finalizer, thus removing the circular reference and avoiding the memory leak.  This is by far the best solution - I'm a huge fan of deterministic finalism.

 

Unfortunately, sometimes it's not possible to have a "Shutdown()" method (for instance, if you're implementing an interface that doesn't implement the finalizer design pattern (in fact, this was the case for the person who reported the problem to us).

In that case, you really want to depend on the fact that the reference count reflects your external references, not your internal references.  Effectively, you want to maintain two separate reference counts, one for external clients, the other for internal usage.

One way to achieve this is to use a delegator object - instead of handing "this" to the RegisterEndpointNotificationCallback, you pass a small object that implements IMMNotificationClient.  So:

class CFooDelegator: public IMMNotificationClient
{
    ULONG   m_cRef;
    CFoo *m_pFoo;

    ~CFoo()
    {
    }
public:
    CFooDelegator(CFoo *pFoo) :
        m_cRef(1),
        m_pFoo(pFoo)
    {
    }
    STDMETHODIMP OnDeviceStateChanged(LPCWSTR pwstrDeviceId, DWORD dwNewState) 
    {
        if (m_pFoo)
        {
            m_pFoo->OnDeviceStateChanged(pwstrDeviceId, dwNewState);
        }
        return S_OK;
    } 
    STDMETHODIMP OnDeviceAdded(LPCWSTR pwstrDeviceId) 
    {
        if (m_pFoo)
        {
            m_pFoo->OnDeviceAdded(pwstrDeviceId);
        }
        return S_OK;
    } 
    STDMETHODIMP OnDeviceRemoved(LPCWSTR pwstrDeviceId)
    {
        if (m_pFoo)
        {
            m_pFoo->OnDeviceRemoved(pwstrDeviceId);
        }
        return S_OK;
    } 
    STDMETHODIMP OnDefaultDeviceChanged(EDataFlow flow, ERole role, LPCWSTR pwstrDefaultDeviceId)
    {
        if (m_pFoo)
        {
            m_pFoo->OnDeviceAdded(flow, role, pwstrDefaultDeviceId);
        }
        return S_OK;
    } 
    STDMETHODIMP OnPropertyValueChanged(LPCWSTR pwstrDeviceId, const PROPERTYKEY key)
    {
        if (m_pFoo)
        {
            m_pFoo->OnPropertyValueChanged(pwstrDeviceId, key);
        }
        return S_OK;
    }

    void OnPFooFinalRelease()
    {
        m_pFoo = NULL;
    }

    STDMETHOD(QueryInterface) (REFIID riid,
                               LPVOID FAR* ppvObj)
    {
        *ppvObj = NULL;
        if (riid == IID_IUnknown)
        {
            *ppvObj = static_cast<IUnknown *>(this);
        }
        else if (riid == IID_IMMNotificationClient)
        {
            *ppvObj = static_cast<IMMNotificationClient *>(this);
        }
        else
        {
            return E_NOINTERFACE;
        }
        return S_OK;
    }
    STDMETHOD_(ULONG,AddRef)()
    {
        return InterlockedIncrement((LONG *)&m_cRef);
    }
    STDMETHOD_(ULONG,Release) ()
    {
        ULONG lRet = InterlockedDecrement((LONG *)&m_cRef);
        if (lRet == 0)
        {
            delete this;
        }
        return lRet;
    }
};

You then have to change the CFoo::Initialize to construct a CFooDelegator object before calling RegisterEndpointNotification().

You also need to change the destructor on the CFoo:

CFoo::~CFoo()
{
    <SNIP>
    m_pEnumerator->UnregisterEndpointNotificationCallback(m_pDelegator);
    m_pDelegator->OnPFooFinalRelease();
    m_pDelegator->Release();
    <SNIP>
}

It's important to call UnregisterEndpointNotificationCallback before you call OnPFooFinalRelease - if you don't, there's a possibility that the client's final release of the CFoo might occur while a notification function is being called - if that happens, the destructor might complete and you end up calling back into a partially destructed object.  And that's bad :).  The good news is that the UnregisterEndpointNotificationCallback function guarantees that all notification routines have completed before it returns.

It's important to realize that this issue occurs with ALL the audio notification callback mechanisms:IAudioEndpointVolume::RegisterControlChangeNotify, IPart::RegisterControlChangeCallback, and IAudioSessionControl::RegisterAudioSessionNotification.

  • PingBack from http://www.soundpages.net/computers/?p=4530

  • Sorry this is a tangent, but it shows yet again yet again yet again how important it seems to be in Microsoft for programmers to learn how to shut up a compiler warning which would have been accurate, instead of making their code correct.  Why not teach programmers how to program correctly instead of doing ship-it like this:

    (void**)&m_pEnumerator

    Sure, in the compiler someone used yesterday, it doesn't crash.  But tomorrow's compiler might be better.  Henry Spencer wrote something like "If you lie to the compiler, it will get revenge."

  • Hmmm... That's too much work for a simple circular reference.

    Couldn't one just look at own ref count, and un-register when it drops to 1? Unregistering will put refcount to 0 and cause destruction of the instance. (Hmmm... This also means ::Release must be re-entrant, but it usually is, no?)

    Not really advisable for anything but simplest cases, though.

  • wouldn't it be easier to put:

    m_pEnumerator->UnregisterEndpointNotificationCallback(m_pDelegator);

    m_pDelegator->OnPFooFinalRelease();

    in the IUnknown::release of the delegator like:

    ULONG lRet = InterlockedDecrement((LONG *)&m_cRef);

           if (lRet == 0)

           {

                m_pEnumerator->UnregisterEndpointNotificationCallback(m_pDelegator);

               m_pDelegator->OnPFooFinalRelease();

               delete this;

           }

           return lRet;

  • BG: Of course - they're functionally equivilant.  ATL replaces your code with a call to the "FinalRelease" method.

    Goran: That is a ...  bad idea - it relies on the fact that you know that there will be one and only one reference applied by the notification logic.  That can be quite tricky.  In addition, it's very hard to get it totally right in a multithreaded environment.

    Norman: You're right - I had a friend once who hated any warning level greater than 2 because it forced him to add casts that could cause errors later on.

  • What's happened to pUnkOuter? That seemed like an elegant solution to this type of problem.

  • Norman: I understand that you do not like the cast to (void**).

    I am not really sure exactly why you think that this is so bad in this particular case. What is your alterantive ?

    If you say that (void**) is bad, you must have something better in mind ?

    Or, in other words, when you are accusing other people of doing "ship-it" programming instead of programming "correctly", then you should really have a good idea yourself of what the "correct" code should be.

    Please enlight us with your infinite wisdom. Do not forget to motivate why the code you propose is more "correct" than using (void**).

  • Norman: I understand that you do not like the cast to (void**).

    I am not really sure exactly why you think that this is so bad in this particular case. What is your alterantive ?

    If you say that (void**) is bad, you must have something better in mind ?

    Or, in other words, when you are accusing other people of doing "ship-it" programming instead of programming "correctly", then you should really have a good idea yourself of what the "correct" code should be.

    Please enlight us with your infinite wisdom. Do not forget to motivate why the code you propose is more "correct" than using (void**).

  • Norman: I understand that you do not like the cast to (void**).

    I am not really sure exactly why you think that this is so bad in this particular case. What is your alterantive ?

    If you say that (void**) is bad, you must have something better in mind ?

    Or, in other words, when you are accusing other people of doing "ship-it" programming instead of programming "correctly", then you should really have a good idea yourself of what the "correct" code should be.

    Please enlight us with your infinite wisdom. Do not forget to motivate why the code you propose is more "correct" than using (void**).

  • sometype *somepointer;

    extern void f(void **ppv);

    // f(&somepointer) would get a diagnostic, deservedly.

    f((void**) &somepointer);  // wow, we shut up the diagnostic.

    somepointer might occupy 16 bytes because it points to a sometype, and the compiler knows that sometype has to meet some conditions so 16 bytes is enough to identify the object.

    void* might occupy 48 bytes because the compiler doesn't know what kind of object is being pointed to, and it has to prepare for the worst possible case.

    f will store 48 bytes into the 16 bytes occupied by somepointer and 32 bytes of innocent bystanders.

    Correct code:

    sometype *somepointer;

    void *voidpointer;

    extern void f(void **ppv);

    voidpointer = somepointer;

    f(&voidpointer);

    somepointer = (sometype *) voidpointer;  // See note

    (Note:  C doesn't require that cast, and I wish C++ didn't.  Excessive uses of casts causes people to become immune to casts, so they don't notice when bug-ridden casts are bug-ridden.)

    Friday, October 26, 2007 3:44 AM by stegus

    and

    Friday, October 26, 2007 3:44 AM by stegus

    and

    Friday, October 26, 2007 3:44 AM by stegus:

    > you should really have a good idea yourself of what

    > the "correct" code should be.

    Sure do, because I read K&R and the standard and some other stuff.

    > Please enlight us with your infinite wisdom.

    Sorry, no can do.  You provided an example of Einstein's statement about human stupidity being infinite.  However, wisdom is only finite, sorry.

  • Norman,

    OK, maybe I'm not awake enough, yet. But I too do not understand what your complaining about, let alone the numbers you state for memory being occupied.

    If you lookup CoCreateInstance() on MSDN (http://msdn2.microsoft.com/en-gb/library/ms686615.aspx), you'll see that the ppv parameter (he last one) is an OUT. So CoCreateInstance is going to return a pointer (to the interface), but it needs you to pass in a pointer of the location that is going to be containing this (first) pointer. Although not shown, obviously m_pEnumerator must be a member of CFoo.

    So what are you complaining about, the design of CoCreateInstance() ?

  • > the ppv parameter (he last one) is an OUT

    Right, so in that case you don't have to assign a value to the last parameter before doing the call.  No "in" value is needed.

    > it needs you to pass in a pointer of the location that is going

    > to be containing this (first) pointer

    Yes, and the caller must pass a pointer to a void*.

    After the caller receives a void* value in the variable that it pointed to, the caller can assign that void* value to a somethingelse* variable, if somethingelse is really the type that the value really points to.

    > So what are you complaining about, the design of

    > CoCreateInstance() ?

    No, I'm complaining about callers who lie, who say that their parameter is a pointer to a void*, when their pointer really points to a somethingelse*.  And I complain about companies that renege on warranties and refuse refunds for bug-ridden programs.

    If someone who doesn't understand C++ writes invalid C++ programs and lies to the compiler, for their own entertainment, or with advance warning for the entertainment of anyone who wants to play with them, fine.  When a company charges money for programs, they'd better provide better training so that their programmers will understand the language they're programming in.

  • Norman:

    Look, I fully understand the dangers of using casts just to shut-up the compiler.

    My point is that in this particular situation - when calling CoCreateInstance - using (void**) is not a "shut-up-the-compiler" cast, it is simpy the correct way of calling CoCreateInstance. There is absolutely no problems with that.

    Your argument that using a cast to (void**)  in this situation could lead to memory corruption is totally wrong. Under windows, all pointers are the same size. On Win32 all pointers are 32 bit, On Win64 all pointers are 64 bit. There might be other operating environments where pointer sizes might differ depending on what type of object they point to, but that never happens under windows.

    So, if I understand you correctly, you would like to change the following code:

    IMMDeviceEnumerator *pEnumerator;

    hr = CoCreateInstance(__uuidof(MMDeviceEnumerator), NULL, CLSCTX_INPROC_SERVER, __uuidof(IMMDeviceEnumerator), (void**)&pEnumerator)

    To something like this:

    IMMDeviceEnumerator *pEnumerator;

    void *pVoid;

    hr = CoCreateInstance(__uuidof(MMDeviceEnumerator), NULL, CLSCTX_INPROC_SERVER, __uuidof(IMMDeviceEnumerator), &pVoid)

    pEnumerator = (IMMDeviceEnumerator*)pVoid;

    There is no difference in functionality between the two pieces of code.

    Which version one prefers is largely a matter of personal taste, but I strongly prefer the first variant for the following reasons:

    1) The cast to (void**) in the call to CoCreateInstance is idiomatic code - that is simply the correct way to call that function.

    2) The cast to (IMMDeviceEnumerator*) in your code is really dangerous. The only way to know if it is correct or not is to look at surrounding code to determine exactly where pVoid comes from.

    3) The first version is how most people write COM code which means that other developers are more likely to be familiar with that kind of code.

    So, what you have done is basically replaced a correct completely safe idiomatic cast with a very dangerous shut-up-the-compiler cast that can easily introduce very nasty bugs.

    If I reviewed code like this I would definitely flag your kind of code as dangerous and recommend it to be rewritten.

  • Thursday, November 01, 2007 5:55 AM by stegus

    "My point is that in this particular situation - when calling CoCreateInstance - using (void**) is not a "shut-up-the-compiler" cast, it is simpy the correct way of calling CoCreateInstance. There is absolutely no problems with that."

    It is not the correct way.  I posted the correct way.  There are problems with that.  In the first edition of K&R they listed a few of the first machines on which they got C running.  Your way would already blow up on one of them, even though all pointers were 36 bits.  I gave an example with varying size pointers (like some IBM machines) in order to make it clearer, but that's not clear enough for you, right?

    You STILL refuse to read K&R, you refuse to read the standard, you refuse to recognize why correct code works correctly.  I lose this battle, wisdom is finite and I cannot illuminate you.  Of course you're not alone; yesterday Internet Explorer reminded me that it was programmed by someone with your skills.

  • Norman, you are not listening.

    You originally said that using a cast to (void**) in a call to CoCreateInstance was incorrect. You even accused people writing code that way of doing ship-it programming and you use this as an example of how incompetent Microsoft programmers are.

    I strongly disagree with your statement as I have explained in my comment above.

    Your comments about 36-bit pointers, varying pointer sizes depending on datatype, old IBM mainframes and such have absolutely nothing to do with what I am talking about.

    I am talking about calling CoCreateInstance on Windows - nothing else!

    If you want to continue discussing this, please stay on topic - the discussion is about calling CoCreateInstance on Windows from a C++ program.

    So far, it seems that your argument is really that casting to (void**) can be wrong if you are writing pure C++ code that calls another C++ function compiled with the same compiler and you want the code to be portable to several esoteric environments like old mainframes, microcontrollers, and such.

    That might or might not be a valid argument, but that is simply not the situation I am talking about!

    As a final comment: I have read K&R ("The C programming language" from 1972), I have read the ANSI C++ standard, I have over 20 years of experience with programming a large variety of different systems, you have absolutely no idea of my skills.

Page 1 of 2 (17 items) 12