From 908017f9532c72c0e2318ae8c32ce521f55a510c Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sat, 25 Jun 2022 22:55:31 +0200 Subject: [PATCH 1/3] Fix data race in `thread::scope()` See # 98498 for more info --- library/std/src/thread/scoped.rs | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/library/std/src/thread/scoped.rs b/library/std/src/thread/scoped.rs index a387a09dc8b15..9620e4e2ba402 100644 --- a/library/std/src/thread/scoped.rs +++ b/library/std/src/thread/scoped.rs @@ -35,10 +35,13 @@ pub struct Scope<'scope, 'env: 'scope> { #[stable(feature = "scoped_threads", since = "1.63.0")] pub struct ScopedJoinHandle<'scope, T>(JoinInner<'scope, T>); +// Note: all of `ScopeData` fields must be interiorly mutable since +// it may be deallocated in the middle of a `&self` method: +// see `decrement_num_running_threads` below for more info. pub(super) struct ScopeData { num_running_threads: AtomicUsize, a_thread_panicked: AtomicBool, - main_thread: Thread, + main_thread: UnsafeCell, } impl ScopeData { @@ -51,12 +54,24 @@ impl ScopeData { panic!("too many running threads in thread scope"); } } + fn main_thread(&self) -> &Thread { + unsafe { &*self.main_thread.get() } + } pub(super) fn decrement_num_running_threads(&self, panic: bool) { if panic { self.a_thread_panicked.store(true, Ordering::Relaxed); } + let main_thread = self.main_thread().clone(); if self.num_running_threads.fetch_sub(1, Ordering::Release) == 1 { - self.main_thread.unpark(); + // By now, `num_running_threads` is `0`, so when `scope()` in the main thread + // is unparked, it will complete its business and deallocate `*self`! + // Two things to look after: + // - it can spuriously unpark / wake up, **so `self` can no longer be used**, even + // before we, ourselves, unpark. Hence why we've cloned the `main_thread`'s handle. + // - no matter how it unparks, `*self` may be deallocated before this function + // returns, so all of `*self` data, **including `main_thread`**, must be interiorly + // mutable. See https://github.com/rust-lang/rust/issues/55005 for more info. + main_thread.unpark(); } } } @@ -133,7 +148,7 @@ where let scope = Scope { data: ScopeData { num_running_threads: AtomicUsize::new(0), - main_thread: current(), + main_thread: current().into(), a_thread_panicked: AtomicBool::new(false), }, env: PhantomData, @@ -328,7 +343,7 @@ impl fmt::Debug for Scope<'_, '_> { f.debug_struct("Scope") .field("num_running_threads", &self.data.num_running_threads.load(Ordering::Relaxed)) .field("a_thread_panicked", &self.data.a_thread_panicked.load(Ordering::Relaxed)) - .field("main_thread", &self.data.main_thread) + .field("main_thread", self.data.main_thread()) .finish_non_exhaustive() } } From 461facc51ea093b3761e52bc3f404eb22f0799e3 Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sun, 26 Jun 2022 14:42:39 +0200 Subject: [PATCH 2/3] fixup! Fix data race in `thread::scope()` --- library/std/src/lib.rs | 1 + library/std/src/thread/scoped.rs | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/library/std/src/lib.rs b/library/std/src/lib.rs index 7da9f248c877a..d7c949498f6c2 100644 --- a/library/std/src/lib.rs +++ b/library/std/src/lib.rs @@ -291,6 +291,7 @@ #![feature(std_internals)] #![feature(str_internals)] #![feature(strict_provenance)] +#![feature(sync_unsafe_cell)] // // Library features (alloc): #![feature(alloc_layout_extra)] diff --git a/library/std/src/thread/scoped.rs b/library/std/src/thread/scoped.rs index 9620e4e2ba402..664778e734570 100644 --- a/library/std/src/thread/scoped.rs +++ b/library/std/src/thread/scoped.rs @@ -41,7 +41,10 @@ pub struct ScopedJoinHandle<'scope, T>(JoinInner<'scope, T>); pub(super) struct ScopeData { num_running_threads: AtomicUsize, a_thread_panicked: AtomicBool, - main_thread: UnsafeCell, + // SAFETY: `ScopeData` is `&`-shared by all the scoped threads, with no + // mutation whatsoever, except when `num_running_threads` drops down to `0`, + // which is when the main thread's `scope()` may deallocate this data. + main_thread: crate::cell::SyncUnsafeCell, } impl ScopeData { From 838e4d82493935fc16399b8497fe3bb08b88e9df Mon Sep 17 00:00:00 2001 From: Daniel Henry-Mantilla Date: Sun, 26 Jun 2022 14:43:05 +0200 Subject: [PATCH 3/3] Code review nits Co-Authored-By: Ralf Jung --- library/std/src/thread/scoped.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/std/src/thread/scoped.rs b/library/std/src/thread/scoped.rs index 664778e734570..2c937c0c3b9b2 100644 --- a/library/std/src/thread/scoped.rs +++ b/library/std/src/thread/scoped.rs @@ -72,8 +72,8 @@ impl ScopeData { // - it can spuriously unpark / wake up, **so `self` can no longer be used**, even // before we, ourselves, unpark. Hence why we've cloned the `main_thread`'s handle. // - no matter how it unparks, `*self` may be deallocated before this function - // returns, so all of `*self` data, **including `main_thread`**, must be interiorly - // mutable. See https://github.com/rust-lang/rust/issues/55005 for more info. + // returns, so all of `*self` fields, *including `main_thread`*, must be interiorly + // mutable. See https://github.com/rust-lang/rust/pull/98017 for more info. main_thread.unpark(); } }