From 2fa07009f2a27c8d77ed48539cf66db6e2b5a676 Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Fri, 13 Mar 2020 12:45:02 -0400 Subject: [PATCH 1/9] Windows shims for env var emulation Shims for GetEnvironmentVariableW / SetEnvironmentVariableW / GetEnvironmentStringsW. Passes test 'tests/run-pass/env.rs' --- src/shims/env.rs | 120 ++++++++++++++++++++++++++++- src/shims/foreign_items/windows.rs | 30 ++++---- tests/run-pass/env.rs | 2 - 3 files changed, 136 insertions(+), 16 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index 79c386e894..97d74ad0e0 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -1,6 +1,7 @@ use std::ffi::{OsString, OsStr}; use std::env; use std::convert::TryFrom; +use std::collections::hash_map::Values; use crate::stacked_borrows::Tag; use crate::rustc_target::abi::LayoutOf; @@ -40,6 +41,10 @@ impl<'tcx> EnvVars<'tcx> { } ecx.update_environ() } + + pub(super) fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer>> { + Ok(self.map.values()) + } } fn alloc_env_var_as_c_str<'mir, 'tcx>( @@ -82,6 +87,79 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }) } + fn getenvironmentvariablew( + &mut self, + name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName + buf_op: OpTy<'tcx, Tag>, // LPWSTR lpBuffer + size_op: OpTy<'tcx, Tag>, // DWORD nSize + ) -> InterpResult<'tcx, u64> { + let this = self.eval_context_mut(); + this.assert_target_os("windows", "GetEnvironmentVariableW"); + + let name_ptr = this.read_scalar(name_op)?.not_undef()?; + let name = this.read_os_str_from_wide_str(name_ptr)?; + Ok(match this.machine.env_vars.map.get(&name) { + Some(var_ptr) => { + // The offset is used to strip the "{name}=" part of the string. + let name_offset_bytes = + u64::try_from(name.len()).unwrap().checked_add(1).unwrap().checked_mul(2).unwrap(); + let var_ptr = Scalar::from(var_ptr.offset(Size::from_bytes(name_offset_bytes), this)?); + + let var_size = u64::try_from(this.read_os_str_from_wide_str(var_ptr)?.len()).unwrap(); + let buf_size = u64::try_from(this.read_scalar(size_op)?.to_i32()?).unwrap(); + let return_val = if var_size.checked_add(1).unwrap() > buf_size { + // If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters, + // required to hold the string and its terminating null character and the contents of lpBuffer are undefined. + var_size + 1 + } else { + let buf_ptr = this.read_scalar(buf_op)?.not_undef()?; + for i in 0..var_size { + this.memory.copy( + this.force_ptr(var_ptr.ptr_offset(Size::from_bytes(i) * 2, this)?)?, + this.force_ptr(buf_ptr.ptr_offset(Size::from_bytes(i) * 2, this)?)?, + Size::from_bytes(2), + true, + )?; + } + // If the function succeeds, the return value is the number of characters stored in the buffer pointed to by lpBuffer, + // not including the terminating null character. + var_size + }; + return_val + } + None => { + this.set_last_error(Scalar::from_u32(203))?; // ERROR_ENVVAR_NOT_FOUND + 0 // return zero upon failure + } + }) + } + + fn getenvironmentstringsw(&mut self) -> InterpResult<'tcx, Scalar> { + let this = self.eval_context_mut(); + this.assert_target_os("windows", "GetEnvironmentStringsW"); + + // Info on layout of environment blocks in Windows: + // https://docs.microsoft.com/en-us/windows/win32/procthread/environment-variables + let mut env_vars = std::ffi::OsString::new(); + for &item in this.machine.env_vars.values()? { + let env_var = this.read_os_str_from_target_str(Scalar::from(item))?; + env_vars.push(env_var); + env_vars.push("\0"); + } + + // Allocate environment block + let tcx = this.tcx; + let env_block_size = env_vars.len().checked_add(1).unwrap(); + let env_block_type = tcx.mk_array(tcx.types.u16, u64::try_from(env_block_size).unwrap()); + let env_block_place = this.allocate(this.layout_of(env_block_type)?, MiriMemoryKind::WinHeap.into()); + + // Store environment variables to environment block + // Final null terminator(block terminator) is pushed by `write_os_str_to_wide_str` + this.write_os_str_to_wide_str(&env_vars, env_block_place, u64::try_from(env_block_size).unwrap())?; + + Ok(env_block_place.ptr) + } + fn setenv( &mut self, name_op: OpTy<'tcx, Tag>, @@ -118,6 +196,46 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } + fn setenvironmentvariablew( + &mut self, + name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName, + value_op: OpTy<'tcx, Tag>, // LPCWSTR lpValue, + ) -> InterpResult<'tcx, i32> { + let mut this = self.eval_context_mut(); + this.assert_target_os("windows", "SetEnvironmentVariableW"); + + let name_ptr = this.read_scalar(name_op)?.not_undef()?; + let value_ptr = this.read_scalar(value_op)?.not_undef()?; + + let mut new = None; + if !this.is_null(name_ptr)? { + let name = this.read_os_str_from_target_str(name_ptr)?; + 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.update_environ()?; + } + return Ok(1); // return non-zero on success + } + if !name.is_empty() && !name.to_string_lossy().contains('=') { + let value = this.read_os_str_from_target_str(value_ptr)?; + new = Some((name.to_owned(), value.to_owned())); + } + } + if let Some((name, value)) = new { + let var_ptr = alloc_env_var_as_target_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())?; + } + this.update_environ()?; + Ok(1) // return non-zero on success + } else { + Ok(0) + } + } + fn unsetenv(&mut self, name_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); let target_os = &this.tcx.sess.target.target.target_os; @@ -126,7 +244,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let name_ptr = this.read_scalar(name_op)?.not_undef()?; let mut success = None; if !this.is_null(name_ptr)? { - let name = this.read_os_str_from_c_str(name_ptr)?.to_owned(); + let name = this.read_os_str_from_target_str(name_ptr)?.to_owned(); if !name.is_empty() && !name.to_string_lossy().contains('=') { success = Some(this.machine.env_vars.map.remove(&name)); } diff --git a/src/shims/foreign_items/windows.rs b/src/shims/foreign_items/windows.rs index a35734573f..057f1c06a8 100644 --- a/src/shims/foreign_items/windows.rs +++ b/src/shims/foreign_items/windows.rs @@ -23,22 +23,26 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Environment related shims "GetEnvironmentVariableW" => { - // args[0] : LPCWSTR lpName (32-bit ptr to a const string of 16-bit Unicode chars) - // args[1] : LPWSTR lpBuffer (32-bit pointer to a string of 16-bit Unicode chars) - // lpBuffer : ptr to buffer that receives contents of the env_var as a null-terminated string. - // Return `# of chars` stored in the buffer pointed to by lpBuffer, excluding null-terminator. - // Return 0 upon failure. - - // This is not the env var you are looking for. - this.set_last_error(Scalar::from_u32(203))?; // ERROR_ENVVAR_NOT_FOUND - this.write_null(dest)?; + let result = this.getenvironmentvariablew(args[0], args[1], args[2])?; + this.write_scalar(Scalar::from_uint(result, dest.layout.size), dest)?; } "SetEnvironmentVariableW" => { - // args[0] : LPCWSTR lpName (32-bit ptr to a const string of 16-bit Unicode chars) - // args[1] : LPCWSTR lpValue (32-bit ptr to a const string of 16-bit Unicode chars) - // Return nonzero if success, else return 0. - throw_unsup_format!("can't set environment variable on Windows"); + let result = this.setenvironmentvariablew(args[0], args[1])?; + this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; + } + + "GetEnvironmentStringsW" => { + let result = this.getenvironmentstringsw()?; + // If the function succeeds, the return value is a pointer to the environment block of the current process. + this.write_scalar(result, dest)?; + } + + "FreeEnvironmentStringsW" => { + let old_vars_ptr = this.read_scalar(args[0])?.not_undef()?; + let result = this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::WinHeap.into()).is_ok(); + // If the function succeeds, the return value is nonzero. + this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; } // File related shims diff --git a/tests/run-pass/env.rs b/tests/run-pass/env.rs index c7506b23c1..23a3724ff7 100644 --- a/tests/run-pass/env.rs +++ b/tests/run-pass/env.rs @@ -1,5 +1,3 @@ -//ignore-windows: TODO env var emulation stubbed out on Windows - use std::env; fn main() { From cf5822af460f342721662a4dc959713a1cd7778c Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Tue, 24 Mar 2020 16:00:11 -0400 Subject: [PATCH 2/9] exclude 'TERM' env_var to avoid terminfo trying to open the termcap file --- README.md | 2 +- src/shims/env.rs | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 59394830d7..03b995aa56 100644 --- a/README.md +++ b/README.md @@ -166,7 +166,7 @@ Several `-Z` flags are relevant for Miri: * `-Zmiri-disable-stacked-borrows` disables checking the experimental [Stacked Borrows] aliasing rules. This can make Miri run faster, but it also means no aliasing violations will be detected. -* `-Zmiri-disable-isolation` disables host host isolation. As a consequence, +* `-Zmiri-disable-isolation` disables host isolation. As a consequence, the program has access to host resources such as environment variables, file systems, and randomness. * `-Zmiri-ignore-leaks` disables the memory leak checker. diff --git a/src/shims/env.rs b/src/shims/env.rs index 97d74ad0e0..32ed65b398 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -24,8 +24,12 @@ pub struct EnvVars<'tcx> { impl<'tcx> EnvVars<'tcx> { pub(crate) fn init<'mir>( ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, - excluded_env_vars: Vec, + mut excluded_env_vars: Vec, ) -> InterpResult<'tcx> { + if ecx.tcx.sess.target.target.target_os.to_lowercase() == "windows" { + // Exclude `TERM` var to avoid terminfo trying to open the termcap file. + excluded_env_vars.push("TERM".to_owned()); + } if ecx.machine.communicate { let target_os = ecx.tcx.sess.target.target.target_os.as_str(); for (name, value) in env::vars() { From 2051805e957d307f7f084172b61cf0a6d69edfc9 Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Wed, 25 Mar 2020 00:52:53 -0400 Subject: [PATCH 3/9] follow-up to reviews --- src/shims/env.rs | 110 +++++++++++++++++------------ src/shims/foreign_items/windows.rs | 9 ++- 2 files changed, 69 insertions(+), 50 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index 32ed65b398..2e647589f4 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -1,3 +1,5 @@ +#![allow(non_snake_case)] + use std::ffi::{OsString, OsStr}; use std::env; use std::convert::TryFrom; @@ -26,7 +28,7 @@ impl<'tcx> EnvVars<'tcx> { ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, mut excluded_env_vars: Vec, ) -> InterpResult<'tcx> { - if ecx.tcx.sess.target.target.target_os.to_lowercase() == "windows" { + if ecx.tcx.sess.target.target.target_os == "windows" { // Exclude `TERM` var to avoid terminfo trying to open the termcap file. excluded_env_vars.push("TERM".to_owned()); } @@ -46,7 +48,7 @@ impl<'tcx> EnvVars<'tcx> { ecx.update_environ() } - pub(super) fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer>> { + fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer>> { Ok(self.map.values()) } } @@ -73,6 +75,28 @@ fn alloc_env_var_as_wide_str<'mir, 'tcx>( Ok(ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into())) } +fn alloc_env_var_as_c_str<'mir, 'tcx>( + name: &OsStr, + value: &OsStr, + ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, +) -> InterpResult<'tcx, Pointer> { + 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())) +} + +fn alloc_env_var_as_wide_str<'mir, 'tcx>( + name: &OsStr, + value: &OsStr, + ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, +) -> InterpResult<'tcx, Pointer> { + 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())) +} + impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { fn getenv(&mut self, name_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, Scalar> { @@ -91,7 +115,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }) } - fn getenvironmentvariablew( + fn GetEnvironmentVariableW( &mut self, name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName buf_op: OpTy<'tcx, Tag>, // LPWSTR lpBuffer @@ -110,21 +134,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let var_ptr = Scalar::from(var_ptr.offset(Size::from_bytes(name_offset_bytes), this)?); let var_size = u64::try_from(this.read_os_str_from_wide_str(var_ptr)?.len()).unwrap(); - let buf_size = u64::try_from(this.read_scalar(size_op)?.to_i32()?).unwrap(); + // `buf_size` represent size in characters. + let buf_size = u64::try_from(this.read_scalar(size_op)?.to_u32()?).unwrap(); let return_val = if var_size.checked_add(1).unwrap() > buf_size { // If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters, // required to hold the string and its terminating null character and the contents of lpBuffer are undefined. var_size + 1 } else { let buf_ptr = this.read_scalar(buf_op)?.not_undef()?; - for i in 0..var_size { - this.memory.copy( - this.force_ptr(var_ptr.ptr_offset(Size::from_bytes(i) * 2, this)?)?, - this.force_ptr(buf_ptr.ptr_offset(Size::from_bytes(i) * 2, this)?)?, - Size::from_bytes(2), - true, - )?; - } + let bytes_to_be_copied = var_size.checked_add(1).unwrap().checked_mul(2).unwrap(); + this.memory.copy(this.force_ptr(var_ptr)?, this.force_ptr(buf_ptr)?, Size::from_bytes(bytes_to_be_copied), true)?; // If the function succeeds, the return value is the number of characters stored in the buffer pointed to by lpBuffer, // not including the terminating null character. var_size @@ -138,7 +157,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }) } - fn getenvironmentstringsw(&mut self) -> InterpResult<'tcx, Scalar> { + fn GetEnvironmentStringsW(&mut self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); this.assert_target_os("windows", "GetEnvironmentStringsW"); @@ -146,22 +165,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // https://docs.microsoft.com/en-us/windows/win32/procthread/environment-variables let mut env_vars = std::ffi::OsString::new(); for &item in this.machine.env_vars.values()? { - let env_var = this.read_os_str_from_target_str(Scalar::from(item))?; + let env_var = this.read_os_str_from_wide_str(Scalar::from(item))?; env_vars.push(env_var); env_vars.push("\0"); } + // Allocate environment block & Store environment variables to environment block. + // Final null terminator(block terminator) is added by `alloc_os_str_to_wide_str`. + let envblock_ptr = this.alloc_os_str_as_wide_str(&env_vars, MiriMemoryKind::WinHeap.into()); - // Allocate environment block - let tcx = this.tcx; - let env_block_size = env_vars.len().checked_add(1).unwrap(); - let env_block_type = tcx.mk_array(tcx.types.u16, u64::try_from(env_block_size).unwrap()); - let env_block_place = this.allocate(this.layout_of(env_block_type)?, MiriMemoryKind::WinHeap.into()); - - // Store environment variables to environment block - // Final null terminator(block terminator) is pushed by `write_os_str_to_wide_str` - this.write_os_str_to_wide_str(&env_vars, env_block_place, u64::try_from(env_block_size).unwrap())?; + Ok(envblock_ptr.into()) + } + + fn FreeEnvironmentStringsW(&mut self, env_block_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, bool> { + let this = self.eval_context_mut(); + this.assert_target_os("windows", "FreeEnvironmentStringsW"); - Ok(env_block_place.ptr) + let env_block_ptr = this.read_scalar(env_block_op)?.not_undef()?; + Ok(this.memory.deallocate(this.force_ptr(env_block_ptr)?, None, MiriMemoryKind::WinHeap.into()).is_ok()) } fn setenv( @@ -200,7 +220,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } - fn setenvironmentvariablew( + fn SetEnvironmentVariableW( &mut self, name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName, value_op: OpTy<'tcx, Tag>, // LPCWSTR lpValue, @@ -211,32 +231,32 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let name_ptr = this.read_scalar(name_op)?.not_undef()?; let value_ptr = this.read_scalar(value_op)?.not_undef()?; - let mut new = None; - if !this.is_null(name_ptr)? { - let name = this.read_os_str_from_target_str(name_ptr)?; - 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.update_environ()?; - } - return Ok(1); // return non-zero on success - } - if !name.is_empty() && !name.to_string_lossy().contains('=') { - let value = this.read_os_str_from_target_str(value_ptr)?; - new = Some((name.to_owned(), value.to_owned())); - } + if this.is_null(name_ptr)? { + // ERROR CODE is not clearly explained in docs.. For now, throw UB instead. + throw_ub_format!("Pointer to environment variable name is NULL"); } - if let Some((name, value)) = new { - let var_ptr = alloc_env_var_as_target_str(&name, &value, &mut this)?; + + let name = this.read_os_str_from_wide_str(name_ptr)?; + if name.is_empty() { + throw_unsup_format!("Environment variable name is an empty string"); + } else if name.to_string_lossy().contains('=') { + throw_unsup_format!("Environment variable name contains '='"); + } 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.update_environ()?; + } + Ok(1) // return non-zero on success + } else { + let value = this.read_os_str_from_wide_str(value_ptr)?; + 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())?; } this.update_environ()?; Ok(1) // return non-zero on success - } else { - Ok(0) } } @@ -248,7 +268,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let name_ptr = this.read_scalar(name_op)?.not_undef()?; let mut success = None; if !this.is_null(name_ptr)? { - let name = this.read_os_str_from_target_str(name_ptr)?.to_owned(); + let name = this.read_os_str_from_c_str(name_ptr)?.to_owned(); if !name.is_empty() && !name.to_string_lossy().contains('=') { success = Some(this.machine.env_vars.map.remove(&name)); } diff --git a/src/shims/foreign_items/windows.rs b/src/shims/foreign_items/windows.rs index 057f1c06a8..cc64057135 100644 --- a/src/shims/foreign_items/windows.rs +++ b/src/shims/foreign_items/windows.rs @@ -23,24 +23,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Environment related shims "GetEnvironmentVariableW" => { - let result = this.getenvironmentvariablew(args[0], args[1], args[2])?; + let result = this.GetEnvironmentVariableW(args[0], args[1], args[2])?; this.write_scalar(Scalar::from_uint(result, dest.layout.size), dest)?; } "SetEnvironmentVariableW" => { - let result = this.setenvironmentvariablew(args[0], args[1])?; + let result = this.SetEnvironmentVariableW(args[0], args[1])?; this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; } "GetEnvironmentStringsW" => { - let result = this.getenvironmentstringsw()?; + let result = this.GetEnvironmentStringsW()?; // If the function succeeds, the return value is a pointer to the environment block of the current process. this.write_scalar(result, dest)?; } "FreeEnvironmentStringsW" => { - let old_vars_ptr = this.read_scalar(args[0])?.not_undef()?; - let result = this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::WinHeap.into()).is_ok(); + let result = this.FreeEnvironmentStringsW(args[0])?; // If the function succeeds, the return value is nonzero. this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; } From 4e38fbe6be6d5c8d67de5ca65ab763f9f314a398 Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Wed, 25 Mar 2020 10:38:27 -0400 Subject: [PATCH 4/9] follow-up2 to review (few issues not resolved yet) --- src/shims/env.rs | 45 +++++++++++++++++------------- src/shims/foreign_items/windows.rs | 4 +-- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index 2e647589f4..cc7c305596 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -1,9 +1,6 @@ -#![allow(non_snake_case)] - use std::ffi::{OsString, OsStr}; use std::env; use std::convert::TryFrom; -use std::collections::hash_map::Values; use crate::stacked_borrows::Tag; use crate::rustc_target::abi::LayoutOf; @@ -47,10 +44,6 @@ impl<'tcx> EnvVars<'tcx> { } ecx.update_environ() } - - fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer>> { - Ok(self.map.values()) - } } fn alloc_env_var_as_c_str<'mir, 'tcx>( @@ -115,11 +108,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }) } + #[allow(non_snake_case)] fn GetEnvironmentVariableW( &mut self, - name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName - buf_op: OpTy<'tcx, Tag>, // LPWSTR lpBuffer - size_op: OpTy<'tcx, Tag>, // DWORD nSize + name_op: OpTy<'tcx, Tag>, // LPCWSTR + buf_op: OpTy<'tcx, Tag>, // LPWSTR + size_op: OpTy<'tcx, Tag>, // DWORD ) -> InterpResult<'tcx, u64> { let this = self.eval_context_mut(); this.assert_target_os("windows", "GetEnvironmentVariableW"); @@ -134,7 +128,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let var_ptr = Scalar::from(var_ptr.offset(Size::from_bytes(name_offset_bytes), this)?); let var_size = u64::try_from(this.read_os_str_from_wide_str(var_ptr)?.len()).unwrap(); - // `buf_size` represent size in characters. + // `buf_size` represents the size in characters. let buf_size = u64::try_from(this.read_scalar(size_op)?.to_u32()?).unwrap(); let return_val = if var_size.checked_add(1).unwrap() > buf_size { // If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters, @@ -157,6 +151,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx }) } + #[allow(non_snake_case)] fn GetEnvironmentStringsW(&mut self) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); this.assert_target_os("windows", "GetEnvironmentStringsW"); @@ -164,24 +159,33 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Info on layout of environment blocks in Windows: // https://docs.microsoft.com/en-us/windows/win32/procthread/environment-variables let mut env_vars = std::ffi::OsString::new(); - for &item in this.machine.env_vars.values()? { + for &item in this.machine.env_vars.map.values() { let env_var = this.read_os_str_from_wide_str(Scalar::from(item))?; env_vars.push(env_var); env_vars.push("\0"); } // 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 `MiMemoryKind::Machine`, + // but using it results in a Stacked Borrows error when running MIRI on 'tests/run-pass/env.rs' + // For now, use `MiriMemoryKind::WinHeap` instead. 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()) } - fn FreeEnvironmentStringsW(&mut self, env_block_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, bool> { + #[allow(non_snake_case)] + fn FreeEnvironmentStringsW(&mut self, env_block_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); this.assert_target_os("windows", "FreeEnvironmentStringsW"); let env_block_ptr = this.read_scalar(env_block_op)?.not_undef()?; - Ok(this.memory.deallocate(this.force_ptr(env_block_ptr)?, None, MiriMemoryKind::WinHeap.into()).is_ok()) + // FIXME: MemoryKind should be `MiMemoryKind::Machine`, + // but using it results in a Stacked Borrows error when running MIRI on 'tests/run-pass/env.rs' + // For now, use `MiriMemoryKind::WinHeap` instead. + 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) } fn setenv( @@ -220,10 +224,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } + #[allow(non_snake_case)] fn SetEnvironmentVariableW( &mut self, - name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName, - value_op: OpTy<'tcx, Tag>, // LPCWSTR lpValue, + name_op: OpTy<'tcx, Tag>, // LPCWSTR + value_op: OpTy<'tcx, Tag>, // LPCWSTR ) -> InterpResult<'tcx, i32> { let mut this = self.eval_context_mut(); this.assert_target_os("windows", "SetEnvironmentVariableW"); @@ -233,14 +238,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if this.is_null(name_ptr)? { // ERROR CODE is not clearly explained in docs.. For now, throw UB instead. - throw_ub_format!("Pointer to environment variable name is NULL"); + throw_ub_format!("pointer to environment variable name is NULL"); } let name = this.read_os_str_from_wide_str(name_ptr)?; if name.is_empty() { - throw_unsup_format!("Environment variable name is an empty string"); + throw_unsup_format!("environment variable name is an empty string"); } else if name.to_string_lossy().contains('=') { - throw_unsup_format!("Environment variable name contains '='"); + throw_unsup_format!("environment variable name contains '='"); } else if this.is_null(value_ptr)? { // Delete environment variable `{name}` if let Some(var) = this.machine.env_vars.map.remove(&name) { diff --git a/src/shims/foreign_items/windows.rs b/src/shims/foreign_items/windows.rs index cc64057135..a64ef0f129 100644 --- a/src/shims/foreign_items/windows.rs +++ b/src/shims/foreign_items/windows.rs @@ -34,14 +34,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx "GetEnvironmentStringsW" => { let result = this.GetEnvironmentStringsW()?; - // If the function succeeds, the return value is a pointer to the environment block of the current process. this.write_scalar(result, dest)?; } "FreeEnvironmentStringsW" => { let result = this.FreeEnvironmentStringsW(args[0])?; - // If the function succeeds, the return value is nonzero. - this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; + this.write_scalar(Scalar::from_i32(result), dest)?; } // File related shims From f3e3af4beec260541dbeeead0a824caee7624e77 Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Fri, 27 Mar 2020 09:18:13 -0400 Subject: [PATCH 5/9] adjust to change of 'fn write_os_str_to_wide_str' --- src/shims/env.rs | 52 ++++++++++++++---------------------------------- 1 file changed, 15 insertions(+), 37 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index cc7c305596..794a1aab42 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -25,12 +25,12 @@ impl<'tcx> EnvVars<'tcx> { ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, mut excluded_env_vars: Vec, ) -> InterpResult<'tcx> { - if ecx.tcx.sess.target.target.target_os == "windows" { + let target_os = ecx.tcx.sess.target.target.target_os.as_str(); + if target_os == "windows" { // Exclude `TERM` var to avoid terminfo trying to open the termcap file. excluded_env_vars.push("TERM".to_owned()); } if ecx.machine.communicate { - let target_os = ecx.tcx.sess.target.target.target_os.as_str(); for (name, value) in env::vars() { if !excluded_env_vars.contains(&name) { let var_ptr = match target_os { @@ -68,28 +68,6 @@ fn alloc_env_var_as_wide_str<'mir, 'tcx>( Ok(ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into())) } -fn alloc_env_var_as_c_str<'mir, 'tcx>( - name: &OsStr, - value: &OsStr, - ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, -) -> InterpResult<'tcx, Pointer> { - 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())) -} - -fn alloc_env_var_as_wide_str<'mir, 'tcx>( - name: &OsStr, - value: &OsStr, - ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>, -) -> InterpResult<'tcx, Pointer> { - 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())) -} - impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { fn getenv(&mut self, name_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, Scalar> { @@ -126,26 +104,26 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let name_offset_bytes = u64::try_from(name.len()).unwrap().checked_add(1).unwrap().checked_mul(2).unwrap(); let var_ptr = Scalar::from(var_ptr.offset(Size::from_bytes(name_offset_bytes), this)?); + let var = this.read_os_str_from_wide_str(var_ptr)?; - let var_size = u64::try_from(this.read_os_str_from_wide_str(var_ptr)?.len()).unwrap(); + let buf_ptr = this.read_scalar(buf_op)?.not_undef()?; // `buf_size` represents the size in characters. let buf_size = u64::try_from(this.read_scalar(size_op)?.to_u32()?).unwrap(); - let return_val = if var_size.checked_add(1).unwrap() > buf_size { - // If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters, - // required to hold the string and its terminating null character and the contents of lpBuffer are undefined. - var_size + 1 - } else { - let buf_ptr = this.read_scalar(buf_op)?.not_undef()?; - let bytes_to_be_copied = var_size.checked_add(1).unwrap().checked_mul(2).unwrap(); - this.memory.copy(this.force_ptr(var_ptr)?, this.force_ptr(buf_ptr)?, Size::from_bytes(bytes_to_be_copied), true)?; + let (success, len) = this.write_os_str_to_wide_str(&var, buf_ptr, buf_size)?; + + if success { // If the function succeeds, the return value is the number of characters stored in the buffer pointed to by lpBuffer, // not including the terminating null character. - var_size - }; - return_val + len + } else { + // If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters, + // required to hold the string and its terminating null character and the contents of lpBuffer are undefined. + len + 1 + } } None => { - this.set_last_error(Scalar::from_u32(203))?; // ERROR_ENVVAR_NOT_FOUND + let envvar_not_found = this.eval_path_scalar(&["std", "sys", "windows", "c", "ERROR_ENVVAR_NOT_FOUND"])?; + this.set_last_error(envvar_not_found.not_undef()?)?; 0 // return zero upon failure } }) From eaca17fcc31a74690bc89d6b2aa7c41b4aedb79a Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Fri, 27 Mar 2020 09:59:42 -0400 Subject: [PATCH 6/9] add reference to issue#1013 --- src/shims/env.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index 794a1aab42..eee9a53918 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -27,9 +27,11 @@ impl<'tcx> EnvVars<'tcx> { ) -> InterpResult<'tcx> { let target_os = ecx.tcx.sess.target.target.target_os.as_str(); if target_os == "windows" { - // Exclude `TERM` var to avoid terminfo trying to open the termcap file. + // Temporary hack: Exclude `TERM` var to avoid terminfo trying to open the termcap file. + // Can be removed once Issue#1013(Implement file system access for Windows) is resolved. excluded_env_vars.push("TERM".to_owned()); } + if ecx.machine.communicate { for (name, value) in env::vars() { if !excluded_env_vars.contains(&name) { From 3fe71dff5a58efaabd1686e2a25c09adb8d74092 Mon Sep 17 00:00:00 2001 From: Youngsuk Kim Date: Fri, 27 Mar 2020 10:15:35 -0400 Subject: [PATCH 7/9] Modify reference to issue 1013 Co-Authored-By: Ralf Jung --- src/shims/env.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index eee9a53918..b6d7010263 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -28,7 +28,7 @@ impl<'tcx> EnvVars<'tcx> { let target_os = ecx.tcx.sess.target.target.target_os.as_str(); if target_os == "windows" { // Temporary hack: Exclude `TERM` var to avoid terminfo trying to open the termcap file. - // Can be removed once Issue#1013(Implement file system access for Windows) is resolved. + // Can be removed once https://github.com/rust-lang/miri/issues/1013 is resolved. excluded_env_vars.push("TERM".to_owned()); } From ea836eeb0d72d5f983489f283ea7fc5f6a7ad442 Mon Sep 17 00:00:00 2001 From: JOE1994 Date: Fri, 27 Mar 2020 14:18:19 -0400 Subject: [PATCH 8/9] remove or update 'ignore-windows' annotations in some tests --- tests/compile-fail/environ-gets-deallocated.rs | 2 +- tests/run-pass/env-exclude.rs | 1 - tests/run-pass/env-without-isolation.rs | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/compile-fail/environ-gets-deallocated.rs b/tests/compile-fail/environ-gets-deallocated.rs index dd00aef450..0f374a2e3f 100644 --- a/tests/compile-fail/environ-gets-deallocated.rs +++ b/tests/compile-fail/environ-gets-deallocated.rs @@ -1,4 +1,4 @@ -//ignore-windows: TODO env var emulation stubbed out on Windows +//ignore-windows: Windows does not have a global environ list that the program can access directly #[cfg(target_os="linux")] fn get_environ() -> *const *const u8 { diff --git a/tests/run-pass/env-exclude.rs b/tests/run-pass/env-exclude.rs index efcf7a7561..1e251084f0 100644 --- a/tests/run-pass/env-exclude.rs +++ b/tests/run-pass/env-exclude.rs @@ -1,4 +1,3 @@ -// ignore-windows: TODO env var emulation stubbed out on Windows // compile-flags: -Zmiri-disable-isolation -Zmiri-env-exclude=MIRI_ENV_VAR_TEST fn main() { diff --git a/tests/run-pass/env-without-isolation.rs b/tests/run-pass/env-without-isolation.rs index 537e0923d2..6384762098 100644 --- a/tests/run-pass/env-without-isolation.rs +++ b/tests/run-pass/env-without-isolation.rs @@ -1,4 +1,3 @@ -// ignore-windows: TODO env var emulation stubbed out on Windows // compile-flags: -Zmiri-disable-isolation fn main() { From 579b3c49dac9def1495f7d97d4f5b2771bbeefd8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 27 Mar 2020 20:36:18 +0100 Subject: [PATCH 9/9] adjust MemoryKind comment --- src/shims/env.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index b6d7010263..ed689b1f42 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -146,9 +146,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } // 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 `MiMemoryKind::Machine`, - // but using it results in a Stacked Borrows error when running MIRI on 'tests/run-pass/env.rs' - // For now, use `MiriMemoryKind::WinHeap` instead. + // FIXME: MemoryKind should be `Machine`, 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()) @@ -160,9 +158,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.assert_target_os("windows", "FreeEnvironmentStringsW"); let env_block_ptr = this.read_scalar(env_block_op)?.not_undef()?; - // FIXME: MemoryKind should be `MiMemoryKind::Machine`, - // but using it results in a Stacked Borrows error when running MIRI on 'tests/run-pass/env.rs' - // For now, use `MiriMemoryKind::WinHeap` instead. + // FIXME: MemoryKind should be `Machine`, 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)