[Insight-developers] Re: LightObject race condition fix

[Gaëtan Lehmann]

Le 6 août 07 à 22:26, kent williams a écrit :

> Simon's hypothetical also "should" never happen -- if thread A can  
> decrement
> the reference count to zero, then there should be no existing  
> references to
> the object for B to acquire and call Register() with.  If there is an
> existing reference for B to acquire, then A should leave the object  
> with a
> reference count of at least 1 when it unlocks, and A won't call  
> 'delete
> this'.
>
> The operative phrase is '... on a LightObject L shared across  
> threads': Each
> thread should count as a reference, and call Register() for  
> themselves.  If
> they 'share' an object without calling Register() for themselves,  
> then they
> should not both call UnRegister(), because it breaks the Register/ 
> Unregister
> symmetry.
>
> I think Gaetan's patch only accidentally solves Julia Smith's race
> condition.  Perhaps this would do a better job:
>
> 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;
>       }
>
> In this case only one thread can get a tmpReferenceCount exactly  
> equal to
> zero.

I agree with that, and haven't done it only to minimize the changes.

However, for Julia Smith's problem, the original patch would also  
work. In her case, there are 2 pointers which are destroyed at the  
same time in 2 different threads. The reference count can be  
decremented to 0 before the 2 threads comes to the if  
( m_ReferenceCount <= 0), and then, the object may be deleted twice.  
With the patch, each thread do the test with its own  
tmpReferenceCount, and only one of the two tmpReferenceCount can be 0  
(the other one is 1).

>
> More to the point, there's a problem in Julia Smith's code if two  
> threads
> are both decrementing m_ReferenceCount < 0.

they are not - see the example above

>   Either each thread should call
> x->Register() for all shared objects 'x' and x->Unregister() when  
> they're
> done with it, or they shouldn't be calling x->Unregister() at all  
> because
> they don't own the reference.
>
> If the problem Julia is seeing is inherent somehow to Java's  
> symantics, then
> the ITK wrapping for Java is wrong for this problem to arise.
>
> On 8/6/07 1:38 PM, "Gaëtan Lehmann" <gaetan.lehmann at jouy.inra.fr>  
> wrote:
>
>>
>> Le 6 août 07 à 18:29, Simon Warfield a écrit :
>>
>>>
>>> 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.
>>
>> Yes, that's a problem, but it is also there currently.
>> This patch does not try to fix that - only the problem reported by
>> Julia Smith.
>>
>> Gaëtan
>>
>>
>>>> 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
>>> _______________________________________________
>>> Insight-developers mailing list
>>> Insight-developers at itk.org
>>> http://www.itk.org/mailman/listinfo/insight-developers
>>
>> --
>> Gaëtan Lehmann
>> Biologie du Développement et de la Reproduction
>> INRA de Jouy-en-Josas (France)
>> tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
>> http://voxel.jouy.inra.fr
>>
>>
>>
>> _______________________________________________
>> Insight-developers mailing list
>> Insight-developers at itk.org
>> http://www.itk.org/mailman/listinfo/insight-developers
>
> _______________________________________________
> Insight-developers mailing list
> Insight-developers at itk.org
> http://www.itk.org/mailman/listinfo/insight-developers

--
Gaëtan Lehmann
Biologie du Développement et de la Reproduction
INRA de Jouy-en-Josas (France)
tel: +33 1 34 65 29 66    fax: 01 34 65 29 09
http://voxel.jouy.inra.fr