Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data race in thread::scope #98498

Closed
RalfJung opened this issue Jun 25, 2022 · 4 comments · Fixed by #98503
Closed

Data race in thread::scope #98498

RalfJung opened this issue Jun 25, 2022 · 4 comments · Fixed by #98503
Milestone

Comments

@RalfJung
Copy link
Member

The following code sometimes causes a data race to be reported by Miri:

#![feature(atomic_from_mut, inline_const)]
use std::sync::atomic::{AtomicBool, Ordering};

fn main() {
let mut some_bools = [const { AtomicBool::new(false) }; 10];

let view: &mut [bool] = AtomicBool::get_mut_slice(&mut some_bools);
assert_eq!(view, [false; 10]);
view[..5].copy_from_slice(&[true; 5]);

std::thread::scope(|s| {
    for t in &some_bools[..5] {
        s.spawn(move || assert_eq!(t.load(Ordering::Relaxed), true));
    }

    for f in &some_bools[5..] {
        s.spawn(move || assert_eq!(f.load(Ordering::Relaxed), false));
    }
});
}

Miri reports:

note: tracking was triggered
   --> /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/scoped.rs:133:17
    |
133 |       let scope = Scope {
    |  _________________^
134 | |         data: ScopeData {
135 | |             num_running_threads: AtomicUsize::new(0),
136 | |             main_thread: current(),
...   |
140 | |         scope: PhantomData,
141 | |     };
    | |_____^ created allocation with id 1082
    |
    = note: inside `std::thread::scope::<[closure@atomic.rs:12:20: 20:2], ()>` at /home/r/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/thread/scoped.rs:133:17
note: inside `main` at atomic.rs:12:1
   --> atomic.rs:12:1
    |
12  | / std::thread::scope(|s| {
13  | |     for t in &some_bools[..5] {
14  | |         s.spawn(move || assert_eq!(t.load(Ordering::Relaxed), true));
15  | |     }
...   |
19  | |     }
20  | | });
    | |__^

error: Undefined Behavior: Data race detected between Deallocate on Thread(id = 0, name = "main") and Read on Thread(id = 2) at alloc1082+0x8 (current vector clock = VClock([114, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8]), conflicting timestamp = VClock([113, 0, 9, 9, 9, 0, 0, 0, 0, 0, 9]))
  --> atomic.rs:12:1
   |
12 | / std::thread::scope(|s| {
13 | |     for t in &some_bools[..5] {
14 | |         s.spawn(move || assert_eq!(t.load(Ordering::Relaxed), true));
15 | |     }
...  |
19 | |     }
20 | | });
   | |__^ Data race detected between Deallocate on Thread(id = 0, name = "main") and Read on Thread(id = 2) at alloc1082+0x8 (current vector clock = VClock([114, 8, 8, 8, 8, 8, 8, 8, 8, 8, 8]), conflicting timestamp = VClock([113, 0, 9, 9, 9, 0, 0, 0, 0, 0, 9]))
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

   = note: inside `main` at atomic.rs:12:1

The deallocation occurs when scope returns and its local variable scope gets deallocated.

The read occurs here (I am fairly sure):

if self.num_running_threads.fetch_sub(1, Ordering::Release) == 1 {
self.main_thread.unpark();

This reads the contents of the field self.main_thread after the fetch_sub. But the moment the fetch_sub is done, the memory backing self might be deallocated. That makes the read a potential use-after-free, and a race with the deallocation.

Cc @m-ou-se

@RalfJung
Copy link
Member Author

The following patch fixes this:

diff --git a/library/std/src/thread/scoped.rs b/library/std/src/thread/scoped.rs
index a387a09dc8b..06019cbc75b 100644
--- a/library/std/src/thread/scoped.rs
+++ b/library/std/src/thread/scoped.rs
@@ -55,8 +55,10 @@ pub(super) fn decrement_num_running_threads(&self, panic: bool) {
         if panic {
             self.a_thread_panicked.store(true, Ordering::Relaxed);
         }
+        // The moment we do the `fetch_sub`, the memory we ourselves are stored in can be deallocated.
+        let main_thread = self.main_thread.clone();
         if self.num_running_threads.fetch_sub(1, Ordering::Release) == 1 {
-            self.main_thread.unpark();
+            main_thread.unpark();
         }
     }
 }

However, even with this patch, we still run into a problem like #55005: the &self gets deallocated while decrement_num_running_threads still runs. This is still UB even after my proposed spec change because self contains a field that is not inside an UnsafeCell (the main_thread field). So that part of what &self points to is expected to stay live for the entire duration of the function call.

Another alternative might be to put the entire Scope into an Arc rather than allocating it on the stack frame of scope.

@Mark-Simulacrum
Copy link
Member

Going to attach this to the 1.63 milestone, since this is in a newly stabilized feature -- we may wish to back out stabilization for a cycle or backport fixes.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Jun 29, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? `@m-ou-se`
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Jun 30, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? ``@m-ou-se``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 30, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? ```@m-ou-se```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 30, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? ````@m-ou-se````
@bors bors closed this as completed in ebecc13 Jul 1, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jul 1, 2022

Re-opening for beta backport.

@m-ou-se m-ou-se reopened this Jul 1, 2022
ehuss pushed a commit to ehuss/rust that referenced this issue Jul 5, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang#98498
r? `````@m-ou-se`````
@cuviper
Copy link
Member

cuviper commented Jul 15, 2022

The fix was backported in #98949.

@cuviper cuviper closed this as completed Jul 15, 2022
workingjubilee pushed a commit to tcdi/postgrestd that referenced this issue Sep 15, 2022
fix data race in thread::scope

Puts the `ScopeData` into an `Arc` so it sticks around as long as we need it.
This means one extra `Arc::clone` per spawned scoped thread, which I hope is fine.

Fixes rust-lang/rust#98498
r? `````@m-ou-se`````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants