follow-up to reviews

JOE1994 · JOE1994 · commit 2051805e957d · 2020-03-27T08:43:50.000-04:00
diff --git 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<String>,
     ) -> 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<Tag>>> {
+    fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer<Tag>>> {
         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<Tag>> {
+    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<Tag>> {
+    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<Tag>> {
@@ -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,30 +157,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         })
     }
 
-    fn getenvironmentstringsw(&mut self) -> InterpResult<'tcx, Scalar<Tag>> {
+    fn GetEnvironmentStringsW(&mut self) -> InterpResult<'tcx, Scalar<Tag>> {
         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))?;
+            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
@@ -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)?;
             }
