From 260b463bb037809e9558421ad5e73bf89e9a08df Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Thu, 24 Oct 2019 08:44:13 -0500 Subject: [PATCH 01/11] Clean file handling functions --- src/shims/fs.rs | 120 +++++++++++++++++++----------------------------- 1 file changed, 46 insertions(+), 74 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index c484795d8f..760fc1daff 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -1,5 +1,5 @@ use std::collections::HashMap; -use std::fs::{File, OpenOptions, remove_file}; +use std::fs::{remove_file, File, OpenOptions}; use std::io::{Read, Write}; use rustc::ty::layout::Size; @@ -125,8 +125,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // `FD_CLOEXEC` value without checking if the flag is set for the file because `std` // always sets this flag when opening a file. However we still need to check that the // file itself is open. - let fd_cloexec = this.eval_libc_i32("FD_CLOEXEC")?; - this.get_handle_and(fd, |_| Ok(fd_cloexec)) + if this.machine.file_handler.handles.contains_key(&fd) { + Ok(this.eval_libc_i32("FD_CLOEXEC")?) + } else { + this.handle_not_found() + } } else { throw_unsup_format!("The {:#x} command is not supported for `fcntl`)", cmd); } @@ -139,9 +142,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; - this.remove_handle_and(fd, |handle, this| { + if let Some(handle) = this.machine.file_handler.handles.remove(&fd) { this.try_unwrap_io_result(handle.file.sync_all().map(|_| 0i32)) - }) + } else { + this.handle_not_found() + } } fn read( @@ -160,20 +165,24 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(0); } let fd = this.read_scalar(fd_op)?.to_i32()?; - let buf_scalar = this.read_scalar(buf_op)?.not_undef()?; - - // Remove the file handle to avoid borrowing issues. - this.remove_handle_and(fd, |mut handle, this| { - // Don't use `?` to avoid returning before reinserting the handle. - let bytes = this.force_ptr(buf_scalar).and_then(|buf| { - this.memory - .get_mut(buf.alloc_id)? - .get_bytes_mut(&*this.tcx, buf, Size::from_bytes(count)) - .map(|buffer| handle.file.read(buffer)) - }); - this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); - this.try_unwrap_io_result(bytes?.map(|bytes| bytes as i64)) - }) + let buf = this.read_scalar(buf_op)?.not_undef()?; + + if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { + // We want to read at most `count` bytes + let mut bytes = vec![0; count as usize]; + let result = handle.file.read(&mut bytes).map(|c| c as i64); + let written_count = this.try_unwrap_io_result(result)?; + // `try_unwrap_io_result` returns Ok(`-1`) if `result` is an error. There is no other + // way of returning `-1` because the `Ok` variant of `result` contains the number of + // written bytes, which is a possitive value. + if written_count != -1 { + // If reading to `bytes` did not fail, we write those bytes to the buffer. + this.memory.write_bytes(buf, bytes)?; + } + Ok(written_count) + } else { + this.handle_not_found() + } } fn write( @@ -192,20 +201,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(0); } let fd = this.read_scalar(fd_op)?.to_i32()?; - let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; - - this.remove_handle_and(fd, |mut handle, this| { - let bytes = this.memory.get(buf.alloc_id).and_then(|alloc| { - alloc - .get_bytes(&*this.tcx, buf, Size::from_bytes(count)) - .map(|bytes| handle.file.write(bytes).map(|bytes| bytes as i64)) - }); - this.machine.file_handler.handles.insert(fd, handle).unwrap_none(); - this.try_unwrap_io_result(bytes?) - }) + let buf = this.read_scalar(buf_op)?.not_undef()?; + + if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { + let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; + let result = handle.file.write(&bytes).map(|c| c as i64); + this.try_unwrap_io_result(result) + } else { + this.handle_not_found() + } } - fn unlink( &mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { + fn unlink(&mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); this.check_no_isolation("unlink")?; @@ -217,49 +224,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.try_unwrap_io_result(result) } - /// Helper function that gets a `FileHandle` immutable reference and allows to manipulate it - /// using the `f` closure. - /// - /// If the `fd` file descriptor does not correspond to a file, this functions returns `Ok(-1)` - /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). - /// - /// This function uses `T: From<i32>` instead of `i32` directly because some IO related - /// functions return different integer types (like `read`, that returns an `i64`). - fn get_handle_and<F, T: From<i32>>(&mut self, fd: i32, f: F) -> InterpResult<'tcx, T> - where - F: Fn(&FileHandle) -> InterpResult<'tcx, T>, - { + /// Function used when a handle is not found inside `FileHandler`. It returns `Ok(-1)`and sets + /// the last OS error to `libc::EBADF` (invalid file descriptor). This function uses + /// `T: From<i32>` instead of `i32` directly because some fs functions return different integer + /// types (like `read`, that returns an `i64`). + fn handle_not_found<T: From<i32>>(&mut self) -> InterpResult<'tcx, T> { let this = self.eval_context_mut(); - if let Some(handle) = this.machine.file_handler.handles.get(&fd) { - f(handle) - } else { - let ebadf = this.eval_libc("EBADF")?; - this.set_last_error(ebadf)?; - Ok((-1).into()) - } - } - - /// Helper function that removes a `FileHandle` and allows to manipulate it using the `f` - /// closure. This function is quite useful when you need to modify a `FileHandle` but you need - /// to modify `MiriEvalContext` at the same time, so you can modify the handle and reinsert it - /// using `f`. - /// - /// If the `fd` file descriptor does not correspond to a file, this functions returns `Ok(-1)` - /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). - /// - /// This function uses `T: From<i32>` instead of `i32` directly because some IO related - /// functions return different integer types (like `read`, that returns an `i64`). - fn remove_handle_and<F, T: From<i32>>(&mut self, fd: i32, mut f: F) -> InterpResult<'tcx, T> - where - F: FnMut(FileHandle, &mut MiriEvalContext<'mir, 'tcx>) -> InterpResult<'tcx, T>, - { - let this = self.eval_context_mut(); - if let Some(handle) = this.machine.file_handler.handles.remove(&fd) { - f(handle, this) - } else { - let ebadf = this.eval_libc("EBADF")?; - this.set_last_error(ebadf)?; - Ok((-1).into()) - } + let ebadf = this.eval_libc("EBADF")?; + this.set_last_error(ebadf)?; + Ok((-1).into()) } } From d7967f6b058b3dbf8608a76b0fbe165585a2b77c Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Sat, 26 Oct 2019 08:54:02 -0500 Subject: [PATCH 02/11] Drop files explicitly when closing them --- src/shims/fs.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 760fc1daff..c57ec1a1f9 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -143,7 +143,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; if let Some(handle) = this.machine.file_handler.handles.remove(&fd) { - this.try_unwrap_io_result(handle.file.sync_all().map(|_| 0i32)) + // `File::sync_all` does the checks that are done when closing a file. We do this to + // to handle possible errors correctly. + let result = this.try_unwrap_io_result(handle.file.sync_all().map(|_| 0i32)); + // Now we actually drop the handle. + drop(handle); + // And return the result. + result } else { this.handle_not_found() } From 122549fd0959634d0255ba40be6c994e9e60d791 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Sat, 26 Oct 2019 09:03:45 -0500 Subject: [PATCH 03/11] Simplify `read` logic --- src/shims/fs.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index c57ec1a1f9..e8c42eb9a3 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -177,15 +177,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // We want to read at most `count` bytes let mut bytes = vec![0; count as usize]; let result = handle.file.read(&mut bytes).map(|c| c as i64); - let written_count = this.try_unwrap_io_result(result)?; - // `try_unwrap_io_result` returns Ok(`-1`) if `result` is an error. There is no other - // way of returning `-1` because the `Ok` variant of `result` contains the number of - // written bytes, which is a possitive value. - if written_count != -1 { - // If reading to `bytes` did not fail, we write those bytes to the buffer. + // If reading to `bytes` did not fail, we write those bytes to the buffer. + if result.is_ok() { this.memory.write_bytes(buf, bytes)?; } - Ok(written_count) + this.try_unwrap_io_result(result) } else { this.handle_not_found() } From 06ef77bfef45b3eb01ab4c3ceb0fa30186de4ea0 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Mon, 28 Oct 2019 16:44:18 -0500 Subject: [PATCH 04/11] Check for usize to i64 overflows --- src/shims/fs.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index e8c42eb9a3..2710cbacb0 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -176,12 +176,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { // We want to read at most `count` bytes let mut bytes = vec![0; count as usize]; - let result = handle.file.read(&mut bytes).map(|c| c as i64); - // If reading to `bytes` did not fail, we write those bytes to the buffer. - if result.is_ok() { - this.memory.write_bytes(buf, bytes)?; + let result = handle.file.read(&mut bytes); + + if let Ok(c) = result { + // Check that we read less than `i64::MAX` bytes. + if c > (i64::max_value() as usize) { + throw_unsup_format!("Number of read bytes {} is larger than the maximum value", c); + } + // If reading to `bytes` did not fail, we write those bytes to the buffer. + this.memory.write_bytes(buf, bytes)? } - this.try_unwrap_io_result(result) + + this.try_unwrap_io_result(result.map(|c| c as i64)) } else { this.handle_not_found() } @@ -207,8 +213,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; - let result = handle.file.write(&bytes).map(|c| c as i64); - this.try_unwrap_io_result(result) + let result = handle.file.write(&bytes); + + if let Ok(c) = result { + // Check that we wrote less than `i64::MAX` bytes. + if c > (i64::max_value() as usize) { + throw_unsup_format!("Number of written bytes {} is larger than the maximum value", c); + } + } + + this.try_unwrap_io_result(result.map(|c| c as i64)) } else { this.handle_not_found() } From d0b4407b258acdf87a2b44b687564468b115071a Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Sun, 3 Nov 2019 10:04:00 -0600 Subject: [PATCH 05/11] Fix casts for `count` check --- src/shims/fs.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 2710cbacb0..66e24eb8b0 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -146,7 +146,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // `File::sync_all` does the checks that are done when closing a file. We do this to // to handle possible errors correctly. let result = this.try_unwrap_io_result(handle.file.sync_all().map(|_| 0i32)); - // Now we actually drop the handle. + // Now we actually close the file. drop(handle); // And return the result. result @@ -180,7 +180,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Ok(c) = result { // Check that we read less than `i64::MAX` bytes. - if c > (i64::max_value() as usize) { + if (c as u64) > (i64::max_value() as u64) { throw_unsup_format!("Number of read bytes {} is larger than the maximum value", c); } // If reading to `bytes` did not fail, we write those bytes to the buffer. @@ -217,7 +217,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Ok(c) = result { // Check that we wrote less than `i64::MAX` bytes. - if c > (i64::max_value() as usize) { + if (c as u64) > (i64::max_value() as u64) { throw_unsup_format!("Number of written bytes {} is larger than the maximum value", c); } } From 56c5e53553859d153009f76d2da15bf2d0e3d037 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Mon, 4 Nov 2019 09:38:21 -0500 Subject: [PATCH 06/11] Handle host's `usize` correctly --- src/helpers.rs | 14 ++++++++++++++ src/shims/fs.rs | 38 ++++++++++++++++++++++++-------------- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index dd0530cf48..c02ba3f1fa 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -1,5 +1,6 @@ use std::{mem, iter}; use std::ffi::{OsStr, OsString}; +use std::convert::TryFrom; use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX}; use rustc::mir; @@ -459,3 +460,16 @@ fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> { .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", bytes))?; Ok(&OsStr::new(s)) } + +pub fn try_into_host_usize(i: impl Into<u128>) -> Option<usize> { + let i: u128 = i.into(); + if i > usize::max_value() as u128 { + None + } else { + Some(i as usize) + } +} + +pub fn try_from_host_usize<T: TryFrom<u128>>(i: usize) -> Option<T> { + T::try_from(i as u128).ok() +} diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 66e24eb8b0..910e390769 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -178,16 +178,21 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let mut bytes = vec![0; count as usize]; let result = handle.file.read(&mut bytes); - if let Ok(c) = result { - // Check that we read less than `i64::MAX` bytes. - if (c as u64) > (i64::max_value() as u64) { - throw_unsup_format!("Number of read bytes {} is larger than the maximum value", c); + match result { + Ok(c) => { + if let Some(read_bytes) = helpers::try_from_host_usize::<i64>(c) { + // If reading to `bytes` did not fail, we write those bytes to the buffer. + this.memory.write_bytes(buf, bytes)?; + Ok(read_bytes) + } else { + throw_unsup_format!("Number of read bytes {} cannot be transformed to i64", c); + } + }, + Err(e) => { + this.set_last_error_from_io_error(e)?; + Ok(-1) } - // If reading to `bytes` did not fail, we write those bytes to the buffer. - this.memory.write_bytes(buf, bytes)? } - - this.try_unwrap_io_result(result.map(|c| c as i64)) } else { this.handle_not_found() } @@ -215,14 +220,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; let result = handle.file.write(&bytes); - if let Ok(c) = result { - // Check that we wrote less than `i64::MAX` bytes. - if (c as u64) > (i64::max_value() as u64) { - throw_unsup_format!("Number of written bytes {} is larger than the maximum value", c); + match result { + Ok(c) => { + if let Some(written_bytes) = helpers::try_from_host_usize::<i64>(c) { + Ok(written_bytes) + } else { + throw_unsup_format!("Number of written bytes {} cannot be transformed to i64", c); + } + }, + Err(e) => { + this.set_last_error_from_io_error(e)?; + Ok(-1) } } - - this.try_unwrap_io_result(result.map(|c| c as i64)) } else { this.handle_not_found() } From f910ea135c0cbfec4dd10adf2a79163e1b8935f3 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Tue, 5 Nov 2019 16:47:24 -0500 Subject: [PATCH 07/11] Avoid using `as` cast --- src/shims/fs.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 910e390769..3fccbaa33f 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -174,19 +174,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.read_scalar(buf_op)?.not_undef()?; if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { + let count = helpers::try_into_host_usize(count) + .ok_or_else(|| err_unsup_format!("Program tries to read into buffer too big for this host platform"))?; // We want to read at most `count` bytes - let mut bytes = vec![0; count as usize]; + let mut bytes = vec![0; count]; let result = handle.file.read(&mut bytes); match result { Ok(c) => { - if let Some(read_bytes) = helpers::try_from_host_usize::<i64>(c) { - // If reading to `bytes` did not fail, we write those bytes to the buffer. - this.memory.write_bytes(buf, bytes)?; - Ok(read_bytes) - } else { - throw_unsup_format!("Number of read bytes {} cannot be transformed to i64", c); - } + let read_bytes = helpers::try_from_host_usize::<i64>(c) + .ok_or_else(|| err_unsup_format!("Number of read bytes {} cannot be transformed to i64", c))?; + // If reading to `bytes` did not fail, we write those bytes to the buffer. + this.memory.write_bytes(buf, bytes)?; + Ok(read_bytes) }, Err(e) => { this.set_last_error_from_io_error(e)?; @@ -221,13 +221,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = handle.file.write(&bytes); match result { - Ok(c) => { - if let Some(written_bytes) = helpers::try_from_host_usize::<i64>(c) { - Ok(written_bytes) - } else { - throw_unsup_format!("Number of written bytes {} cannot be transformed to i64", c); - } - }, + Ok(c) => helpers::try_from_host_usize::<i64>(c) + .ok_or_else(|| err_unsup_format!("Number of written bytes {} cannot be transformed to i64", c).into()), Err(e) => { this.set_last_error_from_io_error(e)?; Ok(-1) From 4bbaa72dc92ca385b2595f6a072b96f36fa4a1ef Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Thu, 7 Nov 2019 20:50:16 +0100 Subject: [PATCH 08/11] Use TryFrom instead --- src/helpers.rs | 14 -------------- src/shims/fs.rs | 19 +++++++++++-------- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index c02ba3f1fa..dd0530cf48 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -1,6 +1,5 @@ use std::{mem, iter}; use std::ffi::{OsStr, OsString}; -use std::convert::TryFrom; use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX}; use rustc::mir; @@ -460,16 +459,3 @@ fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> { .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", bytes))?; Ok(&OsStr::new(s)) } - -pub fn try_into_host_usize(i: impl Into<u128>) -> Option<usize> { - let i: u128 = i.into(); - if i > usize::max_value() as u128 { - None - } else { - Some(i as usize) - } -} - -pub fn try_from_host_usize<T: TryFrom<u128>>(i: usize) -> Option<T> { - T::try_from(i as u128).ok() -} diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 3fccbaa33f..8b2452d33d 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use std::fs::{remove_file, File, OpenOptions}; use std::io::{Read, Write}; +use std::convert::TryFrom; use rustc::ty::layout::Size; @@ -174,16 +175,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.read_scalar(buf_op)?.not_undef()?; if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { - let count = helpers::try_into_host_usize(count) - .ok_or_else(|| err_unsup_format!("Program tries to read into buffer too big for this host platform"))?; - // We want to read at most `count` bytes - let mut bytes = vec![0; count]; + let count = isize::try_from(count) + .map_err(|_| err_unsup_format!("Program tries to read into buffer too big for this host platform"))?; + // We want to read at most `count` bytes. We are sure that `count` is not negative + // because it was a target's `usize`. Also we are sure that its smaller than + // `usize::max_value()` because it is a host's `isize`. + let mut bytes = vec![0; count as usize]; let result = handle.file.read(&mut bytes); match result { Ok(c) => { - let read_bytes = helpers::try_from_host_usize::<i64>(c) - .ok_or_else(|| err_unsup_format!("Number of read bytes {} cannot be transformed to i64", c))?; + let read_bytes = i64::try_from(c) + .map_err(|_| err_unsup_format!("Number of read bytes {} cannot be transformed to i64", c))?; // If reading to `bytes` did not fail, we write those bytes to the buffer. this.memory.write_bytes(buf, bytes)?; Ok(read_bytes) @@ -221,8 +224,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = handle.file.write(&bytes); match result { - Ok(c) => helpers::try_from_host_usize::<i64>(c) - .ok_or_else(|| err_unsup_format!("Number of written bytes {} cannot be transformed to i64", c).into()), + Ok(c) => i64::try_from(c) + .map_err(|_| err_unsup_format!("Number of written bytes {} cannot be transformed to i64", c).into()), Err(e) => { this.set_last_error_from_io_error(e)?; Ok(-1) From 6e37f2f3e158c98d094b7f3c9f681ed8e5b8c4c2 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Sat, 9 Nov 2019 15:15:52 +0100 Subject: [PATCH 09/11] Cap `count` --- src/shims/fs.rs | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 4d1b7fdf06..5e84cdee5c 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; +use std::convert::TryFrom; use std::fs::{remove_file, File, OpenOptions}; use std::io::{Read, Write}; -use std::convert::TryFrom; use rustc::ty::layout::Size; @@ -166,7 +166,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("read")?; - let count = this.read_scalar(count_op)?.to_machine_usize(&*this.tcx)?; + let ptr_size = this.pointer_size().bits(); + + let count = this + .read_scalar(count_op)? + .to_machine_usize(&*this.tcx)? + .min(1 << (ptr_size - 1)); // Reading zero bytes should not change `buf`. if count == 0 { return Ok(0); @@ -175,22 +180,22 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.read_scalar(buf_op)?.not_undef()?; if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { - let count = isize::try_from(count) - .map_err(|_| err_unsup_format!("Program tries to read into buffer too big for this host platform"))?; + let count = isize::try_from(count).unwrap(); // We want to read at most `count` bytes. We are sure that `count` is not negative // because it was a target's `usize`. Also we are sure that its smaller than // `usize::max_value()` because it is a host's `isize`. let mut bytes = vec![0; count as usize]; - let result = handle.file.read(&mut bytes); + let result = handle + .file + .read(&mut bytes) + .map(|c| i64::try_from(c).unwrap()); match result { - Ok(c) => { - let read_bytes = i64::try_from(c) - .map_err(|_| err_unsup_format!("Number of read bytes {} cannot be transformed to i64", c))?; + Ok(read_bytes) => { // If reading to `bytes` did not fail, we write those bytes to the buffer. this.memory.write_bytes(buf, bytes)?; Ok(read_bytes) - }, + } Err(e) => { this.set_last_error_from_io_error(e)?; Ok(-1) @@ -211,7 +216,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("write")?; - let count = this.read_scalar(count_op)?.to_machine_usize(&*this.tcx)?; + let ptr_size = this.pointer_size().bits(); + + let count = this + .read_scalar(count_op)? + .to_machine_usize(&*this.tcx)? + .min(1 << (ptr_size - 1)); // Writing zero bytes should not change `buf`. if count == 0 { return Ok(0); @@ -221,16 +231,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; - let result = handle.file.write(&bytes); - - match result { - Ok(c) => i64::try_from(c) - .map_err(|_| err_unsup_format!("Number of written bytes {} cannot be transformed to i64", c).into()), - Err(e) => { - this.set_last_error_from_io_error(e)?; - Ok(-1) - } - } + let result = handle.file.write(&bytes).map(|c| i64::try_from(c).unwrap()); + this.try_unwrap_io_result(result) } else { this.handle_not_found() } From edd0157069b09e3fceb8036cabbdba96972cb2f7 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Wed, 13 Nov 2019 11:57:20 -0500 Subject: [PATCH 10/11] Cap `count` twice --- src/shims/fs.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 5e84cdee5c..9a819cd9db 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -168,10 +168,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let ptr_size = this.pointer_size().bits(); + // We cap the number of read bytes to the largest value that we are able to fit in both the + // host's and target's `isize`. let count = this .read_scalar(count_op)? .to_machine_usize(&*this.tcx)? - .min(1 << (ptr_size - 1)); + .min(1 << (ptr_size - 1)) + .min(isize::max_value() as u64); // Reading zero bytes should not change `buf`. if count == 0 { return Ok(0); @@ -180,6 +183,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.read_scalar(buf_op)?.not_undef()?; if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) { + // This can never fail because `count` was capped to be smaller than + // `isize::max_value()`. let count = isize::try_from(count).unwrap(); // We want to read at most `count` bytes. We are sure that `count` is not negative // because it was a target's `usize`. Also we are sure that its smaller than @@ -188,6 +193,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = handle .file .read(&mut bytes) + // `File::read` never returns a value larger than `i64::max_value()`, so this + // unwrap cannot fail. .map(|c| i64::try_from(c).unwrap()); match result { @@ -218,10 +225,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let ptr_size = this.pointer_size().bits(); + // We cap the number of read bytes to the largest value that we are able to fit in both the + // host's and target's `isize`. let count = this .read_scalar(count_op)? .to_machine_usize(&*this.tcx)? - .min(1 << (ptr_size - 1)); + .min(1 << (ptr_size - 1)) + .min(isize::max_value() as u64); // Writing zero bytes should not change `buf`. if count == 0 { return Ok(0); From 4baef7120a934b79f87dbe5d09591d781c1c81f7 Mon Sep 17 00:00:00 2001 From: Christian Poveda <christianpoveda@protonmail.com> Date: Wed, 13 Nov 2019 14:45:00 -0500 Subject: [PATCH 11/11] Fix maximum `isize` value for target --- src/shims/fs.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 9a819cd9db..4de59ad32f 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -173,7 +173,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let count = this .read_scalar(count_op)? .to_machine_usize(&*this.tcx)? - .min(1 << (ptr_size - 1)) + .min((1 << (ptr_size - 1)) - 1) // max value of target `isize` .min(isize::max_value() as u64); // Reading zero bytes should not change `buf`. if count == 0 { @@ -193,8 +193,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = handle .file .read(&mut bytes) - // `File::read` never returns a value larger than `i64::max_value()`, so this - // unwrap cannot fail. + // `File::read` never returns a value larger than `count`, so this cannot fail. .map(|c| i64::try_from(c).unwrap()); match result { @@ -230,7 +229,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let count = this .read_scalar(count_op)? .to_machine_usize(&*this.tcx)? - .min(1 << (ptr_size - 1)) + .min((1 << (ptr_size - 1)) - 1) // max value of target `isize` .min(isize::max_value() as u64); // Writing zero bytes should not change `buf`. if count == 0 {