From f8934b2c9df5e4137c33e9f47950bcfce6988367 Mon Sep 17 00:00:00 2001
From: Joel Dice <joel.dice@gmail.com>
Date: Wed, 14 Mar 2012 11:38:20 -0600
Subject: [PATCH] fix race condition in System::Monitor::wait

If we clear Thread::flags before releasing the thread mutex and
re-acquiring the monitor mutex, it's possible that we will be notified
between the release and re-acquire, which will confuse us later if we
try to wait on the same monitor again such that we well not remove
ourselves from the wait list because we think we've been removed by
the notifier.

The solution is to wait until we've acquired both mutexes before we
clear Thread::flags.
---
 src/posix.cpp   | 34 ++++++++++++++++++++++++++++------
 src/windows.cpp | 27 +++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/src/posix.cpp b/src/posix.cpp
index 074c6b379c..35d2d6b7b1 100644
--- a/src/posix.cpp
+++ b/src/posix.cpp
@@ -122,8 +122,7 @@ pathOfExecutable(System* s, const char** retBuf, unsigned* size)
 
 const bool Verbose = false;
 
-const unsigned Waiting = 1 << 0;
-const unsigned Notified = 1 << 1;
+const unsigned Notified = 1 << 0;
 
 class MySystem: public System {
  public:
@@ -256,7 +255,14 @@ class MySystem: public System {
     }
 
     void append(Thread* t) {
+#ifndef NDEBUG
+      for (Thread* x = first; x; x = x->next) {
+        expect(s, t != x);
+      }
+#endif
+
       if (last) {
+        expect(s, t != last);
         last->next = t;
         last = t;
       } else {
@@ -271,6 +277,7 @@ class MySystem: public System {
           if (current == first) {
             first = t->next;
           } else {
+            expect(s, previous != t->next);
             previous->next = t->next;
           }
 
@@ -286,6 +293,12 @@ class MySystem: public System {
           current = current->next;
         }
       }
+
+#ifndef NDEBUG
+      for (Thread* x = first; x; x = x->next) {
+        expect(s, t != x);
+      }
+#endif
     }
 
     virtual void wait(System::Thread* context, int64_t time) {
@@ -308,13 +321,13 @@ class MySystem: public System {
 
         { ACQUIRE(t->mutex);
       
+          expect(s, (t->flags & Notified) == 0);
+
           interrupted = t->r->interrupted();
           if (interrupted and clearInterrupted) {
             t->r->setInterrupted(false);
           }
 
-          t->flags |= Waiting;
-
           append(t);
 
           depth = this->depth;
@@ -343,14 +356,22 @@ class MySystem: public System {
           }
 
           notified = ((t->flags & Notified) != 0);
-        
-          t->flags = 0;
         }
 
         pthread_mutex_lock(&mutex);
 
+        { ACQUIRE(t->mutex);
+          t->flags = 0;
+        }
+
         if (not notified) {
           remove(t);
+        } else {
+#ifndef NDEBUG
+          for (Thread* x = first; x; x = x->next) {
+            expect(s, t != x);
+          }
+#endif
         }
 
         t->next = 0;
@@ -380,6 +401,7 @@ class MySystem: public System {
           Thread* t = first;
           first = first->next;
           if (t == last) {
+            expect(s, first == 0);
             last = 0;
           }
 
diff --git a/src/windows.cpp b/src/windows.cpp
index 42a79d3024..37c9251726 100644
--- a/src/windows.cpp
+++ b/src/windows.cpp
@@ -215,6 +215,12 @@ class MySystem: public System {
     }
 
     void append(Thread* t) {
+#ifndef NDEBUG
+      for (Thread* x = first; x; x = x->next) {
+        expect(s, t != x);
+      }
+#endif
+
       if (last) {
         last->next = t;
         last = t;
@@ -245,6 +251,12 @@ class MySystem: public System {
           current = current->next;
         }
       }
+
+#ifndef NDEBUG
+      for (Thread* x = first; x; x = x->next) {
+        expect(s, t != x);
+      }
+#endif
     }
 
     virtual void wait(System::Thread* context, int64_t time) {
@@ -270,6 +282,8 @@ class MySystem: public System {
 
         { ACQUIRE(s, t->mutex);
 
+          expect(s, (t->flags & Notified) == 0);
+
           interrupted = t->r->interrupted();
           if (interrupted and clearInterrupted) {
             t->r->setInterrupted(false);
@@ -306,15 +320,23 @@ class MySystem: public System {
           }
 
           notified = ((t->flags & Notified) != 0);
-        
-          t->flags = 0;
         }
 
         r = WaitForSingleObject(mutex, INFINITE);
         assert(s, r == WAIT_OBJECT_0);
 
+        { ACQUIRE(s, t->mutex);
+          t->flags = 0;
+        }
+
         if (not notified) {
           remove(t);
+        } else {
+#ifndef NDEBUG
+          for (Thread* x = first; x; x = x->next) {
+            expect(s, t != x);
+          }
+#endif
         }
 
         t->next = 0;
@@ -346,6 +368,7 @@ class MySystem: public System {
           Thread* t = first;
           first = first->next;
           if (t == last) {
+            expect(s, first == 0);
             last = 0;
           }