Auto merge of #1022 - christianpoveda:fix-fd-access, r=RalfJung

Fix unchecked memory access for files

This PR takes care of two problems:

- It uses `Memory::(read|write)_bytes` to guarantee that memory accesses are checked (Fixes: #1007)
- It removes the `(get|remove)_handle_and` methods which were a little bit cumbersome to use. In particular `remove_handle_and`, because we were using it to avoid borrowing issues before the `Evaluator::memory` field was public.

@RalfJung @oli-obk

Author: bors

src/shims/fs.rs

@@ -1,5 +1,6 @@
 use std::collections::HashMap;
-use std::fs::{File, OpenOptions, remove_file};
+use std::convert::TryFrom;
+use std::fs::{remove_file, File, OpenOptions};
 use std::io::{Read, Write};
 
 use rustc::ty::layout::Size;
@@ -125,8 +126,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 +143,17 @@ 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| {
-            this.try_unwrap_io_result(handle.file.sync_all().map(|_| 0i32))
-        })
+        if let Some(handle) = this.machine.file_handler.handles.remove(&fd) {
+            // `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 close the file.
+            drop(handle);
+            // And return the result.
+            result
+        } else {
+            this.handle_not_found()
+        }
     }
 
     fn read(
@@ -154,27 +166,50 @@ 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();
+
+        // 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)) - 1) // max value of target `isize`
+            .min(isize::max_value() as u64);
         // Reading zero bytes should not change `buf`.
         if count == 0 {
             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| {
-                // FIXME: Don't use raw methods
-                this.memory
-                    .get_raw_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) {
+            // 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
+            // `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)
+                // `File::read` never returns a value larger than `count`, so this cannot fail.
+                .map(|c| i64::try_from(c).unwrap());
+
+            match result {
+                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)
+                }
+            }
+        } else {
+            this.handle_not_found()
+        }
     }
 
     fn write(
@@ -187,27 +222,32 @@ 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();
+
+        // 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)) - 1) // max value of target `isize`
+            .min(isize::max_value() as u64);
         // Writing zero bytes should not change `buf`.
         if count == 0 {
             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| {
-            // FIXME: Don't use raw methods
-            let bytes = this.memory.get_raw(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| i64::try_from(c).unwrap());
+            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")?;
@@ -219,49 +259,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())
     }
 }