Skip to content

Commit

Permalink
Auto merge of rust-lang#3852 - tiif:rwrefactor, r=RalfJung
Browse files Browse the repository at this point in the history 
Refactor fd read/write

This PR passed the responsibility of reading to user supplied buffer and dest place to each implementation of ``FileDescription::read/write/pread/pwrite``.

This is part of rust-lang#3665.
  • Loading branch information
@bors
bors committed Sep 22, 2024
2 parents 8243a8c + d70fd88 commit 599b329
Show file tree
Hide file tree
Showing 5 changed files with 249 additions and 153 deletions.
192 changes: 127 additions & 65 deletions src/tools/miri/src/shims/unix/fd.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,49 +25,64 @@ pub(crate) enum FlockOp {
pub trait FileDescription: std::fmt::Debug + Any {
fn name(&self) -> &'static str;

/// Reads as much as possible into the given buffer, and returns the number of bytes read.
/// Reads as much as possible into the given buffer `ptr`.
/// `len` indicates how many bytes we should try to read.
/// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error.
fn read<'tcx>(
&self,
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
_bytes: &mut [u8],
_ptr: Pointer,
_len: usize,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot read from {}", self.name());
}

/// Writes as much as possible from the given buffer, and returns the number of bytes written.
/// Writes as much as possible from the given buffer `ptr`.
/// `len` indicates how many bytes we should try to write.
/// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error.
fn write<'tcx>(
&self,
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
_bytes: &[u8],
_ptr: Pointer,
_len: usize,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot write to {}", self.name());
}

/// Reads as much as possible into the given buffer from a given offset,
/// and returns the number of bytes read.
/// Reads as much as possible into the given buffer `ptr` from a given offset.
/// `len` indicates how many bytes we should try to read.
/// `dest` is where the return value should be stored: number of bytes read, or `-1` in case of error.
fn pread<'tcx>(
&self,
_communicate_allowed: bool,
_bytes: &mut [u8],
_offset: u64,
_ptr: Pointer,
_len: usize,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot pread from {}", self.name());
}

/// Writes as much as possible from the given buffer starting at a given offset,
/// and returns the number of bytes written.
/// Writes as much as possible from the given buffer `ptr` starting at a given offset.
/// `ptr` is the pointer to the user supplied read buffer.
/// `len` indicates how many bytes we should try to write.
/// `dest` is where the return value should be stored: number of bytes written, or `-1` in case of error.
fn pwrite<'tcx>(
&self,
_communicate_allowed: bool,
_bytes: &[u8],
_ptr: Pointer,
_len: usize,
_offset: u64,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot pwrite to {}", self.name());
}

Expand Down Expand Up @@ -125,14 +140,18 @@ impl FileDescription for io::Stdin {
&self,
_self_ref: &FileDescriptionRef,
communicate_allowed: bool,
bytes: &mut [u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
ptr: Pointer,
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
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.
helpers::isolation_abort_error("`read` from stdin")?;
}
Ok(Read::read(&mut { self }, bytes))
let result = Read::read(&mut { self }, &mut bytes);
ecx.return_read_bytes_and_count(ptr, &bytes, result, dest)
}

