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.

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

Jump to bottom

Refactor fd read/write #3852

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Refactor fd read/write #3852

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
de515da
Refactor FileDescription::read/write
tiif Sep 16, 2024
ef43a9b
Use &[u8] instead of Vec<u8>
tiif Sep 17, 2024
a02e399
Update src/shims/unix/fd.rs
tiif Sep 17, 2024
b82a909
Update src/shims/unix/fd.rs
tiif Sep 17, 2024
0363884
Update src/shims/unix/fd.rs
tiif Sep 17, 2024
bb6d03f
Update src/shims/unix/fd.rs
tiif Sep 17, 2024
b588fba
Add documentation for read/write parameters
tiif Sep 17, 2024
274e9c9
Pass pointer and len to FileDescription::write and change the type of…
tiif Sep 17, 2024
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
151 changes: 98 additions & 53 deletions src/shims/unix/fd.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,16 @@ 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.
/// `ptr` is the pointer to user supplied read buffer.
tiif marked this conversation as resolved.
Show resolved Hide resolved
fn read<'tcx>(
&self,
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
_bytes: &mut [u8],
_ptr: Pointer,
tiif marked this conversation as resolved.
Show resolved Hide resolved
_len: u64,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot read from {}", self.name());
}

Expand All @@ -42,8 +45,9 @@ pub trait FileDescription: std::fmt::Debug + Any {
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
_bytes: &[u8],
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot write to {}", self.name());
}

Expand All @@ -52,10 +56,12 @@ pub trait FileDescription: std::fmt::Debug + Any {
fn pread<'tcx>(
&self,
_communicate_allowed: bool,
_bytes: &mut [u8],
_offset: u64,
_ptr: Pointer,
_len: u64,
_dest: &MPlaceTy<'tcx>,
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
) -> InterpResult<'tcx> {
throw_unsup_format!("cannot pread from {}", self.name());
}

Expand All @@ -66,8 +72,9 @@ pub trait FileDescription: std::fmt::Debug + Any {
_communicate_allowed: bool,
_bytes: &[u8],
_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 +132,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: u64,
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
let mut bytes = vec![0; usize::try_from(len).unwrap()];
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 @@ -150,8 +161,9 @@ impl FileDescription for io::Stdout {
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
bytes: &[u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
// 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 +172,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 @@ -179,11 +190,13 @@ impl FileDescription for io::Stderr {
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
bytes: &[u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<usize>> {
dest: &MPlaceTy<'tcx>,
ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx> {
// 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 @@ -205,10 +218,12 @@ impl FileDescription for NullOutput {
_self_ref: &FileDescriptionRef,
_communicate_allowed: bool,
bytes: &[u8],
_ecx: &mut MiriInterpCx<'tcx>,
) -> InterpResult<'tcx, io::Result<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(bytes.len());
ecx.return_written_byte_count_or_error(result, dest)
}
}

Expand Down Expand Up @@ -535,7 +550,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 @@ -555,43 +571,29 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
// 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 +602,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 @@ -618,22 +621,64 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
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, &bytes, 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, &bytes, 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))
/// This function either writes to the user supplied buffer and to dest place, or sets the
/// last libc error and writes -1 to dest.
tiif marked this conversation as resolved.
Show resolved Hide resolved
fn return_read_bytes_and_count(
&mut self,
buf: Pointer,
bytes: Vec<u8>,
tiif marked this conversation as resolved.
Show resolved Hide resolved
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 lesser than `count` so this cannot fail.
tiif marked this conversation as resolved.
Show resolved Hide resolved
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 to dest place, or sets the
tiif marked this conversation as resolved.
Show resolved Hide resolved
/// 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/shims/unix/foreign_items.rs
View file
Open in desktop
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