Skip to content

Commit

Permalink
Avoid double panic when queued_spawn is dropped
Browse files Browse the repository at this point in the history
  • Loading branch information
adrianheine committed Mar 10, 2023
1 parent 101c335 commit 3807110
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 1 deletion.
36 changes: 35 additions & 1 deletion src/rt/scheduler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}

Expand Down
23 changes: 23 additions & 0 deletions tests/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}

0 comments on commit 3807110

Please sign in to comment.