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(a)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