From e3956f4200184c88f0f6100f15669b1e6639804f Mon Sep 17 00:00:00 2001 From: Samrat Man Singh Date: Fri, 31 Jul 2020 22:53:35 +0530 Subject: [PATCH 1/3] Add FileDescriptor trait to abstract fn's on File's and Std{in,out,err} --- src/machine.rs | 2 +- src/shims/posix/fs.rs | 190 ++++++++++++++++++++++++++++-------------- 2 files changed, 130 insertions(+), 62 deletions(-) diff --git a/src/machine.rs b/src/machine.rs index e9217896ef6e..bcdd92070942 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -246,7 +246,7 @@ pub struct Evaluator<'mir, 'tcx> { /// Whether to enforce the validity invariant. pub(crate) validate: bool, - pub(crate) file_handler: shims::posix::FileHandler, + pub(crate) file_handler: shims::posix::FileHandler<'tcx>, pub(crate) dir_handler: shims::posix::DirHandler, /// The temporary used for storing the argument of diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index a43e86dcc5b5..5415e9e1f77c 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -1,7 +1,7 @@ use std::collections::BTreeMap; use std::convert::{TryFrom, TryInto}; use std::fs::{read_dir, remove_dir, remove_file, rename, DirBuilder, File, FileType, OpenOptions, ReadDir}; -use std::io::{Read, Seek, SeekFrom, Write}; +use std::io::{self, Read, Seek, SeekFrom, Write}; use std::path::Path; use std::time::SystemTime; @@ -22,15 +22,42 @@ struct FileHandle { writable: bool, } -#[derive(Debug, Default)] -pub struct FileHandler { - handles: BTreeMap, +trait FileDescriptor<'tcx> : std::fmt::Debug { + fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle>; + + fn read(&mut self, bytes: &mut [u8]) -> Result; + fn write(&mut self, bytes: &[u8]) -> Result; + fn seek(&mut self, offset: SeekFrom) -> Result; } +impl<'tcx> FileDescriptor<'tcx> for FileHandle { + fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle> { + Ok(&self) + } + + fn read(&mut self, bytes: &mut [u8]) -> Result { + self.file.read(bytes) + } + + fn write(&mut self, bytes: &[u8]) -> Result { + self.file.write(bytes) + } + + fn seek(&mut self, offset: SeekFrom) -> Result { + self.file.seek(offset) + } +} + +#[derive(Debug, Default)] +pub struct FileHandler<'tcx> { + handles: BTreeMap>>, +} + + // fd numbers 0, 1, and 2 are reserved for stdin, stdout, and stderr const MIN_NORMAL_FILE_FD: i32 = 3; -impl FileHandler { +impl<'tcx> FileHandler<'tcx> { fn insert_fd(&mut self, file_handle: FileHandle) -> i32 { self.insert_fd_with_min_fd(file_handle, 0) } @@ -62,7 +89,7 @@ impl FileHandler { self.handles.last_key_value().map(|(fd, _)| fd.checked_add(1).unwrap()).unwrap_or(min_fd) }); - self.handles.insert(new_fd, file_handle).unwrap_none(); + self.handles.insert(new_fd, Box::new(file_handle)).unwrap_none(); new_fd } } @@ -383,20 +410,28 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } let fh = &mut this.machine.file_handler; let (file_result, writable) = match fh.handles.get(&fd) { - Some(FileHandle { file, writable }) => (file.try_clone(), *writable), + Some(file_descriptor) => match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => (file.try_clone(), writable.clone()), + Err(_) => return this.handle_not_found(), + }, None => return this.handle_not_found(), }; let fd_result = file_result.map(|duplicated| { - fh.insert_fd_with_min_fd(FileHandle { file: duplicated, writable }, start) + fh.insert_fd_with_min_fd(FileHandle { file: duplicated, writable: writable }, start) }); this.try_unwrap_io_result(fd_result) } else if this.tcx.sess.target.target.target_os == "macos" && cmd == this.eval_libc_i32("F_FULLFSYNC")? { let &[_, _] = check_arg_count(args)?; - if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.get(&fd) { - let io_result = maybe_sync_file(file, *writable, File::sync_all); - this.try_unwrap_io_result(io_result) + if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { + match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => { + let io_result = maybe_sync_file(&file, *writable, File::sync_all); + this.try_unwrap_io_result(io_result) + }, + Err(_) => this.handle_not_found(), + } } else { this.handle_not_found() } @@ -412,24 +447,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; - if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.remove(&fd) { - // We sync the file if it was opened in a mode different than read-only. - if writable { - // `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(file.sync_all().map(|_| 0i32)); - // Now we actually close the file. - drop(file); - // And return the result. - result - } else { - // We drop the file, this closes it but ignores any errors produced when closing - // it. This is done because `File::sync_all` cannot be done over files like - // `/dev/urandom` which are read-only. Check - // https://github.com/rust-lang/miri/issues/999#issuecomment-568920439 for a deeper - // discussion. - drop(file); - Ok(0) + if let Some(file_descriptor) = this.machine.file_handler.handles.remove(&fd) { + match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => { + // We sync the file if it was opened in a mode different than read-only. + if *writable { + // `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(file.sync_all().map(|_| 0i32)); + // Now we actually close the file. + drop(file); + // And return the result. + result + } else { + // We drop the file, this closes it but ignores any errors produced when closing + // it. This is done because `File::sync_all` cannot be done over files like + // `/dev/urandom` which are read-only. Check + // https://github.com/rust-lang/miri/issues/999#issuecomment-568920439 for a deeper + // discussion. + drop(file); + Ok(0) + } + }, + Err(_) => this.handle_not_found() } } else { this.handle_not_found() @@ -460,15 +500,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // host's and target's `isize`. This saves us from having to handle overflows later. let count = count.min(this.machine_isize_max() as u64).min(isize::MAX as u64); - if let Some(FileHandle { file, writable: _ }) = this.machine.file_handler.handles.get_mut(&fd) { - trace!("read: FD mapped to {:?}", file); + if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { + trace!("read: FD mapped to {:?}", file_descriptor); // 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` because it is a host's `isize`. let mut bytes = vec![0; count as usize]; - let result = file + // `File::read` never returns a value larger than `count`, + // so this cannot fail. + let result = file_descriptor .read(&mut bytes) - // `File::read` never returns a value larger than `count`, so this cannot fail. .map(|c| i64::try_from(c).unwrap()); match result { @@ -510,9 +551,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // host's and target's `isize`. This saves us from having to handle overflows later. let count = count.min(this.machine_isize_max() as u64).min(isize::MAX as u64); - if let Some(FileHandle { file, writable: _ }) = this.machine.file_handler.handles.get_mut(&fd) { + if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; - let result = file.write(&bytes).map(|c| i64::try_from(c).unwrap()); + let result = file_descriptor + .write(&bytes) + .map(|c| i64::try_from(c).unwrap()); this.try_unwrap_io_result(result) } else { this.handle_not_found() @@ -545,8 +588,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(-1); }; - if let Some(FileHandle { file, writable: _ }) = this.machine.file_handler.handles.get_mut(&fd) { - let result = file.seek(seek_from).map(|offset| i64::try_from(offset).unwrap()); + if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { + let result = file_descriptor + .seek(seek_from) + .map(|offset| i64::try_from(offset).unwrap()); this.try_unwrap_io_result(result) } else { this.handle_not_found() @@ -1103,21 +1148,26 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; let length = this.read_scalar(length_op)?.to_i64()?; - if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.get_mut(&fd) { - if *writable { - if let Ok(length) = length.try_into() { - let result = file.set_len(length); - this.try_unwrap_io_result(result.map(|_| 0i32)) - } else { - let einval = this.eval_libc("EINVAL")?; - this.set_last_error(einval)?; - Ok(-1) + if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { + match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => { + if *writable { + if let Ok(length) = length.try_into() { + let result = file.set_len(length); + this.try_unwrap_io_result(result.map(|_| 0i32)) + } else { + let einval = this.eval_libc("EINVAL")?; + this.set_last_error(einval)?; + Ok(-1) + } + } else { + // The file is not writable + let einval = this.eval_libc("EINVAL")?; + this.set_last_error(einval)?; + Ok(-1) + } } - } else { - // The file is not writable - let einval = this.eval_libc("EINVAL")?; - this.set_last_error(einval)?; - Ok(-1) + Err(_) => this.handle_not_found() } } else { this.handle_not_found() @@ -1135,9 +1185,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("fsync")?; let fd = this.read_scalar(fd_op)?.to_i32()?; - if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.get(&fd) { - let io_result = maybe_sync_file(file, *writable, File::sync_all); - this.try_unwrap_io_result(io_result) + if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { + match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => { + let io_result = maybe_sync_file(&file, *writable, File::sync_all); + this.try_unwrap_io_result(io_result) + } + Err(_) => this.handle_not_found() + } } else { this.handle_not_found() } @@ -1149,9 +1204,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.check_no_isolation("fdatasync")?; let fd = this.read_scalar(fd_op)?.to_i32()?; - if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.get(&fd) { - let io_result = maybe_sync_file(file, *writable, File::sync_data); - this.try_unwrap_io_result(io_result) + if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { + match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => { + let io_result = maybe_sync_file(&file, *writable, File::sync_data); + this.try_unwrap_io_result(io_result) + } + Err(_) => this.handle_not_found() + } } else { this.handle_not_found() } @@ -1187,9 +1247,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return Ok(-1); } - if let Some(FileHandle { file, writable }) = this.machine.file_handler.handles.get(&fd) { - let io_result = maybe_sync_file(file, *writable, File::sync_data); - this.try_unwrap_io_result(io_result) + if let Some(file_descriptor) = this.machine.file_handler.handles.get(&fd) { + match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable }) => { + let io_result = maybe_sync_file(&file, *writable, File::sync_data); + this.try_unwrap_io_result(io_result) + }, + Err(_) => this.handle_not_found() + } } else { this.handle_not_found() } @@ -1239,7 +1304,10 @@ impl FileMetadata { ) -> InterpResult<'tcx, Option> { let option = ecx.machine.file_handler.handles.get(&fd); let file = match option { - Some(FileHandle { file, writable: _ }) => file, + Some(file_descriptor) => match file_descriptor.as_file_handle() { + Ok(FileHandle { file, writable: _ }) => file, + Err(_) => return ecx.handle_not_found().map(|_: i32| None), + }, None => return ecx.handle_not_found().map(|_: i32| None), }; let metadata = file.metadata(); From 3386f12eca5db75ee679c5d08fecec88ae99e6a0 Mon Sep 17 00:00:00 2001 From: Samrat Man Singh Date: Mon, 3 Aug 2020 11:01:42 +0530 Subject: [PATCH 2/3] Wrap io::Result from `FileDescriptor::{read,write,seek}` in InterpResult The outer InterpResult will be used to indicate that a fn is not implemented for a struct(eg. `write` for Stdin). The inner io::Result is just the result from the read/write/seek. --- src/shims/posix/fs.rs | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index 5415e9e1f77c..6f46401ece65 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -25,9 +25,9 @@ struct FileHandle { trait FileDescriptor<'tcx> : std::fmt::Debug { fn as_file_handle(&self) -> InterpResult<'tcx, &FileHandle>; - fn read(&mut self, bytes: &mut [u8]) -> Result; - fn write(&mut self, bytes: &[u8]) -> Result; - fn seek(&mut self, offset: SeekFrom) -> Result; + fn read(&mut self, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result>; + fn write(&mut self, bytes: &[u8]) -> InterpResult<'tcx, io::Result>; + fn seek(&mut self, offset: SeekFrom) -> InterpResult<'tcx, io::Result>; } impl<'tcx> FileDescriptor<'tcx> for FileHandle { @@ -35,16 +35,16 @@ impl<'tcx> FileDescriptor<'tcx> for FileHandle { Ok(&self) } - fn read(&mut self, bytes: &mut [u8]) -> Result { - self.file.read(bytes) + fn read(&mut self, bytes: &mut [u8]) -> InterpResult<'tcx, io::Result> { + Ok(self.file.read(bytes)) } - fn write(&mut self, bytes: &[u8]) -> Result { - self.file.write(bytes) + fn write(&mut self, bytes: &[u8]) -> InterpResult<'tcx, io::Result> { + Ok(self.file.write(bytes)) } - fn seek(&mut self, offset: SeekFrom) -> Result { - self.file.seek(offset) + fn seek(&mut self, offset: SeekFrom) -> InterpResult<'tcx, io::Result> { + Ok(self.file.seek(offset)) } } @@ -509,7 +509,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // `File::read` never returns a value larger than `count`, // so this cannot fail. let result = file_descriptor - .read(&mut bytes) + .read(&mut bytes)? .map(|c| i64::try_from(c).unwrap()); match result { @@ -554,7 +554,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { let bytes = this.memory.read_bytes(buf, Size::from_bytes(count))?; let result = file_descriptor - .write(&bytes) + .write(&bytes)? .map(|c| i64::try_from(c).unwrap()); this.try_unwrap_io_result(result) } else { @@ -590,7 +590,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some(file_descriptor) = this.machine.file_handler.handles.get_mut(&fd) { let result = file_descriptor - .seek(seek_from) + .seek(seek_from)? .map(|offset| i64::try_from(offset).unwrap()); this.try_unwrap_io_result(result) } else { From 79e066fc95c036e64716a4222c580782a9c932c2 Mon Sep 17 00:00:00 2001 From: Samrat Man Singh Date: Mon, 3 Aug 2020 20:39:09 +0530 Subject: [PATCH 3/3] Remove unnecessary `clone()` on `writable` --- src/shims/posix/fs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index 6f46401ece65..1bba30a1ea0f 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -411,13 +411,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fh = &mut this.machine.file_handler; let (file_result, writable) = match fh.handles.get(&fd) { Some(file_descriptor) => match file_descriptor.as_file_handle() { - Ok(FileHandle { file, writable }) => (file.try_clone(), writable.clone()), + Ok(FileHandle { file, writable }) => (file.try_clone(), *writable), Err(_) => return this.handle_not_found(), }, None => return this.handle_not_found(), }; let fd_result = file_result.map(|duplicated| { - fh.insert_fd_with_min_fd(FileHandle { file: duplicated, writable: writable }, start) + fh.insert_fd_with_min_fd(FileHandle { file: duplicated, writable }, start) }); this.try_unwrap_io_result(fd_result) } else if this.tcx.sess.target.target.target_os == "macos"