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

Show file tree
Hide file tree
Changes from 7 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
14 changes: 14 additions & 0 deletions src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::{mem, iter};
use std::ffi::{OsStr, OsString};
use std::convert::TryFrom;

use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX};
use rustc::mir;
Expand Down Expand Up @@ -459,3 +460,16 @@ fn bytes_to_os_str<'tcx, 'a>(bytes: &'a[u8]) -> InterpResult<'tcx, &'a OsStr> {
.map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", bytes))?;
Ok(&OsStr::new(s))
}

pub fn try_into_host_usize(i: impl Into<u128>) -> Option<usize> {
let i: u128 = i.into();
if i > usize::max_value() as u128 {
None
} else {
Some(i as usize)
}
}

pub fn try_from_host_usize<T: TryFrom<u128>>(i: usize) -> Option<T> {
T::try_from(i as u128).ok()
}
143 changes: 68 additions & 75 deletions src/shims/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -139,9 +142,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 @@ -160,20 +171,31 @@ 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()?;

// 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))
})
let buf = this.read_scalar(buf_op)?.not_undef()?;

if let Some(handle) = this.machine.file_handler.handles.get_mut(&fd) {
let count = helpers::try_into_host_usize(count)
.ok_or_else(|| err_unsup_format!("Program tries to read into buffer too big for this host platform"))?;
// We want to read at most `count` bytes
let mut bytes = vec![0; count];
let result = handle.file.read(&mut bytes);

match result {
Ok(c) => {
let read_bytes = helpers::try_from_host_usize::<i64>(c)
.ok_or_else(|| err_unsup_format!("Number of read bytes {} cannot be transformed to i64", c))?;
Copy link
Member

Choose a reason for hiding this comment

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

So... here we convert to i64 and then, later, we convert to what the target platform really needs. What will happen if that second part is the one that overflows? It's probably a from_int in foreign_items.rs, and that will ICE if the result is too big. So, the error handling isn't really adequate.

But also, do we need this? We know c <= count. What is the largest possible count? You used to_usize, is that right? It seems the type in C is size_t, which would be isize, not usize! So we should know that the buffer is definitely not too big to represent the result.

The only place where we can actually error is if the program does sth too big for the target, but that is already handled above.

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, size_t is unsigned, I was wrong there. But that's silly, what does the original function do if the size fits into a size_t but not an ssize_t?!? I hate C...

Copy link
Member

Choose a reason for hiding this comment

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

@oli-obk any opinions? My conclusion is that the API we are trying to emulate is inherently broken and there is nothing reasonable we can do, so we should do something simple and consistent.^^

We could interpret the count parameter as signed, so if it is "too big" it'll be negative, and then we usize usize::try_from and if that complains we throw an "unsupported big buffer" or so?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the docs on read say

Upon successful completion, read() and pread() shall return a non-negative integer indicating the number of bytes actually read. Otherwise, the functions shall return -1 and set errno to indicate the error.

I would say that any value larger than isize::max_value() (for the target isize) should throw an interpreter error. If the target is 64 bit, it can't happen anyway, because we can't have vectors larger than isize::max_value().

Copy link
Member

@RalfJung RalfJung Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Well, these functions don't have to fill the entire buffer. So I assume what happens is, they truncate saturate the input to isize::MAX and will never use more than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, so the usize we get from Rust's Read::read will never be > i64::max_value(), so for 64bit host platforms interpreting 32 bit target platforms, we basically have to give up when such a situation occurs, since it's too late to do anything about it. Everywhere else, it should be fine

Copy link
Member

Choose a reason for hiding this comment

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

If we truncate the buffer size we pass to Read::read we know the return value can't be bigger than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me see if IIUC. So that basically means being sure that the count parameter doesn't exceeds isize::max_value() so that when we create the intermediate buffer for reading/writing, it isn't larger than isize::max_value() and we can return a proper value at the end. Sounds good to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I figured it out for read. I'm not sure if write is OK tho.

// 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 @@ -192,20 +214,26 @@ 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()?)?;

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?)
})
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);

match result {
Ok(c) => helpers::try_from_host_usize::<i64>(c)
.ok_or_else(|| err_unsup_format!("Number of written bytes {} cannot be transformed to i64", c).into()),
Err(e) => {
this.set_last_error_from_io_error(e)?;
Ok(-1)
}
}
} 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 @@ -217,49 +245,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())
}
}