IUnknown.Release standard implementation race condition?

Question

Here is a standard (not to say recommended) way of implementing Release method of the IUnknown COM interface (directly taken from MSDN):

ULONG CMyMAPIObject::Release()
{
    // Decrement the object's internal counter.
    ULONG ulRefCount = InterlockedDecrement(m_cRef);
    if (0 == m_cRef)
    {
        delete this;
    }
    return ulRefCount;
}

I was wondering if a race condition could occur if the apartment model is not STA:

• say there is one reference left
• thread 1 releases it by calling Release
• it runs and is stopped just before delete this
• thread 2 is scheduled and obtain a new reference to the object, e.g. by calling QueryInterface or AddRef
• thread 1 continue to execute and runs delete this
• thread 2 is left with an invalid object

For me the only way to ensure consistency would be to create a flag, say deleted, lock the whole critical section, i.e. all the Release method except the return, and set the flag to true.

And in the AddRef and QueryInterface methods check this flag, and if it's set then reject the request for new reference.

What am I missing?

Thanks in advance.

Answer 1

thread 2 is scheduled and obtain a new reference to the object, e.g. by calling QueryInterface or AddRef

It can only do this if it already has a reference to IUnknown or one of the other interfaces implemented by the object. For which an AddRef() call was made earlier. The reference count can thus never be decreased to a value less than 1 by another thread's Release call.

Do write the code correctly, you must compare ulRefCount to 0, not m_cRef.

Answer 2

There is a race condition in the code, but it's not the one in your example. This implementation of Release() has a race condition that can cause undefined behavior (typically a "double free"). Consider:

1. Thread 1 & Thread 2 have a reference to the object (m_cRef == 2)
2. Thread 1 calls Release() and is interrupted immediately after running InterlockedDecrement() (m_cRef == 2)
3. Thread 2 calls Release() and runs to completion, so m_cRef == 0 so it calls delete this.
4. Thread 1 resumes at the line if (0 == m_cRef) and m_cRef == 0 so it calls delete this again causing undefined behavior (typically a "double free" error).

The correct implementation is:

ULONG CMyMAPIObject::Release()
{
    // Decrement the object's internal counter.
    ULONG ulRefCount = InterlockedDecrement(m_cRef);
    if (0 == ulRefCount) //<<<< THIS FIXES THE PROBLEM
    {
        delete this;
    }
    return ulRefCount;
}

The if check needs to be on the local ulRefCount variable. Because InterlockedDecrement() returns the value that it decremented to, ulRefCount will only be zero on Thread 2's call so delete this will only be called on Thread 2.

The if (0 == m_cRef) is accessing shared state without a lock and is not safe.

See the answer to this question as well: Why does this implementation of COM IUnknown::Release work?