From 97c5b5a87c3d9df54278e1073d6177f77536bd32 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Fri, 23 Sep 2011 22:58:45 -0400 Subject: [PATCH] fix ABA race in cond vars, improve them overall previously, a waiter could miss the 1->0 transition of block if another thread set block to 1 again after the signal function set block to 0. we now use the caller's thread id as a unique token to store in block, which no other thread will ever write there. this ensures that if block still contains the tid, no signal has occurred. spurious wakeups will of course occur whenever there is a spurious return from the futex wait and another thread has begun waiting on the cond var. this should be a rare occurrence except perhaps in the presence of interrupting signal handlers. signal/bcast operations have been improved by noting that they need not avoid inspecting the cond var's memory after changing the futex value. because the standard allows spurious wakeups, there is no way for an application to distinguish between a spurious wakeup just before another thread called signal/bcast, and the deliberate wakeup resulting from the signal/bcast call. thus the woken thread must assume that the signalling thread may still be waiting to act on the cond var, and therefore it cannot destroy/unmap the cond var. --- src/thread/pthread_cond_broadcast.c | 5 ++--- src/thread/pthread_cond_signal.c | 5 ++--- src/thread/pthread_cond_timedwait.c | 13 ++++++++----- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/thread/pthread_cond_broadcast.c b/src/thread/pthread_cond_broadcast.c index 6002c535..dec91164 100644 --- a/src/thread/pthread_cond_broadcast.c +++ b/src/thread/pthread_cond_broadcast.c @@ -2,8 +2,7 @@ int pthread_cond_broadcast(pthread_cond_t *c) { - int w = c->_c_waiters; - if (a_swap(&c->_c_block, 0) || w) - __wake(&c->_c_block, -1, 0); + a_store(&c->_c_block, 0); + if (c->_c_waiters) __wake(&c->_c_block, -1, 0); return 0; } diff --git a/src/thread/pthread_cond_signal.c b/src/thread/pthread_cond_signal.c index e8ed71cc..b26b8ce5 100644 --- a/src/thread/pthread_cond_signal.c +++ b/src/thread/pthread_cond_signal.c @@ -2,8 +2,7 @@ int pthread_cond_signal(pthread_cond_t *c) { - int w = c->_c_waiters; - if (a_swap(&c->_c_block, 0) || w) - __wake(&c->_c_block, 1, 0); + a_store(&c->_c_block, 0); + if (c->_c_waiters) __wake(&c->_c_block, 1, 0); return 0; } diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c index ec5aa6f4..c71edc95 100644 --- a/src/thread/pthread_cond_timedwait.c +++ b/src/thread/pthread_cond_timedwait.c @@ -15,19 +15,22 @@ static void cleanup(void *p) int pthread_cond_timedwait(pthread_cond_t *c, pthread_mutex_t *m, const struct timespec *ts) { struct cm cm = { .c=c, .m=m }; - int r, e=0; + int r, e, tid; if (ts && ts->tv_nsec >= 1000000000UL) return EINVAL; pthread_testcancel(); - c->_c_block = 1; + a_inc(&c->_c_waiters); + c->_c_block = tid = pthread_self()->tid; + if ((r=pthread_mutex_unlock(m))) return r; - a_inc(&c->_c_waiters); - do e = __timedwait(&c->_c_block, 1, c->_c_clock, ts, cleanup, &cm, 0); - while (e == EINTR); + do e = __timedwait(&c->_c_block, tid, c->_c_clock, ts, cleanup, &cm, 0); + while (c->_c_block == tid && (!e || e==EINTR)); + if (e == EINTR) e = 0; + a_dec(&c->_c_waiters); if ((r=pthread_mutex_lock(m))) return r; -- 2.25.1