Merge pull request #4437 from RalfJung/env-cleanup

skip env var memory for leak check

Author: RalfJung

src/tools/miri/src/concurrency/data_race.rs

@@ -971,14 +971,6 @@ pub trait EvalContextExt<'tcx>: MiriInterpCxExt<'tcx> {
         }
     }
 
-    /// 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());
-        this.machine.data_race.set_ongoing_action_data_race_free(true);
-    }
-
     /// Calls the callback with the "release" clock of the current thread.
     /// Other threads can acquire this clock in the future to establish synchronization
     /// with this program point.

src/tools/miri/src/eval.rs

@@ -283,16 +283,6 @@ impl<'tcx> MainThreadState<'tcx> {
                 // to be like a global `static`, so that all memory reached by it is considered to "not leak".
                 this.terminate_active_thread(TlsAllocAction::Leak)?;
 
-                // Machine cleanup. Only do this if all threads have terminated; threads that are still running
-                // might cause Stacked Borrows errors (https://github.com/rust-lang/miri/issues/2396).
-                if this.have_all_terminated() {
-                    // 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).
-                    this.allow_data_races_all_threads_done();
-                    EnvVars::cleanup(this).expect("error during env var cleanup");
-                }
-
                 // Stop interpreter loop.
                 throw_machine_stop!(TerminationInfo::Exit { code: exit_code, leak_check: true });
             }

src/tools/miri/src/machine.rs

@@ -130,11 +130,11 @@ pub enum MiriMemoryKind {
     WinHeap,
     /// Windows "local" memory (to be freed with `LocalFree`)
     WinLocal,
-    /// Memory for args, errno, and other parts of the machine-managed environment.
+    /// Memory for args, errno, env vars, and other parts of the machine-managed environment.
     /// This memory may leak.
     Machine,
-    /// Memory allocated by the runtime (e.g. env vars). Separate from `Machine`
-    /// because we clean it up and leak-check it.
+    /// Memory allocated by the runtime, e.g. for readdir. Separate from `Machine` because we clean
+    /// it up (or expect the user to invoke operations that clean it up) and leak-check it.
     Runtime,
     /// Globals copied from `tcx`.
     /// This memory may leak.

src/tools/miri/src/shims/env.rs

@@ -59,15 +59,6 @@ impl<'tcx> EnvVars<'tcx> {
         interp_ok(())
     }
 
-    pub(crate) fn cleanup(ecx: &mut InterpCx<'tcx, MiriMachine<'tcx>>) -> InterpResult<'tcx> {
-        let this = ecx.eval_context_mut();
-        match this.machine.env_vars {
-            EnvVars::Unix(_) => UnixEnvVars::cleanup(this),
-            EnvVars::Windows(_) => interp_ok(()), // no cleanup needed
-            EnvVars::Uninit => interp_ok(()),
-        }
-    }
-
     pub(crate) fn unix(&self) -> &UnixEnvVars<'tcx> {
         match self {
             EnvVars::Unix(env) => env,

src/tools/miri/src/shims/unix/env.rs

@@ -1,6 +1,6 @@
+use std::env;
 use std::ffi::{OsStr, OsString};
 use std::io::ErrorKind;
-use std::{env, mem};
 
 use rustc_abi::{FieldIdx, Size};
 use rustc_data_structures::fx::FxHashMap;
@@ -50,20 +50,6 @@ impl<'tcx> UnixEnvVars<'tcx> {
         interp_ok(UnixEnvVars { map: env_vars_machine, environ })
     }
 
-    pub(crate) fn cleanup(ecx: &mut InterpCx<'tcx, MiriMachine<'tcx>>) -> InterpResult<'tcx> {
-        // Deallocate individual env vars.
-        let env_vars = mem::take(&mut ecx.machine.env_vars.unix_mut().map);
-        for (_name, ptr) in env_vars {
-            ecx.deallocate_ptr(ptr, None, MiriMemoryKind::Runtime.into())?;
-        }
-        // Deallocate environ var list.
-        let environ = &ecx.machine.env_vars.unix().environ;
-        let old_vars_ptr = ecx.read_pointer(environ)?;
-        ecx.deallocate_ptr(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?;
-
-        interp_ok(())
-    }
-
     pub(crate) fn environ(&self) -> Pointer {
         self.environ.ptr()
     }
@@ -112,7 +98,7 @@ fn alloc_env_var<'tcx>(
     let mut name_osstring = name.to_os_string();
     name_osstring.push("=");
     name_osstring.push(value);
-    ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Runtime.into())
+    ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into())
 }
 
 /// Allocates an `environ` block with the given list of pointers.
@@ -128,7 +114,7 @@ fn alloc_environ_block<'tcx>(
         ecx.machine.layouts.mut_raw_ptr.ty,
         u64::try_from(vars.len()).unwrap(),
     ))?;
-    let vars_place = ecx.allocate(vars_layout, MiriMemoryKind::Runtime.into())?;
+    let vars_place = ecx.allocate(vars_layout, MiriMemoryKind::Machine.into())?;
     for (idx, var) in vars.into_iter_enumerated() {
         let place = ecx.project_field(&vars_place, idx)?;
         ecx.write_pointer(var, &place)?;
@@ -171,7 +157,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         if let Some((name, value)) = new {
             let var_ptr = alloc_env_var(this, &name, &value)?;
             if let Some(var) = this.machine.env_vars.unix_mut().map.insert(name, var_ptr) {
-                this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?;
+                this.deallocate_ptr(var, None, MiriMemoryKind::Machine.into())?;
             }
             this.update_environ()?;
             interp_ok(Scalar::from_i32(0)) // return zero on success
@@ -195,7 +181,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         }
         if let Some(old) = success {
             if let Some(var) = old {
-                this.deallocate_ptr(var, None, MiriMemoryKind::Runtime.into())?;
+                this.deallocate_ptr(var, None, MiriMemoryKind::Machine.into())?;
             }
             this.update_environ()?;
             interp_ok(Scalar::from_i32(0))
@@ -253,7 +239,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         // Deallocate the old environ list.
         let environ = this.machine.env_vars.unix().environ.clone();
         let old_vars_ptr = this.read_pointer(&environ)?;
-        this.deallocate_ptr(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?;
+        this.deallocate_ptr(old_vars_ptr, None, MiriMemoryKind::Machine.into())?;
 
         // Write the new list.
         let vals = this.machine.env_vars.unix().map.values().copied().collect();