Skip to content

Fix unchecked memory access for files #1022

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Nov 13, 2019
Merged

Fix unchecked memory access for files #1022

Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
260b463
Clean file handling functions
pvdrz Oct 24, 2019
d7967f6
Drop files explicitly when closing them
pvdrz Oct 26, 2019
122549f
Simplify `read` logic
pvdrz Oct 26, 2019
06ef77b
Check for usize to i64 overflows
pvdrz Oct 28, 2019
d0b4407
Fix casts for `count` check
pvdrz Nov 3, 2019
56c5e53
Handle host's `usize` correctly
pvdrz Nov 4, 2019
f910ea1
Avoid using `as` cast
pvdrz Nov 5, 2019
4bbaa72
Use TryFrom instead
pvdrz Nov 7, 2019
10b9373
Fix merge conflicts
pvdrz Nov 9, 2019
6e37f2f
Cap `count`
pvdrz Nov 9, 2019
edd0157
Cap `count` twice
pvdrz Nov 13, 2019
4baef71
Fix maximum `isize` value for target
pvdrz Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 84 additions & 79 deletions src/shims/fs.rs
View file
Open in desktop
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);
}
Comment on lines 179 to 181
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: try removing this, now that we don't force_ptr any more.

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())
}
}