explain our rwlock implementation (and fix a potential data race)

1 files changed, 24 insertions, 20 deletions

diff --git a/src/libstd/sys/unix/rwlock.rs b/src/libstd/sys/unix/rwlock.rs
index 079dea671ef..3e3a01b4ea3 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 {