Skip to content

Commit 405848f

Browse files
JOE1994JOE1994
committed
follow-up to reviews
1 parent 8115e3a commit 405848f

File tree

2 files changed

+77
-53
lines changed

2 files changed

+77
-53
lines changed

‎src/shims/env.rs‎

Lines changed: 73 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
#![allow(non_snake_case)]
2+
13
use std::ffi::{OsString, OsStr};
24
use std::env;
35
use std::convert::TryFrom;
@@ -26,7 +28,7 @@ impl<'tcx> EnvVars<'tcx> {
2628
ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>,
2729
mut excluded_env_vars: Vec<String>,
2830
) -> InterpResult<'tcx> {
29-
if ecx.tcx.sess.target.target.target_os.to_lowercase() == "windows" {
31+
if ecx.tcx.sess.target.target.target_os == "windows" {
3032
// Exclude `TERM` var to avoid terminfo trying to open the termcap file.
3133
excluded_env_vars.push("TERM".to_owned());
3234
}
@@ -42,7 +44,7 @@ impl<'tcx> EnvVars<'tcx> {
4244
ecx.update_environ()
4345
}
4446

45-
pub(super) fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer<Tag>>> {
47+
fn values(&self) -> InterpResult<'tcx, Values<'_, OsString, Pointer<Tag>>> {
4648
Ok(self.map.values())
4749
}
4850
}
@@ -58,6 +60,28 @@ fn alloc_env_var_as_target_str<'mir, 'tcx>(
5860
Ok(ecx.alloc_os_str_as_target_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into())?)
5961
}
6062

63+
fn alloc_env_var_as_c_str<'mir, 'tcx>(
64+
name: &OsStr,
65+
value: &OsStr,
66+
ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>,
67+
) -> InterpResult<'tcx, Pointer<Tag>> {
68+
let mut name_osstring = name.to_os_string();
69+
name_osstring.push("=");
70+
name_osstring.push(value);
71+
Ok(ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into()))
72+
}
73+
74+
fn alloc_env_var_as_wide_str<'mir, 'tcx>(
75+
name: &OsStr,
76+
value: &OsStr,
77+
ecx: &mut InterpCx<'mir, 'tcx, Evaluator<'tcx>>,
78+
) -> InterpResult<'tcx, Pointer<Tag>> {
79+
let mut name_osstring = name.to_os_string();
80+
name_osstring.push("=");
81+
name_osstring.push(value);
82+
Ok(ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Machine.into()))
83+
}
84+
6185
impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
6286
pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
6387
fn getenv(&mut self, name_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, Scalar<Tag>> {
@@ -76,7 +100,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
76100
})
77101
}
78102

79-
fn getenvironmentvariablew(
103+
fn GetEnvironmentVariableW(
80104
&mut self,
81105
name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName
82106
buf_op: OpTy<'tcx, Tag>, // LPWSTR lpBuffer
@@ -95,21 +119,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
95119
let var_ptr = Scalar::from(var_ptr.offset(Size::from_bytes(name_offset_bytes), this)?);
96120

97121
let var_size = u64::try_from(this.read_os_str_from_wide_str(var_ptr)?.len()).unwrap();
98-
let buf_size = u64::try_from(this.read_scalar(size_op)?.to_i32()?).unwrap();
122+
// `buf_size` represent size in characters.
123+
let buf_size = u64::try_from(this.read_scalar(size_op)?.to_u32()?).unwrap();
99124
let return_val = if var_size.checked_add(1).unwrap() > buf_size {
100125
// If lpBuffer is not large enough to hold the data, the return value is the buffer size, in characters,
101126
// required to hold the string and its terminating null character and the contents of lpBuffer are undefined.
102127
var_size + 1
103128
} else {
104129
let buf_ptr = this.read_scalar(buf_op)?.not_undef()?;
105-
for i in 0..var_size {
106-
this.memory.copy(
107-
this.force_ptr(var_ptr.ptr_offset(Size::from_bytes(i) * 2, this)?)?,
108-
this.force_ptr(buf_ptr.ptr_offset(Size::from_bytes(i) * 2, this)?)?,
109-
Size::from_bytes(2),
110-
true,
111-
)?;
112-
}
130+
let bytes_to_be_copied = var_size.checked_add(1).unwrap().checked_mul(2).unwrap();
131+
this.memory.copy(this.force_ptr(var_ptr)?, this.force_ptr(buf_ptr)?, Size::from_bytes(bytes_to_be_copied), true)?;
113132
// If the function succeeds, the return value is the number of characters stored in the buffer pointed to by lpBuffer,
114133
// not including the terminating null character.
115134
var_size
@@ -123,30 +142,31 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
123142
})
124143
}
125144

