Re: [boost] Re: [Boost.Thread] mutex Win32 implementation changes

From: Peter Dimov (pdimov_at_[hidden])
Date: 2004-07-19 12:56:56

Alexander Terekhov wrote:
> Peter Dimov wrote:
>>
>> Alexander Terekhov wrote:
>>>
>>> With respect to swap_based_mutex_for_windows thing (without
>>> pimpl-and-never-destroyed-impl), the problem is that it can go boom
>>> in unlock().
>>>
>>>   void lock() throw() {
>>>     if (m_lock_status.swap(1, msync::acq))
>>>       while (m_lock_status.swap(-1, msync::acq))
>>>         m_retry_event.wait();
>>>   }
>>>
>>>   void unlock() throw() {
>>>     if (m_lock_status.swap(0, msync::rel) < 0)
>>>       m_retry_event.set();
>>>   }
>>>
>>> Scenario...
>>>
>>>   Given: mutex protected refcounting. Two threads, A and B.
>>>
>>>      t0: thread A locks the mutex and decrements refcount to 2;
>>>
>>>      t1: thread B does m_lock_status.swap(1, msync::acq) on the
>>>          fast path and sees 1;
>>>
>>>      t2: thread A unlocks the mutex (doesn't enter slow path);
>>>
>>>      t3: thread B does mutex m_lock_status.swap(-1, msync::acq),
>>>          locks the mutex, decrements refcount to 1, does
>>>          m_lock_status.swap(0, msync::rel) and enters slow path
>>>          in unlock();
>>
>>     SetEvent( e_ );
>
> Nah, it suspends right before it.
>
>>
>>>      t4: thread A locks the mutex, decrements refcount to 0,
>>>          unlocks the mutex, and destroys it (including event);
>>
>>     CloseHandle( e_ );
>
> and operator delete() [with subsequent "unmap"].

Yes, I see it now. I see no protection in MS's CRITICAL_SECTION against
this, either. It's basically (recursivity omitted)

void lock( critical_section * p )
{
    if( atomic_increment(p->LockCount) != 0 )
    {
        slow_lock_path( p );
    }
}

void unlock( critical_section * p )
{
    if( atomic_decrement(p->LockCount) >= 0 )
    {
        slow_unlock_path( p );
    }
}

and it seems to me that slow_unlock_path happily accesses *p, in particular
something called p->LockSemaphore (whether it is really a semaphore is
another story).