diff --git a/src/libstd/sync/condvar.rs b/src/libstd/sync/condvar.rs index 036aff090ead9..aaaecff59be6f 100644 --- a/src/libstd/sync/condvar.rs +++ b/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> { - 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") + } } } } diff --git a/src/libstd/sync/mutex.rs b/src/libstd/sync/mutex.rs index 340dca7ce73b8..71ed020fa869d 100644 --- a/src/libstd/sync/mutex.rs +++ b/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 Mutex { } } +impl AsInner for Mutex { + fn as_inner(&self) -> &sys::Mutex { + &self.inner + } +} + #[stable(feature = "rust1", since = "1.0.0")] unsafe impl<#[may_dangle] T: ?Sized> Drop for Mutex { fn drop(&mut self) { @@ -410,7 +416,10 @@ impl fmt::Debug for Mutex { } impl<'mutex, T: ?Sized> MutexGuard<'mutex, T> { - unsafe fn new(lock: &'mutex Mutex) -> LockResult> { + /// # Safety + /// + /// The mutex must be locked when passed to this function. + pub(super) unsafe fn new(lock: &'mutex Mutex) -> LockResult> { poison::map_result(lock.poison.borrow(), |guard| { MutexGuard { __lock: lock, @@ -418,6 +427,14 @@ impl<'mutex, T: ?Sized> MutexGuard<'mutex, T> { } }) } + + /// Removes the poison guard, but keeps the mutex locked. + pub(super) fn into_mutex(guard: MutexGuard<'mutex, T>) -> &'mutex Mutex { + 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; diff --git a/src/test/run-pass/condvar-wait-panic-poison.rs b/src/test/run-pass/condvar-wait-panic-poison.rs new file mode 100644 index 0000000000000..3d4009f28cabb --- /dev/null +++ b/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"); +}