126-
fn getenvironmentstringsw(&mut self) -> InterpResult<'tcx, Scalar<Tag>> {
145+
fn GetEnvironmentStringsW(&mut self) -> InterpResult<'tcx, Scalar<Tag>> {
127146
let this = self.eval_context_mut();
128147
this.assert_target_os("windows", "GetEnvironmentStringsW");
129148

130149
// Info on layout of environment blocks in Windows:
131150
// https://docs.microsoft.com/en-us/windows/win32/procthread/environment-variables
132151
let mut env_vars = std::ffi::OsString::new();
133152
for &item in this.machine.env_vars.values()? {
134-
let env_var = this.read_os_str_from_target_str(Scalar::from(item))?;
153+
let env_var = this.read_os_str_from_wide_str(Scalar::from(item))?;
135154
env_vars.push(env_var);
136155
env_vars.push("\0");
137156
}
157+
// Allocate environment block & Store environment variables to environment block.
158+
// Final null terminator(block terminator) is added by `alloc_os_str_to_wide_str`.
159+
let envblock_ptr = this.alloc_os_str_as_wide_str(&env_vars, MiriMemoryKind::WinHeap.into());
138160

139-
// Allocate environment block
140-
let tcx = this.tcx;
141-
let env_block_size = env_vars.len().checked_add(1).unwrap();
142-
let env_block_type = tcx.mk_array(tcx.types.u16, u64::try_from(env_block_size).unwrap());
143-
let env_block_place = this.allocate(this.layout_of(env_block_type)?, MiriMemoryKind::WinHeap.into());
144-
145-
// Store environment variables to environment block
146-
// Final null terminator(block terminator) is pushed by `write_os_str_to_wide_str`
147-
this.write_os_str_to_wide_str(&env_vars, env_block_place, u64::try_from(env_block_size).unwrap())?;
161+
Ok(envblock_ptr.into())
162+
}
148163

149-
Ok(env_block_place.ptr)
164+
fn FreeEnvironmentStringsW(&mut self, env_block_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, bool> {
165+
let this = self.eval_context_mut();
166+
this.assert_target_os("windows", "FreeEnvironmentStringsW");
167+
168+
let env_block_ptr = this.read_scalar(env_block_op)?.not_undef()?;
169+
Ok(this.memory.deallocate(this.force_ptr(env_block_ptr)?, None, MiriMemoryKind::WinHeap.into()).is_ok())
150170
}
151171

152172
fn setenv(
@@ -163,26 +183,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
163183

164184
let mut new = None;
165185
if !this.is_null(name_ptr)? {
166-
let name = this.read_os_str_from_target_str(name_ptr)?;
186+
let name = this.read_os_str_from_c_str(name_ptr)?;
167187
if !name.is_empty() && !name.to_string_lossy().contains('=') {
168-
let value = this.read_os_str_from_target_str(value_ptr)?;
188+
let value = this.read_os_str_from_c_str(value_ptr)?;
169189
new = Some((name.to_owned(), value.to_owned()));
170190
}
171191
}
172192
if let Some((name, value)) = new {
173-
let var_ptr = alloc_env_var_as_target_str(&name, &value, &mut this)?;
193+
let var_ptr = alloc_env_var_as_c_str(&name, &value, &mut this)?;
174194
if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) {
175195
this.memory
176196
.deallocate(var, None, MiriMemoryKind::Machine.into())?;
177197
}
178198
this.update_environ()?;
179199
Ok(0) // return zero on success
180200
} else {
201+
// name argument is a null pointer, points to an empty string, or points to a string containing an '=' character.
202+
let einval = this.eval_libc("EINVAL")?;
203+
this.set_last_error(einval)?;
181204
Ok(-1)
182205
}
183206
}
184207

