Avoid double panic when queued_spawn is dropped

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<T> {
     // Sadly, these fields have to be public, since function pointers in const
     // fns are unstable. When fn pointer arguments to const fns stabilize, these

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();

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()

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();
 

src/rt/scheduler.rs

@@ -127,7 +127,41 @@ impl Scheduler {
             panic!("cannot access Loom execution state from outside a Loom model. \
             are you accessing a Loom synchronization primitive from outside a Loom test (a call to `model` or `check`)?")
         }
-        STATE.with(|state| f(&mut state.borrow_mut()))
+        STATE.with(|state| {
+            // When unwinding after a panic, `STATE` is unset before `Scheduler` is dropped.
+            // However, `Scheduler::queued_spawn` could hold loom objects which would try to use
+            // `STATE` when they are dropped. Because of that, we need to clear `queued_spawn`
+            // while STATE is still set. Since `STATE` has an exclusive reference (&mut) to
+            // `Scheduler::queued_spawn`, we also need to use `STATE` ourselves for accessing them,
+            // but drop our `RefMut` before the dropping of `queued_spawn` happens.
+            //
+            // To implement this, we exploit the fact that the struct fields of `DropGuard`
+            // are dropped in declaration order, and move `queued_spawn`in `DropGuard::drop`
+            // to the second struct field of `DropGuard` (replacing it with an empty `VecDeque`).
+            // Then, the destructor first drops the `RefMut` (thereby allowing `STATE` to be
+            // borrowed again), and then the former `queued_spawn` value (possibly accessing `STATE`).
+            struct DropGuard<'a, 'b>(
+                std::cell::RefMut<'a, State<'b>>,
+                VecDeque<Box<dyn FnOnce()>>,
+            );
+            impl<'a, 'b> Drop for DropGuard<'a, 'b> {
+                fn drop(&mut self) {
+                    if std::thread::panicking() {
+                        self.1 = std::mem::take(self.0.queued_spawn);
+                    }
+                }
+            }
+            impl<'a, 'b> DropGuard<'a, 'b> {
+                fn run<F, R>(&mut self, f: F) -> R
+                where
+                    F: FnOnce(&mut State<'b>) -> R,
+                {
+                    f(&mut self.0)
+                }
+            }
+            let mut guard = DropGuard(state.borrow_mut(), Default::default());
+            guard.run(f)
+        })
     }
 }
 

tests/arc.rs

@@ -128,3 +128,26 @@ fn try_unwrap_multithreaded() {
         let _ = Arc::try_unwrap(num).unwrap();
     });
 }
+
+/// Test that there is no double panic
+/// when a model with an Arc in an unspawned thread panics.
+#[test]
+#[should_panic(expected = "loom should not panic inside another panic")]
+fn access_on_drop_during_panic_in_unspawned_thread() {
+    use loom::sync::Arc;
+    use std::panic::catch_unwind;
+
+    let result = catch_unwind(|| {
+        loom::model(move || {
+            let arc = Arc::new(());
+            thread::spawn(move || {
+                let _arc = arc;
+            });
+            panic!();
+        });
+    });
+
+    // propagate the panic from the spawned thread
+    // to the main thread.
+    result.expect("loom should not panic inside another panic");
+}

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));

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:
+/// <https://github.com/tokio-rs/loom/issues/152>
+#[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
+/// <https://github.com/tokio-rs/loom/issues/152>.
+#[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<DropCounter> = Mutex::new(DropCounter {
+        instantiated: 0,
+        dropped: 0,
+    });
+
+    loom::thread_local! {
+        static BAR: Cell<Bar> = 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");
+}