From 260b463bb037809e9558421ad5e73bf89e9a08df Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Thu, 24 Oct 2019 08:44:13 -0500 Subject: [PATCH] Clean file handling functions --- src/shims/fs.rs | 116 ++++++++++++++++++------------------------------ 1 file changed, 44 insertions(+), 72 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index c484795d8fec..760fc1daff1f 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()?; + let buf = 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)) - }) + 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()?)?; + let buf = 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?) - }) + 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` 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>, - { + /// 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` instead of `i32` directly because some fs functions return different integer + /// types (like `read`, that returns an `i64`). + fn handle_not_found>(&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` 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>, - { - 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()) } }