fn is_tty(&self, communicate_allowed: bool) -> bool {
Expand All @@ -149,9 +168,12 @@ impl FileDescription for io::Stdout {
&self,
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
bytes: &[u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
ptr: Pointer,
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
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);
// Stdout is buffered, flush to make sure it appears on the
Expand All @@ -160,8 +182,7 @@ impl FileDescription for io::Stdout {
// the host -- there is no good in adding extra buffering
// here.
io::stdout().flush().unwrap();

Ok(result)
ecx.return_written_byte_count_or_error(result, dest)
}

fn is_tty(&self, communicate_allowed: bool) -> bool {
Expand All @@ -178,12 +199,16 @@ impl FileDescription for io::Stderr {
&self,
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
bytes: &[u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
ptr: Pointer,
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
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.
Ok(Write::write(&mut { self }, bytes))
let result = Write::write(&mut { self }, bytes);
ecx.return_written_byte_count_or_error(result, dest)
}

fn is_tty(&self, communicate_allowed: bool) -> bool {
Expand All @@ -204,11 +229,14 @@ impl FileDescription for NullOutput {
&self,
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
bytes: &[u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
_ptr: Pointer,
len: usize,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
// We just don't write anything, but report to the user that we did.
Ok(Ok(bytes.len()))
let result = Ok(len);
ecx.return_written_byte_count_or_error(result, dest)
}
}

Expand Down Expand Up @@ -535,7 +563,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
buf: Pointer,
count: u64,
offset: Option<i128>,
) -> InterpResult<'tcx, Scalar> {
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();

// Isolation check is done via `FileDescription` trait.
Expand All @@ -550,48 +579,35 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let count = count
.min(u64::try_from(this.target_isize_max()).unwrap())
.min(u64::try_from(isize::MAX).unwrap());
let count = usize::try_from(count).unwrap(); // now it fits in a `usize`
let communicate = this.machine.communicate();

// We temporarily dup the FD to be able to retain mutable access to `this`.
let Some(fd) = this.machine.fds.get(fd_num) else {
trace!("read: FD not found");
return Ok(Scalar::from_target_isize(this.fd_not_found()?, this));
let res: i32 = this.fd_not_found()?;
this.write_int(res, dest)?;
return Ok(());
};

trace!("read: FD mapped to {fd:?}");
// 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` because it is bounded by the host's `isize`.
let mut bytes = vec![0; usize::try_from(count).unwrap()];
let result = match offset {
None => fd.read(&fd, communicate, &mut bytes, this),

match offset {
None => fd.read(&fd, communicate, buf, count, dest, this)?,
Some(offset) => {
let Ok(offset) = u64::try_from(offset) else {
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
return Ok(Scalar::from_target_isize(-1, this));
this.write_int(-1, dest)?;
return Ok(());
};
fd.pread(communicate, &mut bytes, offset, this)
fd.pread(communicate, offset, buf, count, dest, this)?
}
};

// `File::read` never returns a value larger than `count`, so this cannot fail.
match result?.map(|c| i64::try_from(c).unwrap()) {
Ok(read_bytes) => {
// 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
// that much into the output buffer!
this.write_bytes_ptr(
buf,
bytes[..usize::try_from(read_bytes).unwrap()].iter().copied(),
)?;
Ok(Scalar::from_target_isize(read_bytes, this))
}
Err(e) => {
this.set_last_error_from_io_error(e)?;
Ok(Scalar::from_target_isize(-1, this))
}
}
Ok(())
}

fn write(
Expand All @@ -600,7 +616,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
buf: Pointer,
count: u64,
offset: Option<i128>,
) -> InterpResult<'tcx, Scalar> {
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();

// Isolation check is done via `FileDescription` trait.
Expand All @@ -613,27 +630,72 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let count = count
.min(u64::try_from(this.target_isize_max()).unwrap())
.min(u64::try_from(isize::MAX).unwrap());
let count = usize::try_from(count).unwrap(); // now it fits in a `usize`
let communicate = this.machine.communicate();

let bytes = this.read_bytes_ptr_strip_provenance(buf, Size::from_bytes(count))?.to_owned();
// We temporarily dup the FD to be able to retain mutable access to `this`.
let Some(fd) = this.machine.fds.get(fd_num) else {
return Ok(Scalar::from_target_isize(this.fd_not_found()?, this));
let res: i32 = this.fd_not_found()?;
this.write_int(res, dest)?;
return Ok(());
};

let result = match offset {
None => fd.write(&fd, communicate, &bytes, this),
match offset {
None => fd.write(&fd, communicate, buf, count, dest, this)?,
Some(offset) => {
let Ok(offset) = u64::try_from(offset) else {
let einval = this.eval_libc("EINVAL");
this.set_last_error(einval)?;
return Ok(Scalar::from_target_isize(-1, this));
this.write_int(-1, dest)?;
return Ok(());
};
fd.pwrite(communicate, &bytes, offset, this)
fd.pwrite(communicate, buf, count, offset, dest, this)?
}
};
Ok(())
}

let result = result?.map(|c| i64::try_from(c).unwrap());
Ok(Scalar::from_target_isize(this.try_unwrap_io_result(result)?, this))
/// Helper to implement `FileDescription::read`:
/// `result` should be the return value of some underlying `read` call that used `bytes` as its output buffer.
/// The length of `bytes` must not exceed either the host's or the target's `isize`.
/// If `Result` indicates success, `bytes` is written to `buf` and the size is written to `dest`.
/// Otherwise, `-1` is written to `dest` and the last libc error is set appropriately.
fn return_read_bytes_and_count(
&mut self,
buf: Pointer,
bytes: &[u8],
result: io::Result<usize>,
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
match result {
Ok(read_bytes) => {
// 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
// that much into the output buffer!
this.write_bytes_ptr(buf, bytes[..read_bytes].iter().copied())?;
// The actual read size is always less than what got originally requested so this cannot fail.
this.write_int(u64::try_from(read_bytes).unwrap(), dest)?;
return Ok(());
}
Err(e) => {
this.set_last_error_from_io_error(e)?;
this.write_int(-1, dest)?;
return Ok(());
}
}
}

/// This function writes the number of written bytes (given in `result`) to `dest`, or sets the
/// last libc error and writes -1 to dest.
fn return_written_byte_count_or_error(
&mut self,
result: io::Result<usize>,
dest: &MPlaceTy<'tcx>,
) -> InterpResult<'tcx> {
let this = self.eval_context_mut();
let result = this.try_unwrap_io_result(result.map(|c| i64::try_from(c).unwrap()))?;
this.write_int(result, dest)?;
Ok(())
}
}
21 changes: 6 additions & 15 deletions src/tools/miri/src/shims/unix/foreign_items.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,27 +92,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let fd = this.read_scalar(fd)?.to_i32()?;
let buf = this.read_pointer(buf)?;
let count = this.read_target_usize(count)?;
let result = this.read(fd, buf, count, None)?;
this.write_scalar(result, dest)?;
this.read(fd, buf, count, None, dest)?;
}
"write" => {
let [fd, buf, n] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let fd = this.read_scalar(fd)?.to_i32()?;
let buf = this.read_pointer(buf)?;
let count = this.read_target_usize(n)?;
trace!("Called write({:?}, {:?}, {:?})", fd, buf, count);
let result = this.write(fd, buf, count, None)?;
// Now, `result` is the value we return back to the program.
this.write_scalar(result, dest)?;
this.write(fd, buf, count, None, dest)?;
}
"pread" => {
let [fd, buf, count, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let fd = this.read_scalar(fd)?.to_i32()?;
let buf = this.read_pointer(buf)?;
let count = this.read_target_usize(count)?;
let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off_t").size)?;
let result = this.read(fd, buf, count, Some(offset))?;
this.write_scalar(result, dest)?;
this.read(fd, buf, count, Some(offset), dest)?;
}
"pwrite" => {
let [fd, buf, n, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand All @@ -121,18 +117,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let count = this.read_target_usize(n)?;
let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off_t").size)?;
trace!("Called pwrite({:?}, {:?}, {:?}, {:?})", fd, buf, count, offset);
let result = this.write(fd, buf, count, Some(offset))?;
// Now, `result` is the value we return back to the program.
this.write_scalar(result, dest)?;
this.write(fd, buf, count, Some(offset), dest)?;
}
"pread64" => {
let [fd, buf, count, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let fd = this.read_scalar(fd)?.to_i32()?;
let buf = this.read_pointer(buf)?;
let count = this.read_target_usize(count)?;
let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off64_t").size)?;
let result = this.read(fd, buf, count, Some(offset))?;
this.write_scalar(result, dest)?;
this.read(fd, buf, count, Some(offset), dest)?;
}
"pwrite64" => {
let [fd, buf, n, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand All @@ -141,9 +134,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
let count = this.read_target_usize(n)?;
let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off64_t").size)?;
trace!("Called pwrite64({:?}, {:?}, {:?}, {:?})", fd, buf, count, offset);
let result = this.write(fd, buf, count, Some(offset))?;
// Now, `result` is the value we return back to the program.
this.write_scalar(result, dest)?;
this.write(fd, buf, count, Some(offset), dest)?;
}
"close" => {
let [fd] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand Down
Loading

0 comments on commit 599b329

Please sign in to comment.