From 5226395d6faef77a5f1dadb6235bcd99352e1843 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 5 Mar 2022 17:19:04 +0100 Subject: [PATCH 1/4] Fix soundness issue in scoped threads. --- library/std/src/thread/mod.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 5ffc86b4560fc..56baa7e445588 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1293,6 +1293,23 @@ impl<'scope, T> Drop for Packet<'scope, T> { // panicked, and nobody consumed the panic payload, we make sure // the scope function will panic. let unhandled_panic = matches!(self.result.get_mut(), Some(Err(_))); + // Drop the result before decrementing the number of running + // threads, because the Drop implementation might still use things + // it borrowed from 'scope. + // This is only relevant for threads that aren't join()ed, as + // join() will take the `result` and set it to None, such that + // there is nothing left to drop here. + // If this drop panics, that just results in an abort, because + // we're outside of the outermost `catch_unwind` of our thread. + // The same happens for detached non-scoped threads when dropping + // their ignored return value (or panic payload) panics, so + // there's no need to try to do anything better. + // (And even if we tried to handle it, we'd also need to handle + // the case where the panic payload we get out of it also panics + // on drop, and so on. See issue #86027.) + *self.result.get_mut() = None; + // Now that there will be no more user code running on this thread + // that can use 'scope, mark the thread as 'finished'. scope.decrement_num_running_threads(unhandled_panic); } } From 7a481ff8a412121b0a52c51c086ae22ed7e96ab5 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Sat, 5 Mar 2022 18:07:20 +0100 Subject: [PATCH 2/4] Properly abort when thread result panics on drop. --- library/std/src/lib.rs | 10 ++++----- library/std/src/thread/mod.rs | 40 ++++++++++++++++++----------------- 2 files changed, 26 insertions(+), 24 deletions(-) diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index c35389d44f9fc..232b66230782f 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -363,6 +363,11 @@ extern crate std as realstd; #[macro_use] mod macros; +// The runtime entry point and a few unstable public functions used by the +// compiler +#[macro_use] +pub mod rt; + // The Rust prelude pub mod prelude; @@ -547,11 +552,6 @@ pub mod arch { #[stable(feature = "simd_x86", since = "1.27.0")] pub use std_detect::is_x86_feature_detected; -// The runtime entry point and a few unstable public functions used by the -// compiler -#[macro_use] -pub mod rt; - // Platform-abstraction modules mod sys; mod sys_common; diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs index 56baa7e445588..74b29454b94a4 100644 --- a/library/std/src/thread/mod.rs +++ b/library/std/src/thread/mod.rs @@ -1287,29 +1287,31 @@ unsafe impl<'scope, T: Sync> Sync for Packet<'scope, T> {} impl<'scope, T> Drop for Packet<'scope, T> { fn drop(&mut self) { + // If this packet was for a thread that ran in a scope, the thread + // panicked, and nobody consumed the panic payload, we make sure + // the scope function will panic. + let unhandled_panic = matches!(self.result.get_mut(), Some(Err(_))); + // Drop the result without causing unwinding. + // This is only relevant for threads that aren't join()ed, as + // join() will take the `result` and set it to None, such that + // there is nothing left to drop here. + // If this panics, we should handle that, because we're outside the + // outermost `catch_unwind` of our thread. + // We just abort in that case, since there's nothing else we can do. + // (And even if we tried to handle it somehow, we'd also need to handle + // the case where the panic payload we get out of it also panics on + // drop, and so on. See issue #86027.) + if let Err(_) = panic::catch_unwind(panic::AssertUnwindSafe(|| { + *self.result.get_mut() = None; + })) { + rtabort!("thread result panicked on drop"); + } // Book-keeping so the scope knows when it's done. if let Some(scope) = self.scope { - // If this packet was for a thread that ran in a scope, the thread - // panicked, and nobody consumed the panic payload, we make sure - // the scope function will panic. - let unhandled_panic = matches!(self.result.get_mut(), Some(Err(_))); - // Drop the result before decrementing the number of running - // threads, because the Drop implementation might still use things - // it borrowed from 'scope. - // This is only relevant for threads that aren't join()ed, as - // join() will take the `result` and set it to None, such that - // there is nothing left to drop here. - // If this drop panics, that just results in an abort, because - // we're outside of the outermost `catch_unwind` of our thread. - // The same happens for detached non-scoped threads when dropping - // their ignored return value (or panic payload) panics, so - // there's no need to try to do anything better. - // (And even if we tried to handle it, we'd also need to handle - // the case where the panic payload we get out of it also panics - // on drop, and so on. See issue #86027.) - *self.result.get_mut() = None; // Now that there will be no more user code running on this thread // that can use 'scope, mark the thread as 'finished'. + // It's important we only do this after the `result` has been dropped, + // since dropping it might still use things it borrowed from 'scope. scope.decrement_num_running_threads(unhandled_panic); } } From 1c06eb7c1f416055d7ede098f35bcf22cf85b7f8 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 9 Mar 2022 11:47:46 +0100 Subject: [PATCH 3/4] Remove outdated comment. --- library/std/src/thread/tests.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/library/std/src/thread/tests.rs b/library/std/src/thread/tests.rs index 3323ba36bf310..27eebd0ddf4ec 100644 --- a/library/std/src/thread/tests.rs +++ b/library/std/src/thread/tests.rs @@ -293,5 +293,3 @@ fn test_thread_id_not_equal() { assert!(thread::current().id() != spawned_id); } -// NOTE: the corresponding test for stderr is in ui/thread-stderr, due -// to the test harness apparently interfering with stderr configuration. From b97d87518d19e418220f726e774ffceadb4d33b9 Mon Sep 17 00:00:00 2001 From: Mara Bos Date: Wed, 9 Mar 2022 11:47:53 +0100 Subject: [PATCH 4/4] Add soundness test for dropping scoped thread results before joining. --- library/std/src/thread/tests.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/library/std/src/thread/tests.rs b/library/std/src/thread/tests.rs index 27eebd0ddf4ec..7386fe1c442ab 100644 --- a/library/std/src/thread/tests.rs +++ b/library/std/src/thread/tests.rs @@ -4,10 +4,11 @@ use crate::mem; use crate::panic::panic_any; use crate::result; use crate::sync::{ + atomic::{AtomicBool, Ordering}, mpsc::{channel, Sender}, Arc, Barrier, }; -use crate::thread::{self, ThreadId}; +use crate::thread::{self, Scope, ThreadId}; use crate::time::Duration; use crate::time::Instant; @@ -293,3 +294,25 @@ fn test_thread_id_not_equal() { assert!(thread::current().id() != spawned_id); } +#[test] +fn test_scoped_threads_drop_result_before_join() { + let actually_finished = &AtomicBool::new(false); + struct X<'scope, 'env>(&'scope Scope<'scope, 'env>, &'env AtomicBool); + impl Drop for X<'_, '_> { + fn drop(&mut self) { + thread::sleep(Duration::from_millis(20)); + let actually_finished = self.1; + self.0.spawn(move || { + thread::sleep(Duration::from_millis(20)); + actually_finished.store(true, Ordering::Relaxed); + }); + } + } + thread::scope(|s| { + s.spawn(move || { + thread::sleep(Duration::from_millis(20)); + X(s, actually_finished) + }); + }); + assert!(actually_finished.load(Ordering::Relaxed)); +}