diff --git a/src/concurrency/data_race.rs b/src/concurrency/data_race.rs index 0e176ec91fc..c1a3db67c84 100644 --- a/src/concurrency/data_race.rs +++ b/src/concurrency/data_race.rs @@ -689,6 +689,17 @@ fn atomic_fence(&mut self, atomic: AtomicFenceOrd) -> InterpResult<'tcx> { Ok(()) } } + + /// After all threads are done running, this allows data races to occur for subsequent + /// 'administrative' machine accesses (that logically happen outside of the Abstract Machine). + fn allow_data_races_all_threads_done(&mut self) { + let this = self.eval_context_ref(); + assert!(this.have_all_terminated()); + if let Some(data_race) = &this.machine.data_race { + let old = data_race.ongoing_action_data_race_free.replace(true); + assert!(!old, "cannot nest allow_data_races"); + } + } } /// Vector clock metadata for a logical memory allocation. @@ -955,8 +966,8 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> { fn allow_data_races_ref(&self, op: impl FnOnce(&MiriEvalContext<'mir, 'tcx>) -> R) -> R { let this = self.eval_context_ref(); if let Some(data_race) = &this.machine.data_race { - assert!(!data_race.ongoing_action_data_race_free.get(), "cannot nest allow_data_races"); - data_race.ongoing_action_data_race_free.set(true); + let old = data_race.ongoing_action_data_race_free.replace(true); + assert!(!old, "cannot nest allow_data_races"); } let result = op(this); if let Some(data_race) = &this.machine.data_race { @@ -975,8 +986,8 @@ fn allow_data_races_mut( ) -> R { let this = self.eval_context_mut(); if let Some(data_race) = &this.machine.data_race { - assert!(!data_race.ongoing_action_data_race_free.get(), "cannot nest allow_data_races"); - data_race.ongoing_action_data_race_free.set(true); + let old = data_race.ongoing_action_data_race_free.replace(true); + assert!(!old, "cannot nest allow_data_races"); } let result = op(this); if let Some(data_race) = &this.machine.data_race { diff --git a/src/eval.rs b/src/eval.rs index 511163a2e75..f7bc11a445d 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -377,10 +377,13 @@ pub fn eval_entry<'tcx>( }); // Machine cleanup. Only do this if all threads have terminated; threads that are still running - // might cause data races (https://github.com/rust-lang/miri/issues/2020) or Stacked Borrows - // errors (https://github.com/rust-lang/miri/issues/2396) if we deallocate here. + // might cause Stacked Borrows errors (https://github.com/rust-lang/miri/issues/2396). if ecx.have_all_terminated() { - EnvVars::cleanup(&mut ecx).unwrap(); + // Even if all threads have terminated, we have to beware of data races since some threads + // might not have joined the main thread (https://github.com/rust-lang/miri/issues/2020, + // https://github.com/rust-lang/miri/issues/2508). + ecx.allow_data_races_all_threads_done(); + EnvVars::cleanup(&mut ecx).expect("error during env var cleanup"); } // Process the result. diff --git a/tests/pass-dep/shims/env-cleanup-data-race.rs b/tests/pass-dep/shims/env-cleanup-data-race.rs new file mode 100644 index 00000000000..d36ffe70321 --- /dev/null +++ b/tests/pass-dep/shims/env-cleanup-data-race.rs @@ -0,0 +1,23 @@ +//@compile-flags: -Zmiri-disable-isolation -Zmiri-preemption-rate=0 +//@ignore-target-windows: No libc on Windows + +use std::ffi::CStr; +use std::ffi::CString; +use std::thread; + +fn main() { + unsafe { + thread::spawn(|| { + // Access the environment in another thread without taking the env lock + let k = CString::new("MIRI_ENV_VAR_TEST".as_bytes()).unwrap(); + let s = libc::getenv(k.as_ptr()) as *const libc::c_char; + if s.is_null() { + panic!("null"); + } + let _s = String::from_utf8_lossy(CStr::from_ptr(s).to_bytes()); + }); + thread::yield_now(); + // After the main thread exits, env vars will be cleaned up -- but because we have not *joined* + // the other thread, those accesses technically race with those in the other thread. + } +}