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