Re: [PATCH] msvcr120: Fixed a race condition in Concurrency::critical_section.

[Piotr Caban]

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