From 61fdd3e2bee1ea0f517ff0bfe16f3816decab99a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 May 2020 19:17:58 +0200 Subject: [PATCH 1/4] expand comment on default mutex behavior --- src/libstd/sys/unix/mutex.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/libstd/sys/unix/mutex.rs b/src/libstd/sys/unix/mutex.rs index 103d87e3d2f91..185615d01dd50 100644 --- a/src/libstd/sys/unix/mutex.rs +++ b/src/libstd/sys/unix/mutex.rs @@ -28,14 +28,18 @@ impl Mutex { // // A pthread mutex initialized with PTHREAD_MUTEX_INITIALIZER will have // a type of PTHREAD_MUTEX_DEFAULT, which has undefined behavior if you - // try to re-lock it from the same thread when you already hold a lock. + // try to re-lock it from the same thread when you already hold a lock + // (https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_init.html). // // In practice, glibc takes advantage of this undefined behavior to // implement hardware lock elision, which uses hardware transactional - // memory to avoid acquiring the lock. While a transaction is in + // memory to avoid acquiring the lock. + // This is the case even if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL + // (https://github.com/rust-lang/rust/issues/33770#issuecomment-220847521). + // As a consequence, while a transaction is in // progress, the lock appears to be unlocked. This isn't a problem for // other threads since the transactional memory will abort if a conflict - // is detected, however no abort is generated if re-locking from the + // is detected, however no abort is generated when re-locking from the // same thread. // // Since locking the same mutex twice will result in two aliasing &mut From 40a6b8c339376b7879016d5d2a015b6ac43c4c0c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 May 2020 19:37:55 +0200 Subject: [PATCH 2/4] explain our rwlock implementation (and fix a potential data race) --- src/libstd/sys/unix/rwlock.rs | 44 +++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/src/libstd/sys/unix/rwlock.rs b/src/libstd/sys/unix/rwlock.rs index 079dea671ef76..3e3a01b4ea395 100644 --- a/src/libstd/sys/unix/rwlock.rs +++ b/src/libstd/sys/unix/rwlock.rs @@ -22,27 +22,26 @@ impl RWLock { pub unsafe fn read(&self) { let r = libc::pthread_rwlock_rdlock(self.inner.get()); - // According to the pthread_rwlock_rdlock spec, this function **may** - // fail with EDEADLK if a deadlock is detected. On the other hand - // pthread mutexes will *never* return EDEADLK if they are initialized - // as the "fast" kind (which ours always are). As a result, a deadlock - // situation may actually return from the call to pthread_rwlock_rdlock - // instead of blocking forever (as mutexes and Windows rwlocks do). Note - // that not all unix implementations, however, will return EDEADLK for - // their rwlocks. + // According to POSIX, when a thread tries to acquire this read lock + // while it already holds the write lock + // (or vice versa, or tries to acquire the write lock twice), + // "the call shall either deadlock or return [EDEADLK]" + // (https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_wrlock.html, + // https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_rwlock_rdlock.html). + // So, in principle, all we have to do here is check `r == 0` to be sure we properly + // got the lock. // - // We roughly maintain the deadlocking behavior by panicking to ensure - // that this lock acquisition does not succeed. - // - // We also check whether this lock is already write locked. This - // is only possible if it was write locked by the current thread and - // the implementation allows recursive locking. The POSIX standard - // doesn't require recursively locking a rwlock to deadlock, but we can't - // allow that because it could lead to aliasing issues. + // However, (at least) glibc before version 2.25 does not conform to this spec, + // and can return `r == 0` even when this thread already holds the write lock. + // We thus check for this situation ourselves and panic when detecting that a thread + // got the write lock more than once, or got a read and a write lock. if r == libc::EAGAIN { panic!("rwlock maximum reader count exceeded"); } else if r == libc::EDEADLK || (r == 0 && *self.write_locked.get()) { + // Above, we make sure to only access `write_locked` when `r == 0` to avoid + // data races. if r == 0 { + // `pthread_rwlock_rdlock` succeeded when it should not have. self.raw_unlock(); } panic!("rwlock read lock would result in deadlock"); @@ -56,6 +55,7 @@ impl RWLock { let r = libc::pthread_rwlock_tryrdlock(self.inner.get()); if r == 0 { if *self.write_locked.get() { + // `pthread_rwlock_tryrdlock` succeeded when it should not have. self.raw_unlock(); false } else { @@ -69,18 +69,21 @@ impl RWLock { #[inline] pub unsafe fn write(&self) { let r = libc::pthread_rwlock_wrlock(self.inner.get()); - // See comments above for why we check for EDEADLK and write_locked. We - // also need to check that num_readers is 0. + // See comments above for why we check for EDEADLK and write_locked. For the same reason, + // we also need to check that there are no readers (tracked in `num_readers`). if r == libc::EDEADLK - || *self.write_locked.get() + || (r == 0 && *self.write_locked.get()) || self.num_readers.load(Ordering::Relaxed) != 0 { + // Above, we make sure to only access `write_locked` when `r == 0` to avoid + // data races. if r == 0 { + // `pthread_rwlock_wrlock` succeeded when it should not have. self.raw_unlock(); } panic!("rwlock write lock would result in deadlock"); } else { - debug_assert_eq!(r, 0); + assert_eq!(r, 0); } *self.write_locked.get() = true; } @@ -89,6 +92,7 @@ impl RWLock { let r = libc::pthread_rwlock_trywrlock(self.inner.get()); if r == 0 { if *self.write_locked.get() || self.num_readers.load(Ordering::Relaxed) != 0 { + // `pthread_rwlock_trywrlock` succeeded when it should not have. self.raw_unlock(); false } else { From 3f50292edce92fb889bcd7318fead1a8905b6342 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 4 May 2020 20:47:46 +0200 Subject: [PATCH 3/4] edit Mutex comment --- src/libstd/sys/unix/mutex.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/libstd/sys/unix/mutex.rs b/src/libstd/sys/unix/mutex.rs index 185615d01dd50..45c600f75f5cf 100644 --- a/src/libstd/sys/unix/mutex.rs +++ b/src/libstd/sys/unix/mutex.rs @@ -30,13 +30,15 @@ impl Mutex { // a type of PTHREAD_MUTEX_DEFAULT, which has undefined behavior if you // try to re-lock it from the same thread when you already hold a lock // (https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_mutex_init.html). + // This is the case even if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL + // (https://github.com/rust-lang/rust/issues/33770#issuecomment-220847521) -- in that + // case, `pthread_mutexattr_settype(PTHREAD_MUTEX_DEFAULT)` will of course be the same + // as setting it to `PTHREAD_MUTEX_NORMAL`, but not setting any mode will result in + // a Mutex where re-locking is UB. // // In practice, glibc takes advantage of this undefined behavior to // implement hardware lock elision, which uses hardware transactional - // memory to avoid acquiring the lock. - // This is the case even if PTHREAD_MUTEX_DEFAULT == PTHREAD_MUTEX_NORMAL - // (https://github.com/rust-lang/rust/issues/33770#issuecomment-220847521). - // As a consequence, while a transaction is in + // memory to avoid acquiring the lock. While a transaction is in // progress, the lock appears to be unlocked. This isn't a problem for // other threads since the transactional memory will abort if a conflict // is detected, however no abort is generated when re-locking from the From f9866f95afad4ea97b975631c6016aa951fd4f83 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 5 May 2020 09:08:00 +0200 Subject: [PATCH 4/4] rely on rdlock/wrlock not returning anything but the specified error codes --- src/libstd/sys/unix/rwlock.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libstd/sys/unix/rwlock.rs b/src/libstd/sys/unix/rwlock.rs index 3e3a01b4ea395..2b5067a34f648 100644 --- a/src/libstd/sys/unix/rwlock.rs +++ b/src/libstd/sys/unix/rwlock.rs @@ -46,7 +46,9 @@ impl RWLock { } panic!("rwlock read lock would result in deadlock"); } else { - assert_eq!(r, 0); + // According to POSIX, for a properly initialized rwlock this can only + // return EAGAIN or EDEADLK or 0. We rely on that. + debug_assert_eq!(r, 0); self.num_readers.fetch_add(1, Ordering::Relaxed); } } @@ -83,7 +85,9 @@ impl RWLock { } panic!("rwlock write lock would result in deadlock"); } else { - assert_eq!(r, 0); + // According to POSIX, for a properly initialized rwlock this can only + // return EDEADLK or 0. We rely on that. + debug_assert_eq!(r, 0); } *self.write_locked.get() = true; }