summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRich Felker <dalias@aerifal.cx>2011-08-02 19:19:09 -0400
committerRich Felker <dalias@aerifal.cx>2011-08-02 19:19:09 -0400
commit88c4e720317845a8e01aee03f142ba82674cd23d (patch)
tree54c92b29227e097c994e7d902e7c35dca60fe109
parent88798393cab009ce78fe498051072db71ba9d035 (diff)
downloadmusl-88c4e720317845a8e01aee03f142ba82674cd23d.tar.gz
musl-88c4e720317845a8e01aee03f142ba82674cd23d.tar.bz2
musl-88c4e720317845a8e01aee03f142ba82674cd23d.tar.xz
musl-88c4e720317845a8e01aee03f142ba82674cd23d.zip
overhaul posix semaphores to fix destructability race
the race condition these changes address is described in glibc bug report number 12674: http://sourceware.org/bugzilla/show_bug.cgi?id=12674 up until now, musl has shared the bug, and i had not been able to figure out how to eliminate it. in short, the problem is that it's not valid for sem_post to inspect the waiters count after incrementing the semaphore value, because another thread may have already successfully returned from sem_wait, (rightly) deemed itself the only remaining user of the semaphore, and chosen to destroy and free it (or unmap the shared memory it's stored in). POSIX is not explicit in blessing this usage, but it gives a very explicit analogous example with mutexes (which, in musl and glibc, also suffer from the same race condition bug) in the rationale for pthread_mutex_destroy. the new semaphore implementation augments the waiter count with a redundant waiter indication in the semaphore value itself, representing the presence of "last minute" waiters that may have arrived after sem_post read the waiter count. this allows sem_post to read the waiter count prior to incrementing the semaphore value, rather than after incrementing it, so as to avoid accessing the semaphore memory whatsoever after the increment takes place. a similar, but much simpler, fix should be possible for mutexes and other locking primitives whose usage rules are stricter than semaphores.
-rw-r--r--src/thread/sem_post.c9
-rw-r--r--src/thread/sem_timedwait.c36
-rw-r--r--src/thread/sem_trywait.c5
3 files changed, 23 insertions, 27 deletions
diff --git a/src/thread/sem_post.c b/src/thread/sem_post.c
index 8f4700c3..148ab780 100644
--- a/src/thread/sem_post.c
+++ b/src/thread/sem_post.c
@@ -3,8 +3,11 @@
int sem_post(sem_t *sem)
{
- a_inc(sem->__val);
- if (sem->__val[1])
- __wake(sem->__val, 1, 0);
+ int val, waiters;
+ do {
+ val = sem->__val[0];
+ waiters = sem->__val[1];
+ } while (a_cas(sem->__val, val, val+1+(val<0)) != val);
+ if (val<0 || waiters) __wake(sem->__val, 1, 0);
return 0;
}
diff --git a/src/thread/sem_timedwait.c b/src/thread/sem_timedwait.c
index db05b417..1d4b3e2c 100644
--- a/src/thread/sem_timedwait.c
+++ b/src/thread/sem_timedwait.c
@@ -8,31 +8,21 @@ static void cleanup(void *p)
int sem_timedwait(sem_t *sem, const struct timespec *at)
{
- int r;
-
- if (a_fetch_add(sem->__val, -1) > 0) return 0;
- a_inc(sem->__val);
-
- if (at && at->tv_nsec >= 1000000000UL) {
- errno = EINVAL;
- return -1;
- }
-
- a_inc(sem->__val+1);
- pthread_cleanup_push(cleanup, sem->__val+1)
-
- for (;;) {
- r = 0;
- if (!sem_trywait(sem)) break;
- r = __timedwait_cp(sem->__val, 0, CLOCK_REALTIME, at, 0);
+ while (sem_trywait(sem)) {
+ int r;
+ if (at && at->tv_nsec >= 1000000000UL) {
+ errno = EINVAL;
+ return -1;
+ }
+ a_inc(sem->__val+1);
+ a_cas(sem->__val, 0, -1);
+ pthread_cleanup_push(cleanup, sem->__val+1);
+ r = __timedwait_cp(sem->__val, -1, CLOCK_REALTIME, at, 0);
+ pthread_cleanup_pop(1);
if (r) {
errno = r;
- r = -1;
- break;
+ return -1;
}
}
-
- pthread_cleanup_pop(1);
-
- return r;
+ return 0;
}
diff --git a/src/thread/sem_trywait.c b/src/thread/sem_trywait.c
index dd8f57e3..55d90075 100644
--- a/src/thread/sem_trywait.c
+++ b/src/thread/sem_trywait.c
@@ -4,7 +4,10 @@
int sem_trywait(sem_t *sem)
{
int val = sem->__val[0];
- if (val>0 && a_cas(sem->__val, val, val-1)==val) return 0;
+ if (val>0) {
+ int new = val-1-(val==1 && sem->__val[1]);
+ if (a_cas(sem->__val, val, new)==val) return 0;
+ }
errno = EAGAIN;
return -1;
}