From 976c976f0948ebfb080f51c733c5bbe24c366554 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 01:49:28 -0500 Subject: [PATCH 1/8] Rename shims::io to shims::fs --- src/lib.rs | 2 +- src/shims/{io.rs => fs.rs} | 0 src/shims/mod.rs | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename src/shims/{io.rs => fs.rs} (100%) diff --git a/src/lib.rs b/src/lib.rs index 9f4e605b6c9..baae26d91ad 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -34,7 +34,7 @@ pub use crate::shims::intrinsics::EvalContextExt as IntrinsicsEvalContextExt; pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::dlsym::{Dlsym, EvalContextExt as DlsymEvalContextExt}; pub use crate::shims::env::{EnvVars, EvalContextExt as EnvEvalContextExt}; -pub use crate::shims::io::{FileHandler, EvalContextExt as FileEvalContextExt}; +pub use crate::shims::fs::{FileHandler, EvalContextExt as FileEvalContextExt}; pub use crate::operator::EvalContextExt as OperatorEvalContextExt; pub use crate::range_map::RangeMap; pub use crate::helpers::{EvalContextExt as HelpersEvalContextExt}; diff --git a/src/shims/io.rs b/src/shims/fs.rs similarity index 100% rename from src/shims/io.rs rename to src/shims/fs.rs diff --git a/src/shims/mod.rs b/src/shims/mod.rs index 3df5b839e5d..6d80af46850 100644 --- a/src/shims/mod.rs +++ b/src/shims/mod.rs @@ -3,7 +3,7 @@ pub mod env; pub mod foreign_items; pub mod intrinsics; pub mod tls; -pub mod io; +pub mod fs; use rustc::{mir, ty}; From c8df0171e84deb4563f30f9f1c3cf61e859315fb Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 01:53:31 -0500 Subject: [PATCH 2/8] Move functions to eval libc constants to helpers --- src/helpers.rs | 11 +++++++++++ src/shims/foreign_items.rs | 11 ----------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 3bee028c5eb..39c7af94e0a 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -291,4 +291,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } } + /// Helper function to get a `libc` constant as a `Scalar`. + fn eval_libc(&mut self, name: &str) -> InterpResult<'tcx, Scalar> { + self.eval_context_mut() + .eval_path_scalar(&["libc", name])? + .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name).into()) + .and_then(|scalar| scalar.not_undef()) + } + /// Helper function to get a `libc` constant as an `i32`. + fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> { + self.eval_libc(name).and_then(|scalar| scalar.to_i32()) + } } diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 9cd84d0b887..00160156312 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -981,17 +981,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(None); } - fn eval_libc(&mut self, name: &str) -> InterpResult<'tcx, Scalar> { - self.eval_context_mut() - .eval_path_scalar(&["libc", name])? - .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name).into()) - .and_then(|scalar| scalar.not_undef()) - } - - fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> { - self.eval_libc(name).and_then(|scalar| scalar.to_i32()) - } - fn set_last_error(&mut self, scalar: Scalar) -> InterpResult<'tcx> { let this = self.eval_context_mut(); let tcx = &{ this.tcx.tcx }; From 8368d4f2b44ae1441390a2f7eee48af340c8d70c Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 02:35:50 -0500 Subject: [PATCH 3/8] Add comments to explain the chdir test --- tests/run-pass/{change_current_dir.rs => current_dir.rs} | 3 +++ 1 file changed, 3 insertions(+) rename tests/run-pass/{change_current_dir.rs => current_dir.rs} (66%) diff --git a/tests/run-pass/change_current_dir.rs b/tests/run-pass/current_dir.rs similarity index 66% rename from tests/run-pass/change_current_dir.rs rename to tests/run-pass/current_dir.rs index fa8220339db..48045187ab1 100644 --- a/tests/run-pass/change_current_dir.rs +++ b/tests/run-pass/current_dir.rs @@ -6,6 +6,9 @@ use std::path::Path; fn main() { // test that `getcwd` is available let cwd = env::current_dir().unwrap(); + // test that changing dir to `..` actually sets the current directory to the parent of `cwd`. + // the only exception here is if `cwd` is the root directory, then changing directory must + // keep the current directory equal to `cwd`. let parent = cwd.parent().unwrap_or(&cwd); // test that `chdir` is available assert!(env::set_current_dir(&Path::new("..")).is_ok()); From 12040aae885b8df967892ea1de420b9a35b42599 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 03:36:34 -0500 Subject: [PATCH 4/8] Add comment explaining why buffer isn't overflowed --- src/shims/env.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index f4b6a7c4dba..b39bde4dedc 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -133,11 +133,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(cwd) => { // It is not clear what happens with non-utf8 paths here let mut bytes = cwd.display().to_string().into_bytes(); - // If the buffer is smaller or equal than the path, we return null. + // If `size` is smaller or equal than the `bytes.len()`, writing `bytes` 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 is larger than the path with the null terminator. + // 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_mut() .get_mut(buf.alloc_id)? .write_bytes(tcx, buf, &bytes)?; From 1c64f29811db779bb202c3450c60c227fb0234bb Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 03:55:18 -0500 Subject: [PATCH 5/8] Add comment for the flag diff check --- src/shims/fs.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 0893d0b4e0e..7e684489b5c 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -107,6 +107,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(FileHandle { flag: old_flag, .. }) = this.machine.file_handler.handles.get_mut(&fd) { + // Check that the only difference between the old flag and the current flag is + // exactly the `FD_CLOEXEC` value. if flag ^ *old_flag == fd_cloexec { *old_flag = flag; } else { From 67ea4546472edace0ae1d8fc97ae85bf5953e8a5 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 04:17:43 -0500 Subject: [PATCH 6/8] Correct style of comments --- src/helpers.rs | 2 ++ src/shims/env.rs | 6 +++--- tests/run-pass/current_dir.rs | 10 +++++----- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index 39c7af94e0a..b2a462ef906 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -291,6 +291,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } } + /// Helper function to get a `libc` constant as a `Scalar`. fn eval_libc(&mut self, name: &str) -> InterpResult<'tcx, Scalar> { self.eval_context_mut() @@ -298,6 +299,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name).into()) .and_then(|scalar| scalar.not_undef()) } + /// Helper function to get a `libc` constant as an `i32`. fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> { self.eval_libc(name).and_then(|scalar| scalar.to_i32()) diff --git a/src/shims/env.rs b/src/shims/env.rs index b39bde4dedc..6f32783d7a7 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -133,9 +133,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(cwd) => { // It is not clear what happens with non-utf8 paths here let mut bytes = cwd.display().to_string().into_bytes(); - // If `size` is smaller or equal than the `bytes.len()`, writing `bytes` using the - // `buf` pointer would cause an overflow, the desired behavior in this case is to - // return null. + // 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); diff --git a/tests/run-pass/current_dir.rs b/tests/run-pass/current_dir.rs index 48045187ab1..5e896659c85 100644 --- a/tests/run-pass/current_dir.rs +++ b/tests/run-pass/current_dir.rs @@ -4,14 +4,14 @@ use std::env; use std::path::Path; fn main() { - // test that `getcwd` is available + // Test that `getcwd` is available let cwd = env::current_dir().unwrap(); - // test that changing dir to `..` actually sets the current directory to the parent of `cwd`. - // the only exception here is if `cwd` is the root directory, then changing directory must + // Test that changing dir to `..` actually sets the current directory to the parent of `cwd`. + // The only exception here is if `cwd` is the root directory, then changing directory must // keep the current directory equal to `cwd`. let parent = cwd.parent().unwrap_or(&cwd); - // test that `chdir` is available + // Test that `chdir` is available assert!(env::set_current_dir(&Path::new("..")).is_ok()); - // test that `..` goes to the parent directory + // Test that `..` goes to the parent directory assert_eq!(env::current_dir().unwrap(), parent); } From 60cf06a03fa06bc1c5e2981d4ad7f5496b06c62f Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 04:20:18 -0500 Subject: [PATCH 7/8] Use existing tcx instead --- src/shims/env.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shims/env.rs b/src/shims/env.rs index 6f32783d7a7..8c1645cde73 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -127,7 +127,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let tcx = &{ this.tcx.tcx }; let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; - let size = this.read_scalar(size_op)?.to_usize(&*this.tcx)?; + let size = this.read_scalar(size_op)?.to_usize(&*tcx)?; // If we cannot get the current directory, we return null match env::current_dir() { Ok(cwd) => { @@ -152,7 +152,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Err(e) => this.consume_io_error(e)?, } - Ok(Scalar::ptr_null(&*this.tcx)) + Ok(Scalar::ptr_null(&*tcx)) } fn chdir(&mut self, path_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { From 003b257f87b94921594a46b4e0ad4dc22bb4cf4d Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 08:20:32 -0500 Subject: [PATCH 8/8] Change error handling style for consistency --- src/helpers.rs | 6 +++--- src/shims/env.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index b2a462ef906..c7b7853388c 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -296,12 +296,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx fn eval_libc(&mut self, name: &str) -> InterpResult<'tcx, Scalar> { self.eval_context_mut() .eval_path_scalar(&["libc", name])? - .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name).into()) - .and_then(|scalar| scalar.not_undef()) + .ok_or_else(|| err_unsup_format!("Path libc::{} cannot be resolved.", name))? + .not_undef() } /// Helper function to get a `libc` constant as an `i32`. fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> { - self.eval_libc(name).and_then(|scalar| scalar.to_i32()) + self.eval_libc(name)?.to_i32() } } diff --git a/src/shims/env.rs b/src/shims/env.rs index 8c1645cde73..2ccbc0238e5 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -135,7 +135,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let mut bytes = cwd.display().to_string().into_bytes(); // 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. + // 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);