Skip to content

Commit

Permalink
Auto merge of #1022 - christianpoveda:fix-fd-access, r=RalfJung
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@bors
bors committed Nov 13, 2019
2 parents 69415b4 + 4baef71 commit 09b0a8a
Showing 1 changed file with 84 additions and 79 deletions.
163 changes: 84 additions & 79 deletions src/shims/fs.rs
View file
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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(
Expand All @@ -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(
Expand All @@ -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")?;
Expand All @@ -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())
}
}

0 comments on commit 09b0a8a

Please sign in to comment.