From 61da8b84282d4b44f4dd1742328867cae9e5cefc Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sun, 13 Oct 2019 15:26:03 -0500 Subject: [PATCH 01/10] Add OsString from/to bytes helper functions --- src/helpers.rs | 23 +++++++++++++++++++++++ src/shims/env.rs | 16 +++++----------- src/shims/fs.rs | 5 ++--- tests/compile-fail/chdir_invalid_path.rs | 11 ----------- 4 files changed, 30 insertions(+), 25 deletions(-) delete mode 100644 tests/compile-fail/chdir_invalid_path.rs diff --git a/src/helpers.rs b/src/helpers.rs index c09d5c823e..c2e4131a76 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -1,4 +1,5 @@ use std::mem; +use std::ffi::OsString; use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX}; use rustc::mir; @@ -345,3 +346,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(()) } } + + +pub fn bytes_to_os_string<'tcx>(bytes: Vec) -> InterpResult<'tcx, OsString> { + if cfg!(unix) { + Ok(std::os::unix::ffi::OsStringExt::from_vec(bytes)) + } else { + std::str::from_utf8(&bytes) + .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", bytes).into()) + .map(OsString::from) + } + } + +pub fn os_string_to_bytes<'tcx>(os_string: OsString) -> InterpResult<'tcx, Vec> { + if cfg!(unix) { + Ok(std::os::unix::ffi::OsStringExt::into_vec(os_string)) + } else { + os_string + .into_string() + .map_err(|os_string| err_unsup_format!("{:?} is not a valid utf-8 string", os_string).into()) + .map(|s| s.into_bytes()) + } + } diff --git a/src/shims/env.rs b/src/shims/env.rs index 6078ca26e2..1fe08aeffb 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -1,9 +1,9 @@ use std::collections::HashMap; use std::env; -use std::path::Path; use crate::stacked_borrows::Tag; use crate::*; + use rustc::ty::layout::Size; use rustc_mir::interpret::{Memory, Pointer}; @@ -128,7 +128,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx match env::current_dir() { Ok(cwd) => { // It is not clear what happens with non-utf8 paths here - let mut bytes = cwd.display().to_string().into_bytes(); + let mut bytes = helpers::os_string_to_bytes(cwd.into())?; // If `size` is smaller or equal than the `bytes.len()`, writing `bytes` plus the // required null terminator to memory using the `buf` pointer would cause an // overflow. The desired behavior in this case is to return null. @@ -156,16 +156,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("chdir")?; - let path_bytes = this - .memory - .read_c_str(this.read_scalar(path_op)?.not_undef()?)?; - - let path = Path::new( - std::str::from_utf8(path_bytes) - .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", path_bytes))?, - ); + let bytes = this.memory.read_c_str(this.read_scalar(path_op)?.not_undef()?)?; + let path = helpers::bytes_to_os_string(bytes.to_vec()); - match env::set_current_dir(path) { + match env::set_current_dir(path?) { Ok(()) => Ok(0), Err(e) => { this.consume_io_error(e)?; diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 891474bc3b..cc776295bd 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -94,11 +94,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx throw_unsup_format!("unsupported flags {:#x}", flag & !mirror); } - let path_bytes = this + let bytes = this .memory .read_c_str(this.read_scalar(path_op)?.not_undef()?)?; - let path = std::str::from_utf8(path_bytes) - .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", path_bytes))?; + let path: std::path::PathBuf = helpers::bytes_to_os_string(bytes.to_vec())?.into(); let fd = options.open(path).map(|file| { let mut fh = &mut this.machine.file_handler; diff --git a/tests/compile-fail/chdir_invalid_path.rs b/tests/compile-fail/chdir_invalid_path.rs deleted file mode 100644 index 22b0d723aa..0000000000 --- a/tests/compile-fail/chdir_invalid_path.rs +++ /dev/null @@ -1,11 +0,0 @@ -// compile-flags: -Zmiri-disable-isolation - -extern { - pub fn chdir(dir: *const u8) -> i32; -} - -fn main() { - let path = vec![0xc3u8, 0x28, 0]; - // test that `chdir` errors with invalid utf-8 path - unsafe { chdir(path.as_ptr()) }; //~ ERROR is not a valid utf-8 string -} From 1241abbec473827acf8dbbfff8f79fe08117b967 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 17 Oct 2019 10:21:06 -0500 Subject: [PATCH 02/10] Change helper functions to read/write --- src/helpers.rs | 32 ++++++++++++++++++++++++-------- src/shims/env.rs | 20 +++----------------- src/shims/fs.rs | 5 +---- 3 files changed, 28 insertions(+), 29 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index c2e4131a76..c384b89742 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -345,10 +345,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Ok(()) } -} - -pub fn bytes_to_os_string<'tcx>(bytes: Vec) -> InterpResult<'tcx, OsString> { + fn read_os_string(&mut self, scalar: Scalar) -> InterpResult<'tcx, OsString> { + let bytes = self.eval_context_mut().memory.read_c_str(scalar)?.to_vec(); if cfg!(unix) { Ok(std::os::unix::ffi::OsStringExt::from_vec(bytes)) } else { @@ -358,13 +357,30 @@ pub fn bytes_to_os_string<'tcx>(bytes: Vec) -> InterpResult<'tcx, OsString> } } -pub fn os_string_to_bytes<'tcx>(os_string: OsString) -> InterpResult<'tcx, Vec> { - if cfg!(unix) { - Ok(std::os::unix::ffi::OsStringExt::into_vec(os_string)) + fn write_os_string(&mut self, os_string: OsString, ptr: Pointer, size: u64) -> InterpResult<'tcx> { + let mut bytes = if cfg!(unix) { + std::os::unix::ffi::OsStringExt::into_vec(os_string) } else { os_string .into_string() - .map_err(|os_string| err_unsup_format!("{:?} is not a valid utf-8 string", os_string).into()) - .map(|s| s.into_bytes()) + .map_err(|os_string| err_unsup_format!("{:?} is not a valid utf-8 string", os_string))? + .into_bytes() + }; + // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null + // terminator to memory using the `ptr` pointer would cause an overflow. + if (bytes.len() as u64) < size { + // We add a `/0` terminator + bytes.push(0); + let this = self.eval_context_mut(); + let tcx = &{ this.tcx.tcx }; + // This is ok because the buffer was strictly larger than `bytes`, so after adding the + // null terminator, the buffer size is larger or equal to `bytes.len()`, meaning that + // `bytes` actually fit inside tbe buffer. + this.memory + .get_mut(ptr.alloc_id)? + .write_bytes(tcx, ptr, &bytes) + } else { + throw_unsup_format!("OsString is larger than destination") } } +} diff --git a/src/shims/env.rs b/src/shims/env.rs index 1fe08aeffb..b344a5c549 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -127,20 +127,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // If we cannot get the current directory, we return null match env::current_dir() { Ok(cwd) => { - // It is not clear what happens with non-utf8 paths here - let mut bytes = helpers::os_string_to_bytes(cwd.into())?; - // If `size` is smaller or equal than the `bytes.len()`, writing `bytes` plus the - // required null terminator to memory using the `buf` pointer would cause an - // overflow. The desired behavior in this case is to return null. - if (bytes.len() as u64) < size { - // We add a `/0` terminator - bytes.push(0); - // This is ok because the buffer was strictly larger than `bytes`, so after - // adding the null terminator, the buffer size is larger or equal to - // `bytes.len()`, meaning that `bytes` actually fit inside tbe buffer. - this.memory - .get_mut(buf.alloc_id)? - .write_bytes(&*this.tcx, buf, &bytes)?; + if this.write_os_string(cwd.into(), buf, size).is_ok() { return Ok(Scalar::Ptr(buf)); } let erange = this.eval_libc("ERANGE")?; @@ -156,10 +143,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("chdir")?; - let bytes = this.memory.read_c_str(this.read_scalar(path_op)?.not_undef()?)?; - let path = helpers::bytes_to_os_string(bytes.to_vec()); + let path = this.read_os_string(this.read_scalar(path_op)?.not_undef()?)?; - match env::set_current_dir(path?) { + match env::set_current_dir(path) { Ok(()) => Ok(0), Err(e) => { this.consume_io_error(e)?; diff --git a/src/shims/fs.rs b/src/shims/fs.rs index cc776295bd..d4f9fde24e 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -94,10 +94,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx throw_unsup_format!("unsupported flags {:#x}", flag & !mirror); } - let bytes = this - .memory - .read_c_str(this.read_scalar(path_op)?.not_undef()?)?; - let path: std::path::PathBuf = helpers::bytes_to_os_string(bytes.to_vec())?.into(); + let path: std::path::PathBuf = this.read_os_string(this.read_scalar(path_op)?.not_undef()?)?.into(); let fd = options.open(path).map(|file| { let mut fh = &mut this.machine.file_handler; From 68fec4b3fe3a86a01e39d31d05a8438c07845aba Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 17 Oct 2019 21:20:05 -0500 Subject: [PATCH 03/10] Use conditional compilation properly and work with `OsStr`s instead --- src/helpers.rs | 60 +++++++++++++++++++++++++++++++----------------- src/shims/env.rs | 3 ++- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index c384b89742..32edb107f3 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -1,5 +1,5 @@ use std::mem; -use std::ffi::OsString; +use std::ffi::{OsStr, OsString}; use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX}; use rustc::mir; @@ -347,30 +347,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } fn read_os_string(&mut self, scalar: Scalar) -> InterpResult<'tcx, OsString> { - let bytes = self.eval_context_mut().memory.read_c_str(scalar)?.to_vec(); - if cfg!(unix) { - Ok(std::os::unix::ffi::OsStringExt::from_vec(bytes)) - } else { - std::str::from_utf8(&bytes) - .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", bytes).into()) - .map(OsString::from) - } + let bytes = self.eval_context_mut().memory.read_c_str(scalar)?; + Ok(bytes_to_os_str(bytes)?.into()) } - fn write_os_string(&mut self, os_string: OsString, ptr: Pointer, size: u64) -> InterpResult<'tcx> { - let mut bytes = if cfg!(unix) { - std::os::unix::ffi::OsStringExt::into_vec(os_string) - } else { - os_string - .into_string() - .map_err(|os_string| err_unsup_format!("{:?} is not a valid utf-8 string", os_string))? - .into_bytes() - }; + fn write_os_str(&mut self, os_str: &OsStr, ptr: Pointer, size: u64) -> InterpResult<'tcx> { + let bytes = os_str_to_bytes(os_str)?; // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null // terminator to memory using the `ptr` pointer would cause an overflow. if (bytes.len() as u64) < size { - // We add a `/0` terminator - bytes.push(0); let this = self.eval_context_mut(); let tcx = &{ this.tcx.tcx }; // This is ok because the buffer was strictly larger than `bytes`, so after adding the @@ -378,9 +363,42 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // `bytes` actually fit inside tbe buffer. this.memory .get_mut(ptr.alloc_id)? - .write_bytes(tcx, ptr, &bytes) + .write_bytes(tcx, ptr, &bytes)?; + // We write the `/0` terminator + let tail_ptr = ptr.offset(Size::from_bytes(bytes.len() as u64 + 1), this)?; + this.memory + .get_mut(ptr.alloc_id)? + .write_bytes(tcx, tail_ptr, b"0") } else { throw_unsup_format!("OsString is larger than destination") } } } + +#[cfg(target_os = "unix")] +fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> { + Ok(std::os::unix::ffi::OsStringExt::from_bytes(bytes)) +} + +#[cfg(target_os = "unix")] +fn os_str_to_bytes<'tcx, 'a>(os_str: &'a OsStr) -> InterpResult<'tcx, &'a [u8]> { + std::os::unix::ffi::OsStringExt::into_bytes(os_str) +} + +// On non-unix platforms the best we can do to transform bytes from/to OS strings is to do the +// intermediate transformation into strings. Which invalidates non-utf8 paths that are actually +// valid. +#[cfg(not(target_os = "unix"))] +fn os_str_to_bytes<'tcx, 'a>(os_str: &'a OsStr) -> InterpResult<'tcx, &'a [u8]> { + os_str + .to_str() + .map(|s| s.as_bytes()) + .ok_or_else(|| err_unsup_format!("{:?} is not a valid utf-8 string", os_str).into()) +} + +#[cfg(not(target_os = "unix"))] +fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> { + let s = std::str::from_utf8(bytes) + .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", bytes))?; + Ok(&OsStr::new(s)) +} diff --git a/src/shims/env.rs b/src/shims/env.rs index b344a5c549..b2e0709557 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -1,4 +1,5 @@ use std::collections::HashMap; +use std::ffi::OsString; use std::env; use crate::stacked_borrows::Tag; @@ -127,7 +128,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // If we cannot get the current directory, we return null match env::current_dir() { Ok(cwd) => { - if this.write_os_string(cwd.into(), buf, size).is_ok() { + if this.write_os_str(&OsString::from(cwd), buf, size).is_ok() { return Ok(Scalar::Ptr(buf)); } let erange = this.eval_libc("ERANGE")?; From 85941c72497e69653b847744a447ae51b433d4ba Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 18 Oct 2019 09:30:12 -0500 Subject: [PATCH 04/10] Rename write/read os string functions --- src/helpers.rs | 4 ++-- src/shims/env.rs | 4 ++-- src/shims/fs.rs | 8 ++------ 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 32edb107f3..454f7d2c2f 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -346,12 +346,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(()) } - fn read_os_string(&mut self, scalar: Scalar) -> InterpResult<'tcx, OsString> { + fn read_os_string_from_c_string(&mut self, scalar: Scalar) -> InterpResult<'tcx, OsString> { let bytes = self.eval_context_mut().memory.read_c_str(scalar)?; Ok(bytes_to_os_str(bytes)?.into()) } - fn write_os_str(&mut self, os_str: &OsStr, ptr: Pointer, size: u64) -> InterpResult<'tcx> { + fn write_os_str_to_c_string(&mut self, os_str: &OsStr, ptr: Pointer, size: u64) -> InterpResult<'tcx> { let bytes = os_str_to_bytes(os_str)?; // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null // terminator to memory using the `ptr` pointer would cause an overflow. diff --git a/src/shims/env.rs b/src/shims/env.rs index b2e0709557..7e2df4f985 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -128,7 +128,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // If we cannot get the current directory, we return null match env::current_dir() { Ok(cwd) => { - if this.write_os_str(&OsString::from(cwd), buf, size).is_ok() { + if this.write_os_str_to_c_string(&OsString::from(cwd), buf, size).is_ok() { return Ok(Scalar::Ptr(buf)); } let erange = this.eval_libc("ERANGE")?; @@ -144,7 +144,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("chdir")?; - let path = this.read_os_string(this.read_scalar(path_op)?.not_undef()?)?; + let path = this.read_os_string_from_c_string(this.read_scalar(path_op)?.not_undef()?)?; match env::set_current_dir(path) { Ok(()) => Ok(0), diff --git a/src/shims/fs.rs b/src/shims/fs.rs index d4f9fde24e..7cc574f05f 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -94,7 +94,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx throw_unsup_format!("unsupported flags {:#x}", flag & !mirror); } - let path: std::path::PathBuf = this.read_os_string(this.read_scalar(path_op)?.not_undef()?)?.into(); + let path: std::path::PathBuf = this.read_os_string_from_c_string(this.read_scalar(path_op)?.not_undef()?)?.into(); let fd = options.open(path).map(|file| { let mut fh = &mut this.machine.file_handler; @@ -210,11 +210,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("unlink")?; - let path_bytes = this - .memory - .read_c_str(this.read_scalar(path_op)?.not_undef()?)?; - let path = std::str::from_utf8(path_bytes) - .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", path_bytes))?; + let path = this.read_os_string_from_c_string(this.read_scalar(path_op)?.not_undef()?)?; let result = remove_file(path).map(|_| 0); From 0201cc55873dd27f0be148482ad3619083fed80b Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 18 Oct 2019 14:25:49 -0500 Subject: [PATCH 05/10] Fix writing errors --- src/helpers.rs | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 454f7d2c2f..c975ad144e 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -353,22 +353,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn write_os_str_to_c_string(&mut self, os_str: &OsStr, ptr: Pointer, size: u64) -> InterpResult<'tcx> { let bytes = os_str_to_bytes(os_str)?; + let len = bytes.len(); // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null // terminator to memory using the `ptr` pointer would cause an overflow. - if (bytes.len() as u64) < size { + if (len as u64) < size { let this = self.eval_context_mut(); let tcx = &{ this.tcx.tcx }; + let buffer = this.memory.get_mut(ptr.alloc_id)?.get_bytes_mut(tcx, ptr, Size::from_bytes(len as u64 + 1))?; + buffer[..len].copy_from_slice(bytes); // This is ok because the buffer was strictly larger than `bytes`, so after adding the // null terminator, the buffer size is larger or equal to `bytes.len()`, meaning that // `bytes` actually fit inside tbe buffer. - this.memory - .get_mut(ptr.alloc_id)? - .write_bytes(tcx, ptr, &bytes)?; - // We write the `/0` terminator - let tail_ptr = ptr.offset(Size::from_bytes(bytes.len() as u64 + 1), this)?; - this.memory - .get_mut(ptr.alloc_id)? - .write_bytes(tcx, tail_ptr, b"0") + buffer[len] = 0; + Ok(()) } else { throw_unsup_format!("OsString is larger than destination") } @@ -376,13 +373,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } #[cfg(target_os = "unix")] -fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> { - Ok(std::os::unix::ffi::OsStringExt::from_bytes(bytes)) +fn os_str_to_bytes<'tcx, 'a>(os_str: &'a OsStr) -> InterpResult<'tcx, &'a [u8]> { + std::os::unix::ffi::OsStringExt::into_bytes(os_str) } #[cfg(target_os = "unix")] -fn os_str_to_bytes<'tcx, 'a>(os_str: &'a OsStr) -> InterpResult<'tcx, &'a [u8]> { - std::os::unix::ffi::OsStringExt::into_bytes(os_str) +fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> { + Ok(std::os::unix::ffi::OsStringExt::from_bytes(bytes)) } // On non-unix platforms the best we can do to transform bytes from/to OS strings is to do the From ab059671cb78016bb039b3db07c62279d37ef42b Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sat, 19 Oct 2019 14:13:49 -0500 Subject: [PATCH 06/10] Change comparison order for clarity --- src/helpers.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index c975ad144e..ddaf8f48a5 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -356,19 +356,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let len = bytes.len(); // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null // terminator to memory using the `ptr` pointer would cause an overflow. - if (len as u64) < size { - let this = self.eval_context_mut(); - let tcx = &{ this.tcx.tcx }; - let buffer = this.memory.get_mut(ptr.alloc_id)?.get_bytes_mut(tcx, ptr, Size::from_bytes(len as u64 + 1))?; - buffer[..len].copy_from_slice(bytes); - // This is ok because the buffer was strictly larger than `bytes`, so after adding the - // null terminator, the buffer size is larger or equal to `bytes.len()`, meaning that - // `bytes` actually fit inside tbe buffer. - buffer[len] = 0; - Ok(()) - } else { - throw_unsup_format!("OsString is larger than destination") + if size <= bytes.len() as u64 { + throw_unsup_format!("OsString of length {} is too large for destination buffer of size {}", len, size) } + + let this = self.eval_context_mut(); + let buffer = this.memory.get_mut(ptr.alloc_id)?.get_bytes_mut(&*this.tcx, ptr, Size::from_bytes(len as u64 + 1))?; + buffer[..len].copy_from_slice(bytes); + // This is ok because the buffer was strictly larger than `bytes`, so after adding the + // null terminator, the buffer size is larger or equal to `bytes.len()`, meaning that + // `bytes` actually fit inside tbe buffer. + buffer[len] = 0; + Ok(()) } } From f7c6e0efbe11343562c713dce1d28e2d141dcb89 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sat, 19 Oct 2019 15:49:00 -0500 Subject: [PATCH 07/10] Do additional bounds checks --- src/helpers.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index ddaf8f48a5..63380199be 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -356,12 +356,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let len = bytes.len(); // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null // terminator to memory using the `ptr` pointer would cause an overflow. - if size <= bytes.len() as u64 { + if size <= len as u64 { throw_unsup_format!("OsString of length {} is too large for destination buffer of size {}", len, size) } - + let actual_len = (len as u64) + .checked_add(1) + .map(Size::from_bytes) + .ok_or_else(|| err_unsup_format!("OsString of length {} is too large", len))?; let this = self.eval_context_mut(); - let buffer = this.memory.get_mut(ptr.alloc_id)?.get_bytes_mut(&*this.tcx, ptr, Size::from_bytes(len as u64 + 1))?; + this.memory.check_ptr_access(ptr.into(), actual_len, Align::from_bytes(1).unwrap())?; + let buffer = this.memory.get_mut(ptr.alloc_id)?.get_bytes_mut(&*this.tcx, ptr, actual_len)?; buffer[..len].copy_from_slice(bytes); // This is ok because the buffer was strictly larger than `bytes`, so after adding the // null terminator, the buffer size is larger or equal to `bytes.len()`, meaning that From 283a130ddafa01537123b0650f869abc14886911 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sun, 20 Oct 2019 17:40:21 -0500 Subject: [PATCH 08/10] Add docs for the new helper functions --- src/helpers.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/helpers.rs b/src/helpers.rs index 63380199be..4d84106dbe 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -346,11 +346,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(()) } + /// Helper function to read an OsString from a null-terminated sequence of bytes, which is what + /// the Unix APIs usually handle. fn read_os_string_from_c_string(&mut self, scalar: Scalar) -> InterpResult<'tcx, OsString> { let bytes = self.eval_context_mut().memory.read_c_str(scalar)?; Ok(bytes_to_os_str(bytes)?.into()) } + /// Helper function to write an OsStr as a null-terminated sequence of bytes, which is what + /// the Unix APIs usually handle. fn write_os_str_to_c_string(&mut self, os_str: &OsStr, ptr: Pointer, size: u64) -> InterpResult<'tcx> { let bytes = os_str_to_bytes(os_str)?; let len = bytes.len(); From fb4cb5bf4af633ca057a84b995bab3b08333df41 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 22 Oct 2019 16:57:07 -0500 Subject: [PATCH 09/10] Make size error distinguishable from other errors --- src/helpers.rs | 17 +++++++++++++---- src/shims/env.rs | 2 +- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index b31e0a2c2e..79efd95eb6 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -412,16 +412,25 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } /// Helper function to write an OsStr as a null-terminated sequence of bytes, which is what - /// the Unix APIs usually handle. - fn write_os_str_to_c_string(&mut self, os_str: &OsStr, scalar: Scalar, size: u64) -> InterpResult<'tcx> { + /// the Unix APIs usually handle. This function returns `Ok(false)` without trying to write if + /// `size` is not large enough to fit the contents of `os_string` plus a null terminator. It + /// returns `Ok(true)` if the writing process was successful. Otherwise it returns an + /// `InterpError`. + fn write_os_str_to_c_string( + &mut self, + os_str: &OsStr, + scalar: Scalar, + size: u64 + ) -> InterpResult<'tcx, bool> { let bytes = os_str_to_bytes(os_str)?; // If `size` is smaller or equal than `bytes.len()`, writing `bytes` plus the required null // terminator to memory using the `ptr` pointer would cause an overflow. if size <= bytes.len() as u64 { - throw_unsup_format!("OsString of length {} is too large for destination buffer of size {}", bytes.len(), size) + return Ok(false); } // FIXME: We should use `Iterator::chain` instead when rust-lang/rust#65704 lands. - self.eval_context_mut().memory.write_bytes(scalar, [bytes, &[0]].concat()) + self.eval_context_mut().memory.write_bytes(scalar, [bytes, &[0]].concat())?; + Ok(true) } } diff --git a/src/shims/env.rs b/src/shims/env.rs index f88a5bbac7..2dc47d74ff 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -128,7 +128,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // If we cannot get the current directory, we return null match env::current_dir() { Ok(cwd) => { - if this.write_os_str_to_c_string(&OsString::from(cwd), buf, size).is_ok() { + if this.write_os_str_to_c_string(&OsString::from(cwd), buf, size)? { return Ok(buf); } let erange = this.eval_libc("ERANGE")?; From de12cbcb32a53a06e1f73e7d9cdfa11e41d79ded Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 23 Oct 2019 08:58:25 -0500 Subject: [PATCH 10/10] Fix documentation --- src/helpers.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 79efd95eb6..0245540deb 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -414,8 +414,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// Helper function to write an OsStr as a null-terminated sequence of bytes, which is what /// the Unix APIs usually handle. This function returns `Ok(false)` without trying to write if /// `size` is not large enough to fit the contents of `os_string` plus a null terminator. It - /// returns `Ok(true)` if the writing process was successful. Otherwise it returns an - /// `InterpError`. + /// returns `Ok(true)` if the writing process was successful. fn write_os_str_to_c_string( &mut self, os_str: &OsStr,