From 204c13b8c724b208b8e26148ba21087b543e960b Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Mar 2020 11:06:56 +0100 Subject: [PATCH] env shim: make sure we clean up all the memory we allocate --- src/eval.rs | 5 +++++ src/lib.rs | 2 ++ src/machine.rs | 8 ++++++-- src/shims/env.rs | 37 ++++++++++++++++++++++++++----------- src/stacked_borrows.rs | 4 +++- 5 files changed, 42 insertions(+), 14 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index 7f07fc1a7db..d6e5162edae 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -199,16 +199,21 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> // Perform the main execution. let res: InterpResult<'_, i64> = (|| { + // Main loop. while ecx.step()? { ecx.process_diagnostics(); } // Read the return code pointer *before* we run TLS destructors, to assert // that it was written to by the time that `start` lang item returned. let return_code = ecx.read_scalar(ret_place.into())?.not_undef()?.to_machine_isize(&ecx)?; + // Global destructors. ecx.run_tls_dtors()?; Ok(return_code) })(); + // Machine cleanup. + EnvVars::cleanup(&mut ecx).unwrap(); + // Process the result. match res { Ok(return_code) => { diff --git a/src/lib.rs b/src/lib.rs index 32eb5b41e59..0d93becb448 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -2,6 +2,8 @@ #![feature(option_expect_none, option_unwrap_none)] #![feature(map_first_last)] #![feature(never_type)] +#![feature(or_patterns)] + #![warn(rust_2018_idioms)] #![allow(clippy::cast_lossless)] diff --git a/src/machine.rs b/src/machine.rs index 73e7ce665b8..a530ef66d38 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -48,9 +48,13 @@ pub enum MiriMemoryKind { C, /// Windows `HeapAlloc` memory. WinHeap, - /// Memory for env vars and args, errno, extern statics and other parts of the machine-managed environment. + /// Memory for args, errno, extern statics and other parts of the machine-managed environment. + /// This memory may leak. Machine, + /// Memory for env vars. Separate from `Machine` because we clean it up and leak-check it. + Env, /// Globals copied from `tcx`. + /// This memory may leak. Global, } @@ -484,7 +488,7 @@ impl MayLeak for MiriMemoryKind { fn may_leak(self) -> bool { use self::MiriMemoryKind::*; match self { - Rust | C | WinHeap => false, + Rust | C | WinHeap | Env => false, Machine | Global => true, } } diff --git a/src/shims/env.rs b/src/shims/env.rs index ed689b1f427..6cde82903bc 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -46,6 +46,20 @@ pub(crate) fn init<'mir>( } ecx.update_environ() } + + pub(crate) fn cleanup<'mir>( + ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, + ) -> InterpResult<'tcx> { + // Deallocate individual env vars. + for (_name, ptr) in ecx.machine.env_vars.map.drain() { + ecx.memory.deallocate(ptr, None, MiriMemoryKind::Env.into())?; + } + // Deallocate environ var list. + let environ = ecx.machine.env_vars.environ.take().unwrap(); + let old_vars_ptr = ecx.read_scalar(environ.into())?.not_undef()?; + ecx.memory.deallocate(ecx.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::Env.into())?; + Ok(()) + } } fn alloc_env_var_as_c_str<'mir, 'tcx>( @@ -56,7 +70,7 @@ fn alloc_env_var_as_c_str<'mir, 'tcx>( let mut name_osstring = name.to_os_string(); name_osstring.push("="); name_osstring.push(value); - Ok(ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into())) + Ok(ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Env.into())) } fn alloc_env_var_as_wide_str<'mir, 'tcx>( @@ -67,7 +81,7 @@ fn alloc_env_var_as_wide_str<'mir, 'tcx>( let mut name_osstring = name.to_os_string(); name_osstring.push("="); name_osstring.push(value); - Ok(ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into())) + Ok(ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Env.into())) } impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} @@ -146,7 +160,7 @@ fn GetEnvironmentStringsW(&mut self) -> InterpResult<'tcx, Scalar> { } // Allocate environment block & Store environment variables to environment block. // Final null terminator(block terminator) is added by `alloc_os_str_to_wide_str`. - // FIXME: MemoryKind should be `Machine`, blocked on https://github.com/rust-lang/rust/pull/70479. + // FIXME: MemoryKind should be `Env`, blocked on https://github.com/rust-lang/rust/pull/70479. let envblock_ptr = this.alloc_os_str_as_wide_str(&env_vars, MiriMemoryKind::WinHeap.into()); // If the function succeeds, the return value is a pointer to the environment block of the current process. Ok(envblock_ptr.into()) @@ -158,7 +172,7 @@ fn FreeEnvironmentStringsW(&mut self, env_block_op: OpTy<'tcx, Tag>) -> InterpRe this.assert_target_os("windows", "FreeEnvironmentStringsW"); let env_block_ptr = this.read_scalar(env_block_op)?.not_undef()?; - // FIXME: MemoryKind should be `Machine`, blocked on https://github.com/rust-lang/rust/pull/70479. + // FIXME: MemoryKind should be `Env`, blocked on https://github.com/rust-lang/rust/pull/70479. let result = this.memory.deallocate(this.force_ptr(env_block_ptr)?, None, MiriMemoryKind::WinHeap.into()); // If the function succeeds, the return value is nonzero. Ok(result.is_ok() as i32) @@ -188,7 +202,7 @@ fn setenv( let var_ptr = alloc_env_var_as_c_str(&name, &value, &mut this)?; if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) { this.memory - .deallocate(var, None, MiriMemoryKind::Machine.into())?; + .deallocate(var, None, MiriMemoryKind::Env.into())?; } this.update_environ()?; Ok(0) // return zero on success @@ -225,7 +239,7 @@ fn SetEnvironmentVariableW( } else if this.is_null(value_ptr)? { // Delete environment variable `{name}` if let Some(var) = this.machine.env_vars.map.remove(&name) { - this.memory.deallocate(var, None, MiriMemoryKind::Machine.into())?; + this.memory.deallocate(var, None, MiriMemoryKind::Env.into())?; this.update_environ()?; } Ok(1) // return non-zero on success @@ -234,7 +248,7 @@ fn SetEnvironmentVariableW( let var_ptr = alloc_env_var_as_wide_str(&name, &value, &mut this)?; if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) { this.memory - .deallocate(var, None, MiriMemoryKind::Machine.into())?; + .deallocate(var, None, MiriMemoryKind::Env.into())?; } this.update_environ()?; Ok(1) // return non-zero on success @@ -257,7 +271,7 @@ fn unsetenv(&mut self, name_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { if let Some(old) = success { if let Some(var) = old { this.memory - .deallocate(var, None, MiriMemoryKind::Machine.into())?; + .deallocate(var, None, MiriMemoryKind::Env.into())?; } this.update_environ()?; Ok(0) @@ -314,12 +328,13 @@ fn chdir(&mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { /// The first time it gets called, also initializes `extra.environ`. fn update_environ(&mut self) -> InterpResult<'tcx> { let this = self.eval_context_mut(); - // Deallocate the old environ value, if any. + // Deallocate the old environ list, if any. if let Some(environ) = this.machine.env_vars.environ { let old_vars_ptr = this.read_scalar(environ.into())?.not_undef()?; - this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::Machine.into())?; + this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::Env.into())?; } else { // No `environ` allocated yet, let's do that. + // This is memory backing an extern static, hence `Machine`, not `Env`. let layout = this.layout_of(this.tcx.types.usize)?; let place = this.allocate(layout, MiriMemoryKind::Machine.into()); this.write_scalar(Scalar::from_machine_usize(0, &*this.tcx), place.into())?; @@ -334,7 +349,7 @@ fn update_environ(&mut self) -> InterpResult<'tcx> { let tcx = this.tcx; let vars_layout = this.layout_of(tcx.mk_array(tcx.types.usize, u64::try_from(vars.len()).unwrap()))?; - let vars_place = this.allocate(vars_layout, MiriMemoryKind::Machine.into()); + let vars_place = this.allocate(vars_layout, MiriMemoryKind::Env.into()); for (idx, var) in vars.into_iter().enumerate() { let place = this.mplace_field(vars_place, idx)?; this.write_scalar(var, place.into())?; diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 11b6cdca579..c130abf0576 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -460,8 +460,10 @@ pub fn new_allocation( // Global memory can be referenced by global pointers from `tcx`. // Thus we call `global_base_ptr` such that the global pointers get the same tag // as what we use here. + // `Machine` is used for extern statics, and thus must also be listed here. + // `Env` we list because we can get away with precise tracking there. // The base pointer is not unique, so the base permission is `SharedReadWrite`. - MemoryKind::Machine(MiriMemoryKind::Global) | MemoryKind::Machine(MiriMemoryKind::Machine) => + MemoryKind::Machine(MiriMemoryKind::Global | MiriMemoryKind::Machine | MiriMemoryKind::Env) => (extra.borrow_mut().global_base_ptr(id), Permission::SharedReadWrite), // Everything else we handle entirely untagged for now. // FIXME: experiment with more precise tracking. -- 2.44.0