Don't poison the Mutex when panicking inside Condvar::wait

Author: Jethro Beekman

src/libstd/sync/condvar.rs

@@ -1,7 +1,7 @@
 use fmt;
 use sync::atomic::{AtomicUsize, Ordering};
-use sync::{mutex, MutexGuard, PoisonError};
-use sys_common::condvar as sys;
+use sync::{MutexGuard, PoisonError};
+use sys_common::{condvar as sys, AsInner};
 use sys_common::mutex as sys_mutex;
 use sys_common::poison::{self, LockResult};
 use time::{Duration, Instant};
@@ -198,16 +198,11 @@ impl Condvar {
     #[stable(feature = "rust1", since = "1.0.0")]
     pub fn wait<'a, T>(&self, guard: MutexGuard<'a, T>)
                        -> LockResult<MutexGuard<'a, T>> {
-        let poisoned = unsafe {
-            let lock = mutex::guard_lock(&guard);
-            self.verify(lock);
-            self.inner.wait(lock);
-            mutex::guard_poison(&guard).get()
-        };
-        if poisoned {
-            Err(PoisonError::new(guard))
-        } else {
-            Ok(guard)
+        unsafe {
+            let lock = MutexGuard::into_mutex(guard);
+            self.verify(lock.as_inner());
+            self.inner.wait(lock.as_inner());
+            MutexGuard::new(lock)
         }
     }
 
@@ -399,16 +394,15 @@ impl Condvar {
     pub fn wait_timeout<'a, T>(&self, guard: MutexGuard<'a, T>,
                                dur: Duration)
                                -> LockResult<(MutexGuard<'a, T>, WaitTimeoutResult)> {
-        let (poisoned, result) = unsafe {
-            let lock = mutex::guard_lock(&guard);
-            self.verify(lock);
-            let success = self.inner.wait_timeout(lock, dur);
-            (mutex::guard_poison(&guard).get(), WaitTimeoutResult(!success))
+        let (guard, result) = unsafe {
+            let lock = MutexGuard::into_mutex(guard);
+            self.verify(lock.as_inner());
+            let success = self.inner.wait_timeout(lock.as_inner(), dur);
+            (MutexGuard::new(lock), WaitTimeoutResult(!success))
         };
-        if poisoned {
-            Err(PoisonError::new((guard, result)))
-        } else {
-            Ok((guard, result))
+        match guard {
+            Ok(g) => Ok((g, result)),
+            Err(p) => Err(PoisonError::new((p.into_inner(), result)))
         }
     }
 
@@ -568,7 +562,10 @@ impl Condvar {
         unsafe { self.inner.notify_all() }
     }
 
-    fn verify(&self, mutex: &sys_mutex::Mutex) {
+    /// # Safety
+    ///
+    /// The mutex must be locked when passed to this function.
+    unsafe fn verify(&self, mutex: &sys_mutex::Mutex) {
         let addr = mutex as *const _ as usize;
         match self.mutex.compare_and_swap(0, addr, Ordering::SeqCst) {
             // If we got out 0, then we have successfully bound the mutex to
@@ -581,8 +578,12 @@ impl Condvar {
 
             // Anything else and we're using more than one mutex on this cvar,
             // which is currently disallowed.
-            _ => panic!("attempted to use a condition variable with two \
-                         mutexes"),
+            _ => {
+                // unlock the mutex before panicking
+                mutex.raw_unlock();
+                panic!("attempted to use a condition variable with two \
+                         mutexes")
+            }
         }
     }
 }

src/libstd/sync/mutex.rs

@@ -3,7 +3,7 @@ use fmt;
 use mem;
 use ops::{Deref, DerefMut};
 use ptr;
-use sys_common::mutex as sys;
+use sys_common::{mutex as sys, AsInner};
 use sys_common::poison::{self, TryLockError, TryLockResult, LockResult};
 
 /// A mutual exclusion primitive useful for protecting shared data
@@ -360,6 +360,12 @@ impl<T: ?Sized> Mutex<T> {
     }
 }
 
+impl<T: ?Sized> AsInner<sys::Mutex> for Mutex<T> {
+    fn as_inner(&self) -> &sys::Mutex {
+        &self.inner
+    }
+}
+
 #[stable(feature = "rust1", since = "1.0.0")]
 unsafe impl<#[may_dangle] T: ?Sized> Drop for Mutex<T> {
     fn drop(&mut self) {
@@ -410,14 +416,25 @@ impl<T: ?Sized + fmt::Debug> fmt::Debug for Mutex<T> {
 }
 
 impl<'mutex, T: ?Sized> MutexGuard<'mutex, T> {
-    unsafe fn new(lock: &'mutex Mutex<T>) -> LockResult<MutexGuard<'mutex, T>> {
+    /// # Safety
+    ///
+    /// The mutex must be locked when passed to this function.
+    pub(super) unsafe fn new(lock: &'mutex Mutex<T>) -> LockResult<MutexGuard<'mutex, T>> {
         poison::map_result(lock.poison.borrow(), |guard| {
             MutexGuard {
                 __lock: lock,
                 __poison: guard,
             }
         })
     }
+
+    /// Removes the poison guard, but keeps the mutex locked.
+    pub(super) fn into_mutex(guard: MutexGuard<'mutex, T>) -> &'mutex Mutex<T> {
+        guard.__lock.poison.done(&guard.__poison);
+        let lock = guard.__lock;
+        mem::forget(guard);
+        lock
+    }
 }
 
 #[stable(feature = "rust1", since = "1.0.0")]
@@ -461,14 +478,6 @@ impl<'a, T: ?Sized + fmt::Display> fmt::Display for MutexGuard<'a, T> {
     }
 }
 
-pub fn guard_lock<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a sys::Mutex {
-    &guard.__lock.inner
-}
-
-pub fn guard_poison<'a, T: ?Sized>(guard: &MutexGuard<'a, T>) -> &'a poison::Flag {
-    &guard.__lock.poison
-}
-
 #[cfg(all(test, not(target_os = "emscripten")))]
 mod tests {
     use sync::mpsc::channel;

src/test/run-pass/condvar-wait-panic-poison.rs

@@ -0,0 +1,39 @@
+// Test that panicking inside `Condvar::wait` doesn't poison the `Mutex`.
+//
+// Various platforms may trigger a panic while a thread is blocked, due to an
+// error condition. It can be tricky to trigger such a panic. The test here
+// shims `pthread_cond_timedwait` on Unix-like systems to trigger an assertion.
+// If at some point in the future, the assertion is changed or removed so that
+// the panic no longer happens, that doesn't mean this test should be removed.
+// Instead, another way should be found to trigger a panic inside
+// `Condvar::wait`.
+
+// only-unix
+
+#![feature(rustc_private)]
+
+extern crate libc;
+
+#[no_mangle]
+pub unsafe extern "C" fn pthread_cond_timedwait(
+    _cond: *mut libc::pthread_cond_t,
+    _mutex: *mut libc::pthread_mutex_t,
+    _abstime: *const libc::timespec
+) -> libc::c_int {
+    // Linux `man pthread_cond_timedwait` says EINTR may be returned
+    *libc::__errno_location() = libc::EINTR;
+    return 1;
+}
+
+use std::sync::{Condvar, Mutex};
+
+fn main() {
+    let m = Mutex::new(());
+
+    std::panic::catch_unwind(|| {
+        let one_ms = std::time::Duration::from_millis(2000);
+        Condvar::new().wait_timeout(m.lock().unwrap(), one_ms).unwrap();
+    }).expect_err("Condvar::wait should panic");
+
+    let _ = m.lock().expect("Mutex mustn't be poisoned");
+}