IUnkown::Release() implementation


Mike Stall talks about “Don’t do complicated work in Release()”. He has a sample implementation for IUnkown::Release().

 

<quote>

 

An object’s destructor has the same caveats if Release() is often implemented as:

 

InterlockedDecrement(&m_ref)

if (m_ref == 0) { delete this; }

 

</quote>

 

There is a bug in this implementation, that m_ref is accessed after InterlockedDecrement. The problem is the object may have been deleted after InterlockedDecrement. Thus access violation exception may occur when m_ref is accessd after the memory has been reclaimed.

 

Imagine the object’s ref count is 2. There are two threads calling Release() simultaneously.

 

Thread 1                                                                      Thread 2

 

InterlockedDecrement(&m_ref);

//m_ref now is 1

           

                                                                                    InterlockedDecrement(&m_ref);

                                                                                    // m_ref now is 0

                                                                                    if (m_ref==0) {delete this;}

                                                                                    // now the object is gone

 

// access violation here, since the object is gone

if (m_ref==0) {delete this;}

 

The right implementation is

 

if (!InterlockedDecrement(&m_ref)) {delete this;}

 

We have found a few occurrences of this pattern in CLR’s code base. This bug is revealed in our internal ASP.NET stress. We have fixed all of them in CLR’s codebase since.

 

Devil is in the details.

Comments (8)

  1. You guys might want to check the RCW cookie cutter template code used in ActiveX imports for that as well =)

    I’ve been seeing quite a few hard to pin down thread deadlocks with code that uses the IE control and I think it must be something like this. I ended up doing a manual AddRef() on the control and having to live with one leaked instance to avoid an app getting frozen to heck.

  2. Amit Joshi says:

    Interesting! Why are people still in a business of writing COM components by hand when ATL does such a nice job of automating those mundane details?

    I am perplexed and frustrated to see a lot of code written by MS that hand implements COM objects and fails to provide consistent implementation of class factories, QI (believe it or not, but one object refused QI for IUnknown), aggregation etc.

    What could be the reason for not using ATL or any such framework?

  3. As a critical component in .Net framework, CLR has rules on what can be used and what cannot in our codebase.

    For example, we are not allowed to use STL, nor ATL.

    If you ever read Rotor’s code, you will notice this.

  4. Amit Joshi says:

    Though I haven’t read Rotor’s code, I have read/debugged enough other code from MS with hand implemented, incomplete, inconsistent and often buggy implementations of COM contracts.

    Reusing, extending those objects is a big pain. Imagine the frustation when you debug into someone else’s QI and find out that the author of the QI forgot to add a case for IUnknown! I can understand not using STL, but knowing little bit about ATL (and the claims of newer optimizing compilers) I have difficulty in understanding the resons not to use it.

    Even if ATL is not used, I would expect there to be some common internal framework used to implement such boilerplate code. Please help me understand why reinventing the wheel makes sense here?

  5. Rico Mariani says:

    You can never write code of this form:

    InterlockedDecrement(&m_ref )

    if (m_ref == 0) { delete this; }

    You might just as well have written

    m_ref –;

    if (m_ref == 0) { delete this; }

    By not using the return of the InterlockedDecrement it’s possible that two threads will see the zero value and the destructor will be run twice, possibly in an overlapped fashion. Accessing m_ref after the operation is just the tip of an iceberg of problems.

  6. Doron Orbach says:

    Did tyis solution worked?

    D. Orbach<br/>

    <a href="http://www.javasight.com"><b>javasight – java news & books</b></a>

  7. Doron Orbach says:

    Did tyis solution worked?

    D. Orbach

    http://www.javasight.com javasight – java news & books