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.

How does thread 2 obtain that new reference? You should not allow re-using an object which ref count has become zero. You'd rather create a new object to give to the thread 2. And if you hold a strong reference to the object somewhere across threads, thread 1 would not make the object's ref count zero. — noseratio, Oct 02 '13 at 17:16
@Noseratio: thanks for your response. OK, so in the *QueryInterface* and *AddRef* methods I must check that the counter is not already 0 (this boolean is my flag). For *QueryInterface* I can make the caller aware of the rejection of its request, but for *AddRef* is there any standard pattern? If I return 0, then the caller should check the return value, but this value is "intended to be used only for test purposes" according to the MSDN documentation... — Pragmateek, Oct 02 '13 at 17:29

Hans Passant · Accepted Answer · 2013-10-02T18:13:36.587

6

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.

edited Oct 02 '13 at 18:13

answered Oct 02 '13 at 17:47

Hans Passant

922,412
146
1,693
2,536

Correct! Thanks *Hans*. :) – Pragmateek Oct 02 '13 at 17:57

score 4 · Answer 2 · edited May 23 '17 at 12:29

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:

Thread 1 & Thread 2 have a reference to the object (m_cRef == 2)
Thread 1 calls Release() and is interrupted immediately after running InterlockedDecrement() (m_cRef == 2)
Thread 2 calls Release() and runs to completion, so m_cRef == 0 so it calls delete this.
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?

thanks for pointing this race condition. In my code I always use *ulRefCount* so it's fine, but I've never considered this double free issue. Good point. :) — Pragmateek, Oct 02 '13 at 19:06
This is all good, but I'll say that it's not the potential "double free" that is the first problem to occur, it may happen that the comparison runs into undefined behavior because the member variable belongs to an already deleted object. — sharptooth, Oct 03 '13 at 06:15

IUnknown.Release standard implementation race condition?

2 Answers2