[Insight-developers] Re: LightObject race condition fix

[Simon Warfield]

So imagine the following sequence of events:
Object A in thread 1 calls UnRegister() on a LightObject L shared across 
threads.

L obtains a lock, and decrements the temporary reference count to be 
equal to zero, and releases the lock.

Object B in thread 2 , unaware that Object A has asked L to destroy 
itself, calls Register().
It obtains a lock on the reference count variable, and m_ReferenceCount 
variable goes back up to 1, and then the lock is released.

Light object L then commits suicide by calling delete this, since its 
temporary reference count variable is 0.

Object B is not expecting this to happen, since it just incremented the 
reference count.
Object B dereferences some pointer  inside L (or a class derived from L) 
was managing for it, gets random memory contents, and crashes with a 
segmentation violation. 

> The changes are:
>
> [glehmann at marvin Common]$ cvs diff -u
> Index: itkLightObject.cxx
> ===================================================================
> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.cxx,v
> retrieving revision 1.34
> diff -u -r1.34 itkLightObject.cxx
> --- itkLightObject.cxx  4 Apr 2007 20:04:10 -0000       1.34
> +++ itkLightObject.cxx  6 Aug 2007 12:07:57 -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;
>       }
> Index: itkLightObject.h
> ===================================================================
> RCS file: /cvsroot/Insight/Insight/Code/Common/itkLightObject.h,v
> retrieving revision 1.33
> diff -u -r1.33 itkLightObject.h
> --- itkLightObject.h    6 Feb 2006 22:01:56 -0000       1.33
> +++ itkLightObject.h    6 Aug 2007 12:07:57 -0000
> @@ -114,7 +114,7 @@
>     virtual void PrintTrailer(std::ostream& os, Indent indent) const;
>
>     /** Number of uses of this object by other objects. */
> -  mutable int m_ReferenceCount;
> +  volatile mutable int m_ReferenceCount;
>     /** Mutex lock to protect modification to the reference count */
>     mutable SimpleFastMutexLock m_ReferenceCountLock;
>
> Gaëtan
>
>   

-- 
Simon