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.
This commit is contained in:
Joel Dice 2012-03-14 11:38:20 -06:00
parent eb9f5c6dcc
commit f8934b2c9d
2 changed files with 53 additions and 8 deletions

View File

@ -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;
}

View File

@ -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;
}