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.

diff --git a/src/thread/sem_post.c b/src/thread/sem_post.c
index 8f4700c3058a4735516a5ae94330fb59df004234..148ab780e8f94d04ac2a0e222ff1f8631096708a 100644 (file)
--- 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 db05b417ca3703c58cea5a5c64148f004b569720..1d4b3e2c161534d33189f59b58743851013abfb9 100644 (file)
--- 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 dd8f57e36a0bd54a60a5b4c21516b9a1b6a8bba5..55d90075e274a5e0a82359ffc96e6b45a279e286 100644 (file)
--- 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;
 }