Rollup merge of #94644 - m-ou-se:scoped-threads-drop-soundness, r=joshtriplett

Fix soundness issue in scoped threads.

This was discovered in https://github.com/rust-lang/rust/pull/94559#discussion_r820116323

The `scope()` function returns when all threads are finished, but I accidentally considered a thread 'finished' before dropping their panic payload or ignored return value.

So if a thread returned (or panics with) something that in its `Drop` implementation still uses borrowed stuff, it goes wrong.

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2a1f19ac4676cdabe43e24e536ff9358

2 files changed, 47 insertions, 7 deletions

diff --git a/library/std/src/thread/mod.rs b/library/std/src/thread/mod.rs
index 5ffc86b4560..74b29454b94 100644
--- a/library/std/src/thread/mod.rs
+++ b/library/std/src/thread/mod.rs
@@ -1287,12 +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(_)));
+            // 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);
         }
     }
diff --git a/library/std/src/thread/tests.rs b/library/std/src/thread/tests.rs
index 3323ba36bf3..7386fe1c442 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,5 +294,25 @@ 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.
+#[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));
+}