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