Auto merge of rust-lang#2509 - RalfJung:env-data-race, r=RalfJung

fix data race error during env var cleanup

Fixes rust-lang/miri#2508

Author: bors

src/concurrency/data_race.rs

@@ -689,6 +689,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, '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<R>(&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 @@ trait EvalContextPrivExt<'mir, 'tcx: 'mir>: MiriEvalContextExt<'mir, 'tcx> {
     ) -> 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 {

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.

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.
+    }
+}