diff options
| author | Rich Felker <dalias@aerifal.cx> | 2013-09-20 02:00:27 -0400 |
|---|---|---|
| committer | Rich Felker <dalias@aerifal.cx> | 2013-09-20 02:00:27 -0400 |
| commit | e803829e6b087c0ed91adc11f87185109bc59b31 (patch) | |
| tree | ad2fdda58fbba2c542785d9cc8babcd5f7d001ec | |
| parent | d8e283df58eb8bff1aa2f8a99347e294c7f67cb9 (diff) | |
| download | musl-e803829e6b087c0ed91adc11f87185109bc59b31.tar.gz musl-e803829e6b087c0ed91adc11f87185109bc59b31.tar.xz musl-e803829e6b087c0ed91adc11f87185109bc59b31.zip | |
fix potential deadlock bug in libc-internal locking logic
if a multithreaded program became non-multithreaded (i.e. all other threads exited) while one thread held an internal lock, the remaining thread would fail to release the lock. the the program then became multithreaded again at a later time, any further attempts to obtain the lock would deadlock permanently. the underlying cause is that the value of libc.threads_minus_1 at unlock time might not match the value at lock time. one solution would be returning a flag to the caller indicating whether the lock was taken and needs to be unlocked, but there is a simpler solution: using the lock itself as such a flag. note that this flag is not needed anyway for correctness; if the lock is not held, the unlock code is harmless. however, the memory synchronization properties associated with a_store are costly on some archs, so it's best to avoid executing the unlock code when it is unnecessary.
| -rw-r--r-- | src/internal/libc.h | 4 | ||||
| -rw-r--r-- | src/malloc/malloc.c | 15 | ||||
| -rw-r--r-- | src/thread/__lock.c | 9 |
3 files changed, 15 insertions, 13 deletions
diff --git a/src/internal/libc.h b/src/internal/libc.h index 3350b3d1..d625b56a 100644 --- a/src/internal/libc.h +++ b/src/internal/libc.h @@ -53,8 +53,8 @@ void __lock(volatile int *) ATTR_LIBC_VISIBILITY; void __unlock(volatile int *) ATTR_LIBC_VISIBILITY; int __lockfile(FILE *) ATTR_LIBC_VISIBILITY; void __unlockfile(FILE *) ATTR_LIBC_VISIBILITY; -#define LOCK(x) (libc.threads_minus_1 ? (__lock(x),1) : ((void)(x),1)) -#define UNLOCK(x) (libc.threads_minus_1 ? (__unlock(x),1) : ((void)(x),1)) +#define LOCK(x) __lock(x) +#define UNLOCK(x) __unlock(x) void __synccall(void (*)(void *), void *); int __setxid(int, int, int, int); diff --git a/src/malloc/malloc.c b/src/malloc/malloc.c index 4044eb2a..fb65ab5b 100644 --- a/src/malloc/malloc.c +++ b/src/malloc/malloc.c @@ -64,28 +64,27 @@ static struct { static inline void lock(volatile int *lk) { - if (!libc.threads_minus_1) return; - while(a_swap(lk, 1)) __wait(lk, lk+1, 1, 1); + if (libc.threads_minus_1) + while(a_swap(lk, 1)) __wait(lk, lk+1, 1, 1); } static inline void unlock(volatile int *lk) { - if (!libc.threads_minus_1) return; - a_store(lk, 0); - if (lk[1]) __wake(lk, 1, 1); + if (lk[0]) { + a_store(lk, 0); + if (lk[1]) __wake(lk, 1, 1); + } } static inline void lock_bin(int i) { - if (libc.threads_minus_1) - lock(mal.bins[i].lock); + lock(mal.bins[i].lock); if (!mal.bins[i].head) mal.bins[i].head = mal.bins[i].tail = BIN_TO_CHUNK(i); } static inline void unlock_bin(int i) { - if (!libc.threads_minus_1) return; unlock(mal.bins[i].lock); } diff --git a/src/thread/__lock.c b/src/thread/__lock.c index 2f345ae7..0874c04a 100644 --- a/src/thread/__lock.c +++ b/src/thread/__lock.c @@ -2,11 +2,14 @@ void __lock(volatile int *l) { - while (a_swap(l, 1)) __wait(l, l+1, 1, 1); + if (libc.threads_minus_1) + while (a_swap(l, 1)) __wait(l, l+1, 1, 1); } void __unlock(volatile int *l) { - a_store(l, 0); - if (l[1]) __wake(l, 1, 1); + if (l[0]) { + a_store(l, 0); + if (l[1]) __wake(l, 1, 1); + } } |