185-
fn setenvironmentvariablew(
208+
fn SetEnvironmentVariableW(
186209
&mut self,
187210
name_op: OpTy<'tcx, Tag>, // LPCWSTR lpName,
188211
value_op: OpTy<'tcx, Tag>, // LPCWSTR lpValue,
@@ -193,42 +216,44 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
193216
let name_ptr = this.read_scalar(name_op)?.not_undef()?;
194217
let value_ptr = this.read_scalar(value_op)?.not_undef()?;
195218

196-
let mut new = None;
197-
if !this.is_null(name_ptr)? {
198-
let name = this.read_os_str_from_target_str(name_ptr)?;
199-
if this.is_null(value_ptr)? {
200-
// Delete environment variable `{name}`
201-
if let Some(var) = this.machine.env_vars.map.remove(&name) {
202-
this.memory.deallocate(var, None, MiriMemoryKind::Machine.into())?;
203-
this.update_environ()?;
204-
}
205-
return Ok(1); // return non-zero on success
206-
}
207-
if !name.is_empty() && !name.to_string_lossy().contains('=') {
208-
let value = this.read_os_str_from_target_str(value_ptr)?;
209-
new = Some((name.to_owned(), value.to_owned()));
210-
}
219+
if this.is_null(name_ptr)? {
220+
// ERROR CODE is not clearly explained in docs.. For now, throw UB instead.
221+
throw_ub_format!("Pointer to environment variable name is NULL");
211222
}
212-
if let Some((name, value)) = new {
213-
let var_ptr = alloc_env_var_as_target_str(&name, &value, &mut this)?;
223+
224+
let name = this.read_os_str_from_wide_str(name_ptr)?;
225+
if name.is_empty() {
226+
throw_unsup_format!("Environment variable name is an empty string");
227+
} else if name.to_string_lossy().contains('=') {
228+
throw_unsup_format!("Environment variable name contains '='");
229+
} else if this.is_null(value_ptr)? {
230+
// Delete environment variable `{name}`
231+
if let Some(var) = this.machine.env_vars.map.remove(&name) {
232+
this.memory.deallocate(var, None, MiriMemoryKind::Machine.into())?;
233+
this.update_environ()?;
234+
}
235+
Ok(1) // return non-zero on success
236+
} else {
237+
let value = this.read_os_str_from_wide_str(value_ptr)?;
238+
let var_ptr = alloc_env_var_as_wide_str(&name, &value, &mut this)?;
214239
if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) {
215240
this.memory
216241
.deallocate(var, None, MiriMemoryKind::Machine.into())?;
217242
}
218243
this.update_environ()?;
219244
Ok(1) // return non-zero on success
220-
} else {
221-
Ok(0)
222245
}
223246
}
224247

225248
fn unsetenv(&mut self, name_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> {
226249
let this = self.eval_context_mut();
250+
let target_os = this.tcx.sess.target.target.target_os.as_str();
251+
assert!(target_os == "linux" || target_os == "macos", "`unsetenv` is only available for the UNIX target family");
227252

228253
let name_ptr = this.read_scalar(name_op)?.not_undef()?;
229254
let mut success = None;
230255
if !this.is_null(name_ptr)? {
231-
let name = this.read_os_str_from_target_str(name_ptr)?.to_owned();
256+
let name = this.read_os_str_from_c_str(name_ptr)?.to_owned();
232257
if !name.is_empty() && !name.to_string_lossy().contains('=') {
233258
success = Some(this.machine.env_vars.map.remove(&name));
234259
}

‎src/shims/foreign_items/windows.rs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
2323

2424
// Environment related shims
2525
"GetEnvironmentVariableW" => {
26-
let result = this.getenvironmentvariablew(args[0], args[1], args[2])?;
26+
let result = this.GetEnvironmentVariableW(args[0], args[1], args[2])?;
2727
this.write_scalar(Scalar::from_uint(result, dest.layout.size), dest)?;
2828
}
2929

3030
"SetEnvironmentVariableW" => {
31-
let result = this.setenvironmentvariablew(args[0], args[1])?;
31+
let result = this.SetEnvironmentVariableW(args[0], args[1])?;
3232
this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?;
3333
}
3434

3535
"GetEnvironmentStringsW" => {
36-
let result = this.getenvironmentstringsw()?;
36+
let result = this.GetEnvironmentStringsW()?;
3737
// If the function succeeds, the return value is a pointer to the environment block of the current process.
3838
this.write_scalar(result, dest)?;
3939
}
4040

4141
"FreeEnvironmentStringsW" => {
42-
let old_vars_ptr = this.read_scalar(args[0])?.not_undef()?;
43-
let result = this.memory.deallocate(this.force_ptr(old_vars_ptr)?, None, MiriMemoryKind::WinHeap.into()).is_ok();
42+
let result = this.FreeEnvironmentStringsW(args[0])?;
4443
// If the function succeeds, the return value is nonzero.
4544
this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?;
4645
}

0 commit comments

Comments
 (0)