[Insight-developers] LightObject race condition fix

[Gaëtan Lehmann]

Le 10 juil. 07 à 21:59, Luis Ibanez a écrit :

>
> Hi Gaetan,
>
> Wasnt' the right solution to move the code:
>
>   if ( m_ReferenceCount <= 0)
>    {
>    delete this;
>    }
>
> inside the mutex locked section ?
>

Can the mutex be unlocked once destroyed by the delete call ? What  
happens in that case ?
In the current code (as well as with the proposed patch), delete is  
the last thing ran, which seem to be a quite safe behavior.

Here is the description by Julia of the race condition: “Two threads  
could be un-registering at the same time. The decrement is all fine,  
but as soon as you release the lock, one thread is going to win the  
delete race, and sometime later the second thread is going to examine  
the memory and will attempt delete again because the reference count  
is still zero or < 0. Saving the value before releasing the lock will  
guarantee the value of the reference count.”

About the Steve's example, a light object should never get a Register 
() call when m_ReferenceCount is 0, because at that point, the object  
is, or will be, deleted, so it is likely to show a bug in the  
program. The behavior with the patch in that example does not seem  
worth than without the patch - and it doesn't seem better though. In  
that case, I think it's up to the user to keep a smart pointer in a  
safe place so it can be safely referenced in a thread.

The changes made are as small as possible to avoid breaking anything,  
but there are surely some others changes to be made.


>
> --
>
> 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

--
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



-------------- next part --------------
A non-text attachment was scrubbed...
Name: PGP.sig
Type: application/pgp-signature
Size: 186 bytes
Desc: =?ISO-8859-1?Q?Ceci_est_une_signature_=E9lectronique_PGP?=
Url : http://www.itk.org/mailman/private/insight-developers/attachments/20070710/9c1e3722/PGP.pgp