From 79b1f91f45e3883e0946846cfd82aa4273c9809a Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 24 Sep 2019 17:28:00 -0500 Subject: [PATCH 01/11] First version of file handling --- src/lib.rs | 1 + src/machine.rs | 3 + src/shims/foreign_items.rs | 20 ++++ src/shims/io.rs | 179 ++++++++++++++++++++++++++++++++++++ src/shims/mod.rs | 1 + tests/hello.txt | 1 + tests/run-pass/file_read.rs | 12 +++ 7 files changed, 217 insertions(+) create mode 100644 src/shims/io.rs create mode 100644 tests/hello.txt create mode 100644 tests/run-pass/file_read.rs diff --git a/src/lib.rs b/src/lib.rs index cea99d86ea..9f4e605b6c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -34,6 +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::operator::EvalContextExt as OperatorEvalContextExt; pub use crate::range_map::RangeMap; pub use crate::helpers::{EvalContextExt as HelpersEvalContextExt}; diff --git a/src/machine.rs b/src/machine.rs index 340dd109e1..19be5b547b 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -96,6 +96,8 @@ pub struct Evaluator<'tcx> { /// If enabled, the `env_vars` field is populated with the host env vars during initialization /// and random number generation is delegated to the host. pub(crate) communicate: bool, + + pub(crate) file_handler: FileHandler, } impl<'tcx> Evaluator<'tcx> { @@ -110,6 +112,7 @@ impl<'tcx> Evaluator<'tcx> { last_error: 0, tls: TlsData::default(), communicate, + file_handler: Default::default(), } } } diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index fedb6354b8..f717d7959f 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -446,6 +446,26 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; } + "open" | "open64" => { + let result = this.open(args[0], args[1])?; + this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; + } + + "fcntl" => { + let result = this.fcntl(args[0], args[1], args.get(2).cloned())?; + this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; + } + + "close" => { + let result = this.close(args[0])?; + this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; + } + + "read" => { + let result = this.read(args[0], args[1], args[2])?; + this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; + } + "write" => { let fd = this.read_scalar(args[0])?.to_i32()?; let buf = this.read_scalar(args[1])?.not_undef()?; diff --git a/src/shims/io.rs b/src/shims/io.rs new file mode 100644 index 0000000000..02e5133c9a --- /dev/null +++ b/src/shims/io.rs @@ -0,0 +1,179 @@ +use std::collections::HashMap; +use std::fs::File; +use std::io::Read; + +use crate::stacked_borrows::Tag; +use crate::*; + +#[derive(Default)] +pub struct FileHandler { + files: HashMap, + flags: HashMap, + low: i32, +} + +impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + fn open( + &mut self, + path_op: OpTy<'tcx, Tag>, + flag_op: OpTy<'tcx, Tag>, + ) -> InterpResult<'tcx, i32> { + let this = self.eval_context_mut(); + + if !this.machine.communicate { + throw_unsup_format!("`open` not available when isolation is enabled") + } + + let flag = this.read_scalar(flag_op)?.to_i32()?; + + if flag + != this + .eval_path_scalar(&["libc", "O_RDONLY"])? + .unwrap() + .to_i32()? + && flag + != this + .eval_path_scalar(&["libc", "O_CLOEXEC"])? + .unwrap() + .to_i32()? + { + throw_unsup_format!("Unsupported flag {:#x}", flag); + } + + 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))?; + + match File::open(path) { + Ok(file) => { + let mut fh = &mut this.machine.file_handler; + fh.low += 1; + fh.files.insert(fh.low, file); + fh.flags.insert(fh.low, flag); + Ok(fh.low) + } + + Err(e) => { + this.machine.last_error = e.raw_os_error().unwrap() as u32; + Ok(-1) + } + } + } + + fn fcntl( + &mut self, + fd_op: OpTy<'tcx, Tag>, + cmd_op: OpTy<'tcx, Tag>, + arg_op: Option>, + ) -> InterpResult<'tcx, i32> { + let this = self.eval_context_mut(); + + if !this.machine.communicate { + throw_unsup_format!("`open` not available when isolation is enabled") + } + + let fd = this.read_scalar(fd_op)?.to_i32()?; + let cmd = this.read_scalar(cmd_op)?.to_i32()?; + + if cmd + == this + .eval_path_scalar(&["libc", "F_SETFD"])? + .unwrap() + .to_i32()? + { + let flag = this.read_scalar(arg_op.unwrap())?.to_i32()?; + this.machine.file_handler.flags.insert(fd, flag); + Ok(0) + } else if cmd + == this + .eval_path_scalar(&["libc", "F_GETFD"])? + .unwrap() + .to_i32()? + { + if let Some(flag) = this.machine.file_handler.flags.get(&fd) { + Ok(*flag) + } else { + this.machine.last_error = this + .eval_path_scalar(&["libc", "EBADF"])? + .unwrap() + .to_u32()?; + Ok(-1) + } + } else { + throw_unsup_format!("Unsupported command {:#x}", cmd); + } + } + + fn close(&mut self, fd_op: OpTy<'tcx, Tag>) -> InterpResult<'tcx, i32> { + let this = self.eval_context_mut(); + + if !this.machine.communicate { + throw_unsup_format!("`open` not available when isolation is enabled") + } + + let fd = this.read_scalar(fd_op)?.to_i32()?; + + if let Some(file) = this.machine.file_handler.files.remove(&fd) { + match file.sync_all() { + Ok(()) => Ok(0), + Err(e) => { + this.machine.last_error = e.raw_os_error().unwrap() as u32; + Ok(-1) + } + } + } else { + this.machine.last_error = this + .eval_path_scalar(&["libc", "EBADF"])? + .unwrap() + .to_u32()?; + Ok(-1) + } + } + + fn read( + &mut self, + fd_op: OpTy<'tcx, Tag>, + buf_op: OpTy<'tcx, Tag>, + count_op: OpTy<'tcx, Tag>, + ) -> InterpResult<'tcx, i64> { + let this = self.eval_context_mut(); + + if !this.machine.communicate { + throw_unsup_format!("`open` not available when isolation is enabled") + } + + let tcx = &{ this.tcx.tcx }; + + let fd = this.read_scalar(fd_op)?.to_i32()?; + let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; + let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; + + if let Some(file) = this.machine.file_handler.files.get_mut(&fd) { + let mut bytes = vec![0; count as usize]; + match file.read(&mut bytes) { + Ok(read_bytes) => { + bytes.truncate(read_bytes); + + this.memory_mut() + .get_mut(buf.alloc_id)? + .write_bytes(tcx, buf, &bytes)?; + + Ok(read_bytes as i64) + } + Err(e) => { + this.machine.last_error = e.raw_os_error().unwrap() as u32; + Ok(-1) + } + } + } else { + this.machine.last_error = this + .eval_path_scalar(&["libc", "EBADF"])? + .unwrap() + .to_u32()?; + Ok(-1) + } + } +} diff --git a/src/shims/mod.rs b/src/shims/mod.rs index a0da364ff0..981185d252 100644 --- a/src/shims/mod.rs +++ b/src/shims/mod.rs @@ -3,6 +3,7 @@ pub mod intrinsics; pub mod tls; pub mod dlsym; pub mod env; +pub mod io; use rustc::{ty, mir}; diff --git a/tests/hello.txt b/tests/hello.txt new file mode 100644 index 0000000000..8ab686eafe --- /dev/null +++ b/tests/hello.txt @@ -0,0 +1 @@ +Hello, World! diff --git a/tests/run-pass/file_read.rs b/tests/run-pass/file_read.rs new file mode 100644 index 0000000000..93c986393b --- /dev/null +++ b/tests/run-pass/file_read.rs @@ -0,0 +1,12 @@ +// ignore-windows: File handling is not implemented yet +// compile-flags: -Zmiri-disable-isolation + +use std::fs::File; +use std::io::Read; + +fn main() { + let mut file = File::open("./tests/hello.txt").unwrap(); + let mut contents = String::new(); + file.read_to_string(&mut contents).unwrap(); + assert_eq!("Hello, World!\n", contents); +} From 3726081857e3759ce7b7caeb836d6dce9f515227 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 25 Sep 2019 10:49:12 -0500 Subject: [PATCH 02/11] Add helper function to fetch `libc` constants --- src/shims/foreign_items.rs | 8 ++++++++ src/shims/io.rs | 41 ++++++-------------------------------- 2 files changed, 14 insertions(+), 35 deletions(-) diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index f717d7959f..2c0064976e 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -949,6 +949,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } return Ok(None); } + + fn eval_libc_i32(&mut self, name: &str) -> InterpResult<'tcx, i32> { + 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.to_i32()) + } } // Shims the linux 'getrandom()' syscall. diff --git a/src/shims/io.rs b/src/shims/io.rs index 02e5133c9a..4b9af52962 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -27,17 +27,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let flag = this.read_scalar(flag_op)?.to_i32()?; - if flag - != this - .eval_path_scalar(&["libc", "O_RDONLY"])? - .unwrap() - .to_i32()? - && flag - != this - .eval_path_scalar(&["libc", "O_CLOEXEC"])? - .unwrap() - .to_i32()? - { + if flag != this.eval_libc_i32("O_RDONLY")? && flag != this.eval_libc_i32("O_CLOEXEC")? { throw_unsup_format!("Unsupported flag {:#x}", flag); } @@ -78,28 +68,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; let cmd = this.read_scalar(cmd_op)?.to_i32()?; - if cmd - == this - .eval_path_scalar(&["libc", "F_SETFD"])? - .unwrap() - .to_i32()? - { + if cmd == this.eval_libc_i32("F_SETFD")? { let flag = this.read_scalar(arg_op.unwrap())?.to_i32()?; this.machine.file_handler.flags.insert(fd, flag); Ok(0) - } else if cmd - == this - .eval_path_scalar(&["libc", "F_GETFD"])? - .unwrap() - .to_i32()? - { + } else if cmd == this.eval_libc_i32("F_GETFD")? { if let Some(flag) = this.machine.file_handler.flags.get(&fd) { Ok(*flag) } else { - this.machine.last_error = this - .eval_path_scalar(&["libc", "EBADF"])? - .unwrap() - .to_u32()?; + this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; Ok(-1) } } else { @@ -125,10 +102,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } } else { - this.machine.last_error = this - .eval_path_scalar(&["libc", "EBADF"])? - .unwrap() - .to_u32()?; + this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; Ok(-1) } } @@ -169,10 +143,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } } else { - this.machine.last_error = this - .eval_path_scalar(&["libc", "EBADF"])? - .unwrap() - .to_u32()?; + this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; Ok(-1) } } From 01f64616adbeb6162a6aecabbdf562810f14e20f Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 25 Sep 2019 11:11:20 -0500 Subject: [PATCH 03/11] Check that the only flag change is done to enable `FD_CLOEXEC` --- src/shims/io.rs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 4b9af52962..180748a075 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -69,8 +69,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let cmd = this.read_scalar(cmd_op)?.to_i32()?; if cmd == this.eval_libc_i32("F_SETFD")? { + // This does not affect the file itself. Certain flags might require changing the file + // or the way it is accessed somehow. let flag = this.read_scalar(arg_op.unwrap())?.to_i32()?; - this.machine.file_handler.flags.insert(fd, flag); + // The only usage of this in stdlib at the moment is to enable the `FD_CLOEXEC` flag. + let fd_cloexec = this.eval_libc_i32("FD_CLOEXEC")?; + if let Some(old_flag) = this.machine.file_handler.flags.get_mut(&fd) { + if flag ^ *old_flag == fd_cloexec { + *old_flag = flag; + } else { + throw_unsup_format!("Unsupported arg {:#x} for `F_SETFD`", flag); + } + } Ok(0) } else if cmd == this.eval_libc_i32("F_GETFD")? { if let Some(flag) = this.machine.file_handler.flags.get(&fd) { From bdaa90ceb20a42405f9c7fee31133e70aab1bf7d Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 25 Sep 2019 11:12:46 -0500 Subject: [PATCH 04/11] Add FIXME to file reading test --- tests/run-pass/file_read.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/run-pass/file_read.rs b/tests/run-pass/file_read.rs index 93c986393b..a60640d1b3 100644 --- a/tests/run-pass/file_read.rs +++ b/tests/run-pass/file_read.rs @@ -5,6 +5,7 @@ use std::fs::File; use std::io::Read; fn main() { + // FIXME: create the file and delete it when `rm` is implemented. let mut file = File::open("./tests/hello.txt").unwrap(); let mut contents = String::new(); file.read_to_string(&mut contents).unwrap(); From ca3a917a6fdc02caf40b5c466b8394be7f61f0d5 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 25 Sep 2019 11:16:11 -0500 Subject: [PATCH 05/11] Enable close call for macos --- src/shims/foreign_items.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index 2c0064976e..b8609fd2ef 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -456,7 +456,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; } - "close" => { + "close" | "close$NOCANCEL" => { let result = this.close(args[0])?; this.write_scalar(Scalar::from_int(result, dest.layout.size), dest)?; } From 03ed4123c0b9ee265d713ba76b1b003d02c5bf3b Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Mon, 30 Sep 2019 11:51:09 -0500 Subject: [PATCH 06/11] Add FileHandle struct --- src/shims/io.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 6f8785891c..44acf45c9a 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -5,17 +5,20 @@ use std::io::Read; use crate::stacked_borrows::Tag; use crate::*; +struct FileHandle { + file: File, + flag: i32, +} + pub struct FileHandler { - files: HashMap, - flags: HashMap, + handles: HashMap, low: i32, } impl Default for FileHandler { fn default() -> Self { FileHandler { - files: Default::default(), - flags: Default::default(), + handles: Default::default(), // 0, 1 and 2 are reserved for stdin, stdout and stderr low: 3, } @@ -51,8 +54,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(file) => { let mut fh = &mut this.machine.file_handler; fh.low += 1; - fh.files.insert(fh.low, file); - fh.flags.insert(fh.low, flag); + fh.handles.insert(fh.low, FileHandle{ file, flag}); Ok(fh.low) } @@ -84,7 +86,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let flag = this.read_scalar(arg_op.unwrap())?.to_i32()?; // The only usage of this in stdlib at the moment is to enable the `FD_CLOEXEC` flag. let fd_cloexec = this.eval_libc_i32("FD_CLOEXEC")?; - if let Some(old_flag) = this.machine.file_handler.flags.get_mut(&fd) { + if let Some(FileHandle{ flag: old_flag, .. }) = this.machine.file_handler.handles.get_mut(&fd) { if flag ^ *old_flag == fd_cloexec { *old_flag = flag; } else { @@ -93,8 +95,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Ok(0) } else if cmd == this.eval_libc_i32("F_GETFD")? { - if let Some(flag) = this.machine.file_handler.flags.get(&fd) { - Ok(*flag) + if let Some(handle) = this.machine.file_handler.handles.get(&fd) { + Ok(handle.flag) } else { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; Ok(-1) @@ -113,8 +115,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; - if let Some(file) = this.machine.file_handler.files.remove(&fd) { - match file.sync_all() { + if let Some(handle) = this.machine.file_handler.handles.remove(&fd) { + match handle.file.sync_all() { Ok(()) => Ok(0), Err(e) => { this.machine.last_error = e.raw_os_error().unwrap() as u32; @@ -145,7 +147,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; - if let Some(file) = this.machine.file_handler.files.get_mut(&fd) { + if let Some(FileHandle { file, ..}) = this.machine.file_handler.handles.get_mut(&fd) { let mut bytes = vec![0; count as usize]; match file.read(&mut bytes) { Ok(read_bytes) => { From 775246e3297ef49ae361a43fc5ce469f1cc26c96 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Mon, 30 Sep 2019 14:07:08 -0500 Subject: [PATCH 07/11] Add method to consume std::io::Result --- src/shims/io.rs | 73 +++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 39 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 44acf45c9a..c75d3d5ea2 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -48,21 +48,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .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))?; - - match File::open(path) { - Ok(file) => { - let mut fh = &mut this.machine.file_handler; - fh.low += 1; - fh.handles.insert(fh.low, FileHandle{ file, flag}); - Ok(fh.low) - } - - Err(e) => { - this.machine.last_error = e.raw_os_error().unwrap() as u32; - Ok(-1) - } - } + .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", path_bytes))? + .to_owned(); + let fd = File::open(&path).map(|file| { + let mut fh = &mut this.machine.file_handler; + fh.low += 1; + fh.handles.insert(fh.low, FileHandle{ file, flag}); + fh.low + }); + + this.consume_result::(fd, -1) } fn fcntl( @@ -116,13 +111,7 @@ 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) { - match handle.file.sync_all() { - Ok(()) => Ok(0), - Err(e) => { - this.machine.last_error = e.raw_os_error().unwrap() as u32; - Ok(-1) - } - } + this.consume_result::(handle.file.sync_all().map(|_| 0), -1) } else { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; Ok(-1) @@ -147,26 +136,32 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; - if let Some(FileHandle { file, ..}) = this.machine.file_handler.handles.get_mut(&fd) { - let mut bytes = vec![0; count as usize]; - match file.read(&mut bytes) { - Ok(read_bytes) => { - bytes.truncate(read_bytes); - - this.memory_mut() - .get_mut(buf.alloc_id)? - .write_bytes(tcx, buf, &bytes)?; + let mut bytes = vec![0; count as usize]; - Ok(read_bytes as i64) - } - Err(e) => { - this.machine.last_error = e.raw_os_error().unwrap() as u32; - Ok(-1) - } - } + let read_result = if let Some(FileHandle { file, ..}) = this.machine.file_handler.handles.get_mut(&fd) { + file.read(&mut bytes).map(|bytes| bytes as i64) } else { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - Ok(-1) + return Ok(-1); + }; + + let read_bytes = this.consume_result::(read_result, -1)?; + if read_bytes != -1 { + bytes.truncate(read_bytes as usize); + this.memory_mut() + .get_mut(buf.alloc_id)? + .write_bytes(tcx, buf, &bytes)?; + } + Ok(read_bytes) + } + + fn consume_result(&mut self, result: std::io::Result, t: T) -> InterpResult<'tcx, T> { + match result { + Ok(ok) => Ok(ok), + Err(e) => { + self.eval_context_mut().machine.last_error = e.raw_os_error().unwrap() as u32; + Ok(t) + } } } } From efbe798e626bee92cb7b2858b26ead38746b1960 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Mon, 30 Sep 2019 14:21:45 -0500 Subject: [PATCH 08/11] Avoid buffer allocation to read files --- src/shims/io.rs | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index c75d3d5ea2..6b1952e8d6 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -2,6 +2,8 @@ use std::collections::HashMap; use std::fs::File; use std::io::Read; +use rustc::ty::layout::Size; + use crate::stacked_borrows::Tag; use crate::*; @@ -53,7 +55,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = File::open(&path).map(|file| { let mut fh = &mut this.machine.file_handler; fh.low += 1; - fh.handles.insert(fh.low, FileHandle{ file, flag}); + fh.handles.insert(fh.low, FileHandle { file, flag }); fh.low }); @@ -81,7 +83,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let flag = this.read_scalar(arg_op.unwrap())?.to_i32()?; // The only usage of this in stdlib at the moment is to enable the `FD_CLOEXEC` flag. let fd_cloexec = this.eval_libc_i32("FD_CLOEXEC")?; - if let Some(FileHandle{ flag: old_flag, .. }) = this.machine.file_handler.handles.get_mut(&fd) { + if let Some(FileHandle { flag: old_flag, .. }) = + this.machine.file_handler.handles.get_mut(&fd) + { if flag ^ *old_flag == fd_cloexec { *old_flag = flag; } else { @@ -136,23 +140,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; - let mut bytes = vec![0; count as usize]; - - let read_result = if let Some(FileHandle { file, ..}) = this.machine.file_handler.handles.get_mut(&fd) { - file.read(&mut bytes).map(|bytes| bytes as i64) + // Remove the file handle to avoid borrowing issues + if let Some(mut handle) = this.machine.file_handler.handles.remove(&fd) { + let bytes = handle + .file + .read(this.memory_mut().get_mut(buf.alloc_id)?.get_bytes_mut( + tcx, + buf, + Size::from_bytes(count), + )?) + .map(|bytes| bytes as i64); + // Reinsert the file handle + this.machine.file_handler.handles.insert(fd, handle); + this.consume_result::(bytes, -1) } else { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - return Ok(-1); - }; - - let read_bytes = this.consume_result::(read_result, -1)?; - if read_bytes != -1 { - bytes.truncate(read_bytes as usize); - this.memory_mut() - .get_mut(buf.alloc_id)? - .write_bytes(tcx, buf, &bytes)?; + Ok(-1) } - Ok(read_bytes) } fn consume_result(&mut self, result: std::io::Result, t: T) -> InterpResult<'tcx, T> { From 644467c570314097f626868320bb4a9b2f769fc7 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Mon, 30 Sep 2019 15:18:23 -0500 Subject: [PATCH 09/11] Add methods to handle invalid fides --- src/shims/io.rs | 73 +++++++++++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 26 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 6b1952e8d6..0cb2d7eeea 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -7,7 +7,7 @@ use rustc::ty::layout::Size; use crate::stacked_borrows::Tag; use crate::*; -struct FileHandle { +pub struct FileHandle { file: File, flag: i32, } @@ -94,12 +94,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Ok(0) } else if cmd == this.eval_libc_i32("F_GETFD")? { - if let Some(handle) = this.machine.file_handler.handles.get(&fd) { - Ok(handle.flag) - } else { - this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - Ok(-1) - } + this.get_handle_and(fd, |handle| Ok(handle.flag), -1) } else { throw_unsup_format!("Unsupported command {:#x}", cmd); } @@ -114,12 +109,11 @@ 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.consume_result::(handle.file.sync_all().map(|_| 0), -1) - } else { - this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - Ok(-1) - } + this.remove_handle_and( + fd, + |handle, this| this.consume_result::(handle.file.sync_all().map(|_| 0), -1), + -1, + ) } fn read( @@ -141,21 +135,48 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; // Remove the file handle to avoid borrowing issues - if let Some(mut handle) = this.machine.file_handler.handles.remove(&fd) { - let bytes = handle - .file - .read(this.memory_mut().get_mut(buf.alloc_id)?.get_bytes_mut( - tcx, - buf, - Size::from_bytes(count), - )?) - .map(|bytes| bytes as i64); - // Reinsert the file handle - this.machine.file_handler.handles.insert(fd, handle); - this.consume_result::(bytes, -1) + this.remove_handle_and( + fd, + |mut handle, this| { + let bytes = handle + .file + .read(this.memory_mut().get_mut(buf.alloc_id)?.get_bytes_mut( + tcx, + buf, + Size::from_bytes(count), + )?) + .map(|bytes| bytes as i64); + // Reinsert the file handle + this.machine.file_handler.handles.insert(fd, handle); + this.consume_result::(bytes, -1) + }, + -1, + ) + } + + fn get_handle_and(&mut self, fd: i32, f: F, t: T) -> InterpResult<'tcx, T> + where + F: Fn(&FileHandle) -> InterpResult<'tcx, T>, + { + let this = self.eval_context_mut(); + if let Some(handle) = this.machine.file_handler.handles.get(&fd) { + f(handle) + } else { + this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; + Ok(t) + } + } + + fn remove_handle_and(&mut self, fd: i32, mut f: F, t: T) -> 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 { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - Ok(-1) + Ok(t) } } From 50be5a83c50528cc61d2b65a73839dbb25fc9c67 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 1 Oct 2019 09:18:55 -0500 Subject: [PATCH 10/11] Remove return argument when fd is not found --- src/shims/io.rs | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 0cb2d7eeea..c91d3a0772 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -50,16 +50,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .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))? - .to_owned(); - let fd = File::open(&path).map(|file| { + .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", path_bytes))?; + let fd = File::open(path).map(|file| { let mut fh = &mut this.machine.file_handler; fh.low += 1; fh.handles.insert(fh.low, FileHandle { file, flag }); fh.low }); - this.consume_result::(fd, -1) + this.consume_result(fd) } fn fcntl( @@ -94,7 +93,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } Ok(0) } else if cmd == this.eval_libc_i32("F_GETFD")? { - this.get_handle_and(fd, |handle| Ok(handle.flag), -1) + this.get_handle_and(fd, |handle| Ok(handle.flag)) } else { throw_unsup_format!("Unsupported command {:#x}", cmd); } @@ -111,8 +110,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.remove_handle_and( fd, - |handle, this| this.consume_result::(handle.file.sync_all().map(|_| 0), -1), - -1, + |handle, this| this.consume_result(handle.file.sync_all().map(|_| 0i32)), ) } @@ -148,13 +146,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .map(|bytes| bytes as i64); // Reinsert the file handle this.machine.file_handler.handles.insert(fd, handle); - this.consume_result::(bytes, -1) + this.consume_result(bytes) }, - -1, ) } - fn get_handle_and(&mut self, fd: i32, f: F, t: T) -> InterpResult<'tcx, T> + fn get_handle_and>(&mut self, fd: i32, f: F) -> InterpResult<'tcx, T> where F: Fn(&FileHandle) -> InterpResult<'tcx, T>, { @@ -163,11 +160,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx f(handle) } else { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - Ok(t) + Ok((-1).into()) } } - fn remove_handle_and(&mut self, fd: i32, mut f: F, t: T) -> InterpResult<'tcx, T> + fn remove_handle_and>(&mut self, fd: i32, mut f: F) -> InterpResult<'tcx, T> where F: FnMut(FileHandle, &mut MiriEvalContext<'mir, 'tcx>) -> InterpResult<'tcx, T>, { @@ -176,16 +173,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx f(handle, this) } else { this.machine.last_error = this.eval_libc_i32("EBADF")? as u32; - Ok(t) + Ok((-1).into()) } } - fn consume_result(&mut self, result: std::io::Result, t: T) -> InterpResult<'tcx, T> { + fn consume_result>(&mut self, result: std::io::Result) -> InterpResult<'tcx, T> { match result { Ok(ok) => Ok(ok), Err(e) => { self.eval_context_mut().machine.last_error = e.raw_os_error().unwrap() as u32; - Ok(t) + Ok((-1).into()) } } } From d0509d719c27d95b5d0f8379d66f13b99b60c42e Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 1 Oct 2019 10:31:04 -0500 Subject: [PATCH 11/11] Add docs for helper functions --- src/shims/io.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/shims/io.rs b/src/shims/io.rs index c91d3a0772..0d1adcce65 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -151,6 +151,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) } + /// Helper function that gets a `FileHandle` immutable reference and allows to manipulate it + /// using `f`. + /// + /// If the `fd` file descriptor does not corresponds to a file, this functions returns `Ok(-1)` + /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). + /// + /// This function uses `T: From` instead of `i32` directly because some IO related + /// functions return different integer types (like `read`, that returns an `i64`) fn get_handle_and>(&mut self, fd: i32, f: F) -> InterpResult<'tcx, T> where F: Fn(&FileHandle) -> InterpResult<'tcx, T>, @@ -164,6 +172,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } + /// 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 corresponds to a file, this functions returns `Ok(-1)` + /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). + /// + /// This function uses `T: From` instead of `i32` directly because some IO related + /// functions return different integer types (like `read`, that returns an `i64`) fn remove_handle_and>(&mut self, fd: i32, mut f: F) -> InterpResult<'tcx, T> where F: FnMut(FileHandle, &mut MiriEvalContext<'mir, 'tcx>) -> InterpResult<'tcx, T>, @@ -177,6 +195,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } + /// Helper function that consumes an `std::io::Result` and returns an + /// `InterpResult<'tcx,T>::Ok` instead. It is expected that the result can be converted to an + /// OS error using `std::io::Error::raw_os_error`. + /// + /// This function uses `T: From` instead of `i32` directly because some IO related + /// functions return different integer types (like `read`, that returns an `i64`) fn consume_result>(&mut self, result: std::io::Result) -> InterpResult<'tcx, T> { match result { Ok(ok) => Ok(ok),