[Insight-developers] LightObject race condition fix

[Luis Ibanez]

Hi Gaetan,

Wasnt' the right solution to move the code:

   if ( m_ReferenceCount <= 0)
    {
    delete this;
    }

inside the mutex locked section ?


--

About "volatile" its purpose is to let the compiler
know that the variable may change its value for reasons
that can not be deduced from the current code context.

Its purpose is to prevent the compiler from making
optimization decision based on the assumption that
the variable is not directly modified by assignment
code in the current code context.



    Luis


---------------------------
Steve M. Robbins wrote:
> Hi,
> 
> I'm not sure what race conditions Julia Smith has uncovered, but the
> first change looks like it creates a worse race.
> 
> 
> On Tue, Jul 10, 2007 at 08:51:21PM +0200, Gaëtan Lehmann wrote:
> 
> 
>>Index: Code/Common/itkLightObject.cxx
>>===================================================================
>>RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.cxx,v
>>retrieving revision 1.34
>>diff -u -r1.34 itkLightObject.cxx
>>--- Code/Common/itkLightObject.cxx      4 Apr 2007 20:04:10 -0000       
>>1.34
>>+++ Code/Common/itkLightObject.cxx      10 Jul 2007 18:41:08 -0000
>>@@ -150,12 +150,12 @@
>>::UnRegister() const
>>{
>>   m_ReferenceCountLock.Lock();
>>-  m_ReferenceCount--;
>>+  int tmpReferenceCount = --m_ReferenceCount;
>>   m_ReferenceCountLock.Unlock();
>>
> 
>      [***]
> 
> 
>>   // ReferenceCount in now unlocked.  We may have a race condition
>>   // to delete the object.
>>-  if ( m_ReferenceCount <= 0)
>>+  if ( tmpReferenceCount <= 0)
>>     {
>>     delete this;
>>     }
> 
> 
> Imagine that thread 1 has a reference to a LightObject L with
> m_ReferenceCount = 1 and calls L->Unregister().  The test for zero
> references happens outside the critical section, so imagine what
> happens when thread 2 calls L->Register() while thread 1 is at line
> [***].  Thread 2 thinks it has obtained a reference and is going to be
> very unhappy after thread 1 deletes L.
> 
> I'm not clear on the semantics of "volatile", so it's not clear to me
> whether the other change (marking m_ReferenceCount as volatile) is
> sufficient to cure this race in the original code.
> 
> If volatile isn't sufficient, why not just "delete this" while holding
> the lock?  You'd also need to worry about threads blocking in Register()
> while the object is deleted.  Presumably SimpleFastMutexLock.Lock()
> would have to throw if the lock was destructed.  Or something.
> 
> My head hurts.
> -Steve
> 
> 
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers