Skip to content
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.

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

Implement Windows file read, write, and delete APIs. #4043

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
48 changes: 34 additions & 14 deletions src/shims/files.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::any::Any;
use std::collections::BTreeMap;
use std::io::{IsTerminal, Read, SeekFrom, Write};
use std::io::{ErrorKind, IsTerminal, Read, SeekFrom, Write};
use std::ops::Deref;
use std::rc::{Rc, Weak};
use std::{fs, io};
Expand All @@ -25,7 +25,7 @@ pub trait FileDescription: std::fmt::Debug + Any {
_len: usize,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
throw_unsup_format!("cannot read from {}", self.name());
}

Expand All @@ -40,7 +40,7 @@ pub trait FileDescription: std::fmt::Debug + Any {
_len: usize,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
throw_unsup_format!("cannot write to {}", self.name());
}

Expand Down Expand Up @@ -97,7 +97,7 @@ impl FileDescription for io::Stdin {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let mut bytes = vec![0; len];
if !communicate_allowed {
// We want isolation mode to be deterministic, so we have to disallow all reads, even stdin.
Expand All @@ -106,7 +106,10 @@ impl FileDescription for io::Stdin {
let result = Read::read(&mut { self }, &mut bytes);
match result {
Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest),
Err(e) => ecx.set_last_error_and_return(e, dest),
Err(e) => {
ecx.write_int(-1, dest)?;
interp_ok(Err(e.kind()))
}
}
}

Expand All @@ -128,7 +131,7 @@ impl FileDescription for io::Stdout {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?;
// We allow writing to stderr even with isolation enabled.
let result = Write::write(&mut { self }, bytes);
Expand All @@ -140,7 +143,10 @@ impl FileDescription for io::Stdout {
io::stdout().flush().unwrap();
match result {
Ok(write_size) => ecx.return_write_success(write_size, dest),
Err(e) => ecx.set_last_error_and_return(e, dest),
Err(e) => {
ecx.write_int(-1, dest)?;
interp_ok(Err(e.kind()))
}
}
}

Expand All @@ -162,14 +168,17 @@ impl FileDescription for io::Stderr {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?;
// We allow writing to stderr even with isolation enabled.
// No need to flush, stderr is not buffered.
let result = Write::write(&mut { self }, bytes);
match result {
Ok(write_size) => ecx.return_write_success(write_size, dest),
Err(e) => ecx.set_last_error_and_return(e, dest),
Err(e) => {
ecx.write_int(-1, dest)?;
interp_ok(Err(e.kind()))
}
}
}

Expand All @@ -195,7 +204,7 @@ impl FileDescription for NullOutput {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
// We just don't write anything, but report to the user that we did.
ecx.return_write_success(len, dest)
}
Expand Down Expand Up @@ -384,7 +393,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
bytes: &[u8],
actual_read_size: usize,
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let this = self.eval_context_mut();
// If reading to `bytes` did not fail, we write those bytes to the buffer.
// Crucially, if fewer than `bytes.len()` bytes were read, only write
Expand All @@ -393,7 +402,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {

// The actual read size is always less than what got originally requested so this cannot fail.
this.write_int(u64::try_from(actual_read_size).unwrap(), dest)?;
interp_ok(())
interp_ok(Ok(()))
}

/// Helper to implement `FileDescription::write`:
Expand All @@ -402,10 +411,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
&mut self,
actual_write_size: usize,
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
Copy link
Member

Choose a reason for hiding this comment

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

If you have to completely change how we do error handling for IO functions on Unix, please make that a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. I wanted to make the change to this branch first to ensure it would work well and pass local tests with both the existing unix code, and my new Windows shims. I already intended to split this change off and merge it separately.

let this = self.eval_context_mut();
// The actual write size is always less than what got originally requested so this cannot fail.
this.write_int(u64::try_from(actual_write_size).unwrap(), dest)?;
interp_ok(())
interp_ok(Ok(()))
}

fn return_io_error(
&mut self,
error: ErrorKind,
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let this = self.eval_context_mut();
this.set_last_error(error)?;
this.write_int(-1, dest)?;
interp_ok(Err(error))
}
}
12 changes: 10 additions & 2 deletions src/shims/unix/fd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// `usize::MAX` because it is bounded by the host's `isize`.

match offset {
None => fd.read(&fd, communicate, buf, count, dest, this)?,
None =>
match fd.read(&fd, communicate, buf, count, dest, this)? {
Ok(_) => (),
Err(e) => this.set_last_error(e)?,
},
Some(offset) => {
let Ok(offset) = u64::try_from(offset) else {
return this.set_last_error_and_return(LibcError("EINVAL"), dest);
Expand Down Expand Up @@ -286,7 +290,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
};

match offset {
None => fd.write(&fd, communicate, buf, count, dest, this)?,
None =>
match fd.write(&fd, communicate, buf, count, dest, this)? {
Ok(_) => (),
Err(e) => this.set_last_error(e)?,
},
Some(offset) => {
let Ok(offset) = u64::try_from(offset) else {
return this.set_last_error_and_return(LibcError("EINVAL"), dest);
Expand Down
18 changes: 12 additions & 6 deletions src/shims/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,16 @@ impl FileDescription for FileHandle {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), ErrorKind>> {
assert!(communicate_allowed, "isolation should have prevented even opening a file");
let mut bytes = vec![0; len];
let result = (&mut &self.file).read(&mut bytes);
match result {
Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest),
Err(e) => ecx.set_last_error_and_return(e, dest),
Err(e) => {
ecx.set_last_error_and_return(e.kind(), dest)?;
interp_ok(Err(e.kind()))
}
}
}

Expand All @@ -56,13 +59,16 @@ impl FileDescription for FileHandle {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
assert!(communicate_allowed, "isolation should have prevented even opening a file");
let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?;
let result = (&mut &self.file).write(bytes);
match result {
Ok(write_size) => ecx.return_write_success(write_size, dest),
Err(e) => ecx.set_last_error_and_return(e, dest),
Err(e) => {
ecx.set_last_error_and_return(e.kind(), dest)?;
interp_ok(Err(e.kind()))
}
}
}

Expand Down Expand Up @@ -141,7 +147,7 @@ impl UnixFileDescription for FileHandle {
};
let result = f();
match result {
Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest),
Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest).map(|_| ()),
Err(e) => ecx.set_last_error_and_return(e, dest),
}
}
Expand Down Expand Up @@ -172,7 +178,7 @@ impl UnixFileDescription for FileHandle {
};
let result = f();
match result {
Ok(write_size) => ecx.return_write_success(write_size, dest),
Ok(write_size) => ecx.return_write_success(write_size, dest).map(|_| ()),
Err(e) => ecx.set_last_error_and_return(e, dest),
}
}
Expand Down
42 changes: 26 additions & 16 deletions src/shims/unix/linux_like/eventfd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use std::io;
use std::io::ErrorKind;

use crate::concurrency::VClock;
use crate::shims::files::{FileDescription, FileDescriptionRef, WeakFileDescriptionRef};
use crate::shims::files::{
EvalContextExt as _, FileDescription, FileDescriptionRef, WeakFileDescriptionRef,
};
use crate::shims::unix::UnixFileDescription;
use crate::shims::unix::linux_like::epoll::{EpollReadyEvents, EvalContextExt as _};
use crate::*;
Expand Down Expand Up @@ -54,12 +56,12 @@ impl FileDescription for Event {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
// We're treating the buffer as a `u64`.
let ty = ecx.machine.layouts.u64;
// Check the size of slice, and return error only if the size of the slice < 8.
if len < ty.size.bytes_usize() {
return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest);
return ecx.return_io_error(ErrorKind::InvalidInput, dest);
}

