On 12/31/14 00:45, Yifu Wang wrote:
@@ -380,11 +393,18 @@ void __thiscall critical_section_lock(critical_section *this) memset(&q, 0, sizeof(q)); last = InterlockedExchangePointer(&this->tail, &q); if(last) {
/* If last == &this->unk_active, the current thread might be racing setting* unk_active.next with another thread calling cs_set_head(). If the current* thread sets unk_active.next first, the data structure would be left in an* inconsistent state and causes hang. Spin waiting until unk_active.next* becomes NULL solves the race condition.*/if(last == &this->unk_active)spin_wait_for_next_null(&this->unk_active); last->next = &q; NtWaitForKeyedEvent(keyed_event, &q, 0, NULL); }
- this->unk_active.next = NULL; if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) spin_wait_for_next_cs(&q); cs_set_head(this, &q);
There's still a race with your patch. There's no guarantee that this->unk_active.next != NULL before cs_set_head(this, &q) is called.
Changing the place where cs_set_head is called should fix the issue in cleaner way: diff --git a/dlls/msvcrt/lock.c b/dlls/msvcrt/lock.c index 328b5b5..e2ebef3 100644 --- a/dlls/msvcrt/lock.c +++ b/dlls/msvcrt/lock.c @@ -384,10 +397,11 @@ void __thiscall critical_section_lock(critical_section *this) NtWaitForKeyedEvent(keyed_event, &q, 0, NULL); }
- this->unk_active.next = NULL; - if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) - spin_wait_for_next_cs(&q); cs_set_head(this, &q); + if(InterlockedCompareExchangePointer(&this->tail, &this->unk_active, &q) != &q) { + spin_wait_for_next_cs(&q); + this->unk_active.next = q.next; + } }
/* ?try_lock@critical_section@Concurrency@@QAE_NXZ */
There's also similar race in critical_section_try_lock and critical_section_try_lock_for function. Could you please also fix it there?
Thank you, Piotr