diff --git a/src/lazy_static.rs b/src/lazy_static.rs index bfc7353b..9bb7fc30 100644 --- a/src/lazy_static.rs +++ b/src/lazy_static.rs @@ -1,4 +1,13 @@ //! Mock implementation of the `lazy_static` crate. +//! +//! Note: unlike the [semantics] of the `lazy_static` crate, this +//! mock implementation *will* drop its value when the program finishes. +//! This is due to an implementation detail in `loom`: it will create +//! many instances of the same program and this would otherwise lead +//! to unbounded memory leaks due to instantiating the lazy static +//! many times over. +//! +//! [semantics]: https://docs.rs/lazy_static/latest/lazy_static/#semantics use crate::rt; pub use crate::rt::thread::AccessError; @@ -12,6 +21,15 @@ use std::fmt; use std::marker::PhantomData; /// Mock implementation of `lazy_static::Lazy`. +/// +/// Note: unlike the [semantics] of the `lazy_static` crate, this +/// mock implementation *will* drop its value when the program finishes. +/// This is due to an implementation detail in `loom`: it will create +/// many instances of the same program and this would otherwise lead +/// to unbounded memory leaks due to instantiating the lazy static +/// many times over. +/// +/// [semantics]: https://docs.rs/lazy_static/latest/lazy_static/#semantics pub struct Lazy { // Sadly, these fields have to be public, since function pointers in const // fns are unstable. When fn pointer arguments to const fns stabilize, these diff --git a/src/model.rs b/src/model.rs index c7169c38..905d6421 100644 --- a/src/model.rs +++ b/src/model.rs @@ -182,14 +182,37 @@ impl Builder { let f = f.clone(); scheduler.run(&mut execution, move || { - f(); + /// the given closure `f` may panic when executed. + /// when this happens, we still want to ensure + /// that thread locals are destructed before the + /// global statics are dropped. therefore, we set + /// up a guard that is dropped as part of the unwind + /// logic when `f` panics. this guard in turn ensures, + /// through the implementation of `rt::thread_done`, + /// that thread local fields are dropped before the + /// global statics are dropped. without this guard, + /// a `Drop` implementation on a type that is stored + /// in a thread local field could access the lazy static, + /// and this then would in turn panic from the method + /// [`Lazy::get`](crate::lazy_static::Lazy), which + /// causes a panic inside a panic, which in turn causes + /// Rust to abort, triggering a segfault in `loom`. + struct PanicGuard; + impl Drop for PanicGuard { + fn drop(&mut self) { + rt::thread_done(true); + } + } - let lazy_statics = rt::execution(|execution| execution.lazy_statics.drop()); + // set up the panic guard + let panic_guard = PanicGuard; - // drop outside of execution - drop(lazy_statics); + // execute the closure, note that `f()` may panic! + f(); - rt::thread_done(); + // if `f()` didn't panic, then we terminate the + // main thread by dropping the guard ourselves. + drop(panic_guard); }); execution.check_for_leaks(); diff --git a/src/rt/execution.rs b/src/rt/execution.rs index dd9c28fd..c472d44e 100644 --- a/src/rt/execution.rs +++ b/src/rt/execution.rs @@ -207,13 +207,13 @@ impl Execution { self.threads.set_active(next); - // There is no active thread. Unless all threads have terminated, the - // test has deadlocked. + // There is no active thread. Unless all threads have terminated + // or the current thread is panicking, the test has deadlocked. if !self.threads.is_active() { let terminal = self.threads.iter().all(|(_, th)| th.is_terminated()); assert!( - terminal, + terminal || std::thread::panicking(), "deadlock; threads = {:?}", self.threads .iter() diff --git a/src/rt/mod.rs b/src/rt/mod.rs index 6deda8b4..c3752595 100644 --- a/src/rt/mod.rs +++ b/src/rt/mod.rs @@ -73,8 +73,27 @@ where trace!(thread = ?id, "spawn"); Scheduler::spawn(Box::new(move || { + /// the given closure `f` may panic when executed. + /// when this happens, we still want to ensure that + /// thread locals are destructed. therefore, we set + /// up a guard that is dropped as part of the unwind + /// logic when `f` panics. + struct PanicGuard; + impl Drop for PanicGuard { + fn drop(&mut self) { + thread_done(false); + } + } + + // set up the panic guard + let panic_guard = PanicGuard; + + // execute the closure, note that `f()` may panic! f(); - thread_done(); + + // if `f()` didn't panic, then we terminate the + // spawned thread by dropping the guard ourselves. + drop(panic_guard); })); id @@ -171,7 +190,22 @@ where Scheduler::with_execution(f) } -pub fn thread_done() { +pub fn thread_done(is_main_thread: bool) { + let is_active = execution(|execution| execution.threads.is_active()); + if !is_active { + // if the thread is not active and the current thread is panicking, + // then this means that loom has detected a problem (e.g. a deadlock). + // we don't want to throw a double panic here, because this would cause + // the entire test to abort and this hides the error from the end user. + // instead we ensure that the current thread is panicking already, + // or we cause a panic if it's not yet panicking (which it otherwise + // would anyway, on the call to `execution.threads.active_id()` below). + let panicking = std::thread::panicking(); + trace!(?panicking, "thread_done: no active thread"); + assert!(panicking); + return; + } + let locals = execution(|execution| { let thread = execution.threads.active_id(); @@ -180,9 +214,28 @@ pub fn thread_done() { execution.threads.active_mut().drop_locals() }); - // Drop outside of the execution context + // Drop locals outside of the execution context drop(locals); + if is_main_thread { + // Note that the statics must be dropped AFTER + // dropping the thread local fields. The `Drop` + // implementation of a type stored in a thread + // local field may still try to access the global + // statics on drop, so we need to take extra care + // that the global statics outlive the thread locals. + let statics = execution(|execution| { + let thread = execution.threads.active_id(); + + trace!(?thread, "thread_done: drop statics from the main thread"); + + execution.lazy_statics.drop() + }); + + // Drop statics outside of the execution context + drop(statics); + } + execution(|execution| { let thread = execution.threads.active_id(); diff --git a/tests/deadlock.rs b/tests/deadlock.rs index 52fa71a9..26657c29 100644 --- a/tests/deadlock.rs +++ b/tests/deadlock.rs @@ -6,7 +6,7 @@ use loom::thread; use std::rc::Rc; #[test] -#[should_panic] +#[should_panic(expected = "deadlock; threads =")] fn two_mutexes_deadlock() { loom::model(|| { let a = Rc::new(Mutex::new(1)); diff --git a/tests/thread_local.rs b/tests/thread_local.rs index 3d109b28..daf10f30 100644 --- a/tests/thread_local.rs +++ b/tests/thread_local.rs @@ -107,3 +107,132 @@ fn drop() { // should also be dropped. assert_eq!(DROPS.load(Ordering::Acquire), 3); } + +/// Test that TLS destructors are allowed to access global statics +/// when the TLS type is dropped. +/// +/// This is a regression test for: +/// +#[test] +fn lazy_static() { + loom::lazy_static! { + static ref ID: usize = 0x42; + } + + loom::thread_local! { + static BAR: Bar = Bar; + } + + struct Bar; + + impl Drop for Bar { + fn drop(&mut self) { + let _ = &*ID; + } + } + + loom::model(|| { + BAR.with(|_| ()); + }); +} + +/// When a thread panics it runs the TLS destructors, which +/// in turn may try to access a global static. If the drop +/// order of TLS fields and global statics is switched, then +/// this will trigger a panic from the TLS destructor, too. +/// This causes a panic inside another panic, which will lead +/// to loom triggering a segfault. This should not happen, +/// because it should be allowed for TLS destructors to always +/// access a global static. +/// +/// This is a regression test for a slight varation of +/// . +#[test] +#[should_panic(expected = "loom should not panic inside another panic")] +fn lazy_static_panic() { + loom::lazy_static! { + static ref ID: usize = 0x42; + } + + loom::thread_local! { + static BAR: Bar = Bar; + } + + struct Bar; + + impl Drop for Bar { + fn drop(&mut self) { + assert!(BAR.try_with(|_| ()).is_err()); + let _ = &*ID; + } + } + + loom::model(|| { + // initialize the TLS destructor: + BAR.with(|_| ()); + // intentionally panic: + panic!("loom should not panic inside another panic"); + }); +} + +/// Test that thread locals are properly destructed +/// when a spawned thread panics, without causing +/// a double panic. +#[test] +#[should_panic(expected = "loom should not panic inside another panic")] +fn access_on_drop_during_panic_in_spawned_thread() { + use loom::{cell::Cell, thread}; + use std::{ + panic::catch_unwind, + sync::{Mutex, MutexGuard, PoisonError}, + }; + + struct DropCounter { + instantiated: usize, + dropped: usize, + } + + static DROPPED: Mutex = Mutex::new(DropCounter { + instantiated: 0, + dropped: 0, + }); + + loom::thread_local! { + static BAR: Cell = Cell::new(Bar({ + let mut guard = DROPPED.lock().unwrap(); + guard.instantiated += 1; + guard + })); + } + + struct Bar(MutexGuard<'static, DropCounter>); + impl Drop for Bar { + fn drop(&mut self) { + assert!(BAR.try_with(|_| ()).is_err()); + self.0.dropped += 1; + } + } + + let result = catch_unwind(|| { + loom::model(|| { + thread::spawn(|| { + // initialize the TLS destructor and panic: + BAR.with(|_| { + BAR.with(|_| { + panic!(); + }); + }); + }) + .join() + .unwrap(); + }); + }); + + let guard = DROPPED.lock().unwrap_or_else(PoisonError::into_inner); + assert_eq!(guard.instantiated, 1); + assert_eq!(guard.dropped, 1); + + // propagate the panic from the spawned thread + // to the main thread. + result.expect("loom should not panic inside another panic"); +}