// eventfd read at the size of u64.
Expand Down Expand Up @@ -89,12 +91,12 @@ impl FileDescription for Event {
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
// We're treating the buffer as a `u64`.
let ty = ecx.machine.layouts.u64;
// Check the size of slice, and return error only if the size of the slice < 8.
if len < ty.layout.size.bytes_usize() {
return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest);
return ecx.return_io_error(ErrorKind::InvalidInput, dest);
}

// Read the user-supplied value from the pointer.
Expand All @@ -103,7 +105,7 @@ impl FileDescription for Event {

// u64::MAX as input is invalid because the maximum value of counter is u64::MAX - 1.
if num == u64::MAX {
return ecx.set_last_error_and_return(ErrorKind::InvalidInput, dest);
return ecx.return_io_error(ErrorKind::InvalidInput, dest);
}
// If the addition does not let the counter to exceed the maximum value, update the counter.
// Else, block.
Expand Down Expand Up @@ -198,7 +200,7 @@ fn eventfd_write<'tcx>(
dest: &MPlaceTy<'tcx>,
weak_eventfd: WeakFileDescriptionRef,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let Some(eventfd_ref) = weak_eventfd.upgrade() else {
throw_unsup_format!("eventfd FD got closed while blocking.")
};
Expand Down Expand Up @@ -232,12 +234,13 @@ fn eventfd_write<'tcx>(
}

// Return how many bytes we consumed from the user-provided buffer.
return ecx.write_int(buf_place.layout.size.bytes(), dest);
ecx.write_int(buf_place.layout.size.bytes(), dest)?;
return interp_ok(Ok(()));
}
None | Some(u64::MAX) => {
// We can't update the state, so we have to block.
if eventfd.is_nonblock {
return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest);
return ecx.return_io_error(ErrorKind::WouldBlock, dest);
}

let dest = dest.clone();
Expand All @@ -256,13 +259,16 @@ fn eventfd_write<'tcx>(
}
@unblock = |this| {
// When we get unblocked, try again.
eventfd_write(num, buf_place, &dest, weak_eventfd, this)
match eventfd_write(num, buf_place, &dest, weak_eventfd, this)? {
Ok(_) => interp_ok(()),
Err(e) => this.set_last_error(e),
}
}
),
);
}
};
interp_ok(())
interp_ok(Ok(()))
}

/// Block thread if the current counter is 0,
Expand All @@ -272,7 +278,7 @@ fn eventfd_read<'tcx>(
dest: &MPlaceTy<'tcx>,
weak_eventfd: WeakFileDescriptionRef,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
) -> InterpResult<'tcx, Result<(), io::ErrorKind>> {
let Some(eventfd_ref) = weak_eventfd.upgrade() else {
throw_unsup_format!("eventfd FD got closed while blocking.")
};
Expand All @@ -287,7 +293,7 @@ fn eventfd_read<'tcx>(
// Block when counter == 0.
if counter == 0 {
if eventfd.is_nonblock {
return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest);
return ecx.return_io_error(ErrorKind::WouldBlock, dest);
}
let dest = dest.clone();

Expand All @@ -304,7 +310,10 @@ fn eventfd_read<'tcx>(
}
@unblock = |this| {
// When we get unblocked, try again.
eventfd_read(buf_place, &dest, weak_eventfd, this)
match eventfd_read(buf_place, &dest, weak_eventfd, this)? {
Ok(_) => interp_ok(()),
Err(e) => this.set_last_error(e),
}
}
),
);
Expand All @@ -330,7 +339,8 @@ fn eventfd_read<'tcx>(
}

// Tell userspace how many bytes we put into the buffer.
return ecx.write_int(buf_place.layout.size.bytes(), dest);
ecx.write_int(buf_place.layout.size.bytes(), dest)?;
return interp_ok(Ok(()));
}
interp_ok(())
interp_ok(Ok(()))
}
Loading