From f392648c150397159f91092f4da402b638297ad1 Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 17 Sep 2024 00:37:16 +0800 Subject: [PATCH 1/7] Refactor FileDescription::read/write --- src/tools/miri/src/shims/unix/fd.rs | 151 ++++++++++++------ .../miri/src/shims/unix/foreign_items.rs | 21 +-- src/tools/miri/src/shims/unix/fs.rs | 42 +++-- .../miri/src/shims/unix/linux/eventfd.rs | 67 ++++++-- .../miri/src/shims/unix/unnamed_socket.rs | 38 +++-- 5 files changed, 206 insertions(+), 113 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 48bf959538b..5f719991b37 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -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. fn read<'tcx>( &self, _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _bytes: &mut [u8], + _ptr: Pointer, + _len: u64, + _dest: &MPlaceTy<'tcx>, _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + ) -> InterpResult<'tcx> { throw_unsup_format!("cannot read from {}", self.name()); } @@ -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> { + ) -> InterpResult<'tcx> { throw_unsup_format!("cannot write to {}", self.name()); } @@ -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> { + ) -> InterpResult<'tcx> { throw_unsup_format!("cannot pread from {}", self.name()); } @@ -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> { + ) -> InterpResult<'tcx> { throw_unsup_format!("cannot pwrite to {}", self.name()); } @@ -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> { + 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 { @@ -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> { + 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 @@ -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 { @@ -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> { + 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 { @@ -205,10 +218,12 @@ impl FileDescription for NullOutput { _self_ref: &FileDescriptionRef, _communicate_allowed: bool, bytes: &[u8], - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + 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) } } @@ -535,7 +550,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { buf: Pointer, count: u64, offset: Option, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); // Isolation check is done via `FileDescription` trait. @@ -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( @@ -600,7 +602,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { buf: Pointer, count: u64, offset: Option, - ) -> InterpResult<'tcx, Scalar> { + dest: &MPlaceTy<'tcx>, + ) -> InterpResult<'tcx> { let this = self.eval_context_mut(); // Isolation check is done via `FileDescription` trait. @@ -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. + fn return_read_bytes_and_count( + &mut self, + buf: Pointer, + bytes: Vec, + result: io::Result, + 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. + 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 + /// last libc error and writes -1 to dest. + fn return_written_byte_count_or_error( + &mut self, + result: io::Result, + 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(()) } } diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 273a99b3116..f56a66686b9 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -92,8 +92,7 @@ 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)?; @@ -101,9 +100,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)?; @@ -111,8 +108,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)?; @@ -121,9 +117,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("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)?; @@ -131,8 +125,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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)?; @@ -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)?; diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index e1697a47415..cf709d5e6da 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -34,11 +34,15 @@ impl FileDescription for FileHandle { &self, _self_ref: &FileDescriptionRef, communicate_allowed: bool, - bytes: &mut [u8], - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + ptr: Pointer, + len: u64, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - Ok((&mut &self.file).read(bytes)) + let mut bytes = vec![0; usize::try_from(len).unwrap()]; + let result = (&mut &self.file).read(&mut bytes); + ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest) } fn write<'tcx>( @@ -46,20 +50,25 @@ impl FileDescription for FileHandle { _self_ref: &FileDescriptionRef, communicate_allowed: bool, bytes: &[u8], - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - Ok((&mut &self.file).write(bytes)) + let result = (&mut &self.file).write(bytes); + ecx.return_written_byte_count_or_error(result, dest) } fn pread<'tcx>( &self, communicate_allowed: bool, - bytes: &mut [u8], offset: u64, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + ptr: Pointer, + len: u64, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); + let mut bytes = vec![0; usize::try_from(len).unwrap()]; // Emulates pread using seek + read + seek to restore cursor position. // Correctness of this emulation relies on sequential nature of Miri execution. // The closure is used to emulate `try` block, since we "bubble" `io::Error` using `?`. @@ -67,13 +76,14 @@ impl FileDescription for FileHandle { let mut f = || { let cursor_pos = file.stream_position()?; file.seek(SeekFrom::Start(offset))?; - let res = file.read(bytes); + let res = file.read(&mut bytes); // Attempt to restore cursor position even if the read has failed file.seek(SeekFrom::Start(cursor_pos)) .expect("failed to restore file position, this shouldn't be possible"); res }; - Ok(f()) + let result = f(); + ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest) } fn pwrite<'tcx>( @@ -81,8 +91,9 @@ impl FileDescription for FileHandle { communicate_allowed: bool, bytes: &[u8], offset: u64, - _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); // Emulates pwrite using seek + write + seek to restore cursor position. // Correctness of this emulation relies on sequential nature of Miri execution. @@ -97,7 +108,8 @@ impl FileDescription for FileHandle { .expect("failed to restore file position, this shouldn't be possible"); res }; - Ok(f()) + let result = f(); + ecx.return_written_byte_count_or_error(result, dest) } fn seek<'tcx>( diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index 77d16a032aa..2375533fd17 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -62,18 +62,25 @@ impl FileDescription for Event { &self, self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &mut [u8], + ptr: Pointer, + len: u64, + dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + ) -> InterpResult<'tcx> { + // eventfd read at the size of u64. + let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ecx.machine.layouts.u64); // Check the size of slice, and return error only if the size of the slice < 8. - let Some(bytes) = bytes.first_chunk_mut::() else { - return Ok(Err(Error::from(ErrorKind::InvalidInput))); - }; + if len < U64_ARRAY_SIZE.try_into().unwrap() { + let result = Err(Error::from(ErrorKind::InvalidInput)); + return return_read_bytes_and_count_ev(&buf_place, None, result, dest, ecx); + } + // Block when counter == 0. let counter = self.counter.get(); if counter == 0 { if self.is_nonblock { - return Ok(Err(Error::from(ErrorKind::WouldBlock))); + let result = Err(Error::from(ErrorKind::WouldBlock)); + return return_read_bytes_and_count_ev(&buf_place, None, result, dest, ecx); } else { //FIXME: blocking is not supported throw_unsup_format!("eventfd: blocking is unsupported"); @@ -81,17 +88,13 @@ impl FileDescription for Event { } else { // Synchronize with all prior `write` calls to this FD. ecx.acquire_clock(&self.clock.borrow()); - // Return the counter in the host endianness using the buffer provided by caller. - *bytes = match ecx.tcx.sess.target.endian { - Endian::Little => counter.to_le_bytes(), - Endian::Big => counter.to_be_bytes(), - }; + let result = Ok(U64_ARRAY_SIZE); self.counter.set(0); // When any of the event happened, we check and update the status of all supported event // types for current file description. ecx.check_and_update_readiness(self_ref)?; - return Ok(Ok(U64_ARRAY_SIZE)); + return_read_bytes_and_count_ev(&buf_place, Some(counter), result, dest, ecx) } } @@ -112,11 +115,13 @@ impl FileDescription for Event { self_ref: &FileDescriptionRef, _communicate_allowed: bool, bytes: &[u8], + dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + ) -> InterpResult<'tcx> { // Check the size of slice, and return error only if the size of the slice < 8. let Some(bytes) = bytes.first_chunk::() else { - return Ok(Err(Error::from(ErrorKind::InvalidInput))); + let result = Err(Error::from(ErrorKind::InvalidInput)); + return ecx.return_written_byte_count_or_error(result, dest); }; // Convert from bytes to int according to host endianness. let num = match ecx.tcx.sess.target.endian { @@ -125,7 +130,8 @@ 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 Ok(Err(Error::from(ErrorKind::InvalidInput))); + let result = Err(Error::from(ErrorKind::InvalidInput)); + return ecx.return_written_byte_count_or_error(result, dest); } // If the addition does not let the counter to exceed the maximum value, update the counter. // Else, block. @@ -139,7 +145,8 @@ impl FileDescription for Event { } None | Some(u64::MAX) => { if self.is_nonblock { - return Ok(Err(Error::from(ErrorKind::WouldBlock))); + let result = Err(Error::from(ErrorKind::WouldBlock)); + return ecx.return_written_byte_count_or_error(result, dest); } else { //FIXME: blocking is not supported throw_unsup_format!("eventfd: blocking is unsupported"); @@ -150,7 +157,8 @@ impl FileDescription for Event { // types for current file description. ecx.check_and_update_readiness(self_ref)?; - Ok(Ok(U64_ARRAY_SIZE)) + let result = Ok(U64_ARRAY_SIZE); + ecx.return_written_byte_count_or_error(result, dest) } } @@ -215,3 +223,28 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_i32(fd_value)) } } + +/// This function either writes to the user supplied buffer and to dest place, or sets the +/// last libc error and writes -1 to dest. This is only used by eventfd. +fn return_read_bytes_and_count_ev<'tcx>( + buf_place: &MPlaceTy<'tcx>, + read_val: Option, + result: io::Result, + dest: &MPlaceTy<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, +) -> InterpResult<'tcx> { + match result.map(|c| i64::try_from(c).unwrap()) { + Ok(read_bytes) => { + // Write to the user supplied buffer. + ecx.write_int(read_val.unwrap(), buf_place)?; + // Write to the function return value place. + ecx.write_int(read_bytes, dest)?; + return Ok(()); + } + Err(e) => { + ecx.set_last_error_from_io_error(e)?; + ecx.write_int(-1, dest)?; + return Ok(()); + } + } +} diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 127aef29244..76350254e30 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -126,14 +126,18 @@ impl FileDescription for AnonSocket { &self, _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &mut [u8], + ptr: Pointer, + len: u64, + dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { - let request_byte_size = bytes.len(); + ) -> InterpResult<'tcx> { + let request_byte_size = len; + let mut bytes = vec![0; usize::try_from(len).unwrap()]; // Always succeed on read size 0. if request_byte_size == 0 { - return Ok(Ok(0)); + let result = Ok(0); + return ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest); } let Some(readbuf) = &self.readbuf else { @@ -146,7 +150,8 @@ impl FileDescription for AnonSocket { if self.peer_fd().upgrade().is_none() { // Socketpair with no peer and empty buffer. // 0 bytes successfully read indicates end-of-file. - return Ok(Ok(0)); + let result = Ok(0); + return ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest); } else { if self.is_nonblock { // Non-blocking socketpair with writer and empty buffer. @@ -154,7 +159,8 @@ impl FileDescription for AnonSocket { // EAGAIN or EWOULDBLOCK can be returned for socket, // POSIX.1-2001 allows either error to be returned for this case. // Since there is no ErrorKind for EAGAIN, WouldBlock is used. - return Ok(Err(Error::from(ErrorKind::WouldBlock))); + let result = Err(Error::from(ErrorKind::WouldBlock)); + return ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest); } else { // Blocking socketpair with writer and empty buffer. // FIXME: blocking is currently not supported @@ -170,7 +176,7 @@ impl FileDescription for AnonSocket { // Do full read / partial read based on the space available. // Conveniently, `read` exists on `VecDeque` and has exactly the desired behavior. - let actual_read_size = readbuf.buf.read(bytes).unwrap(); + let actual_read_size = readbuf.buf.read(&mut bytes).unwrap(); // Need to drop before others can access the readbuf again. drop(readbuf); @@ -186,7 +192,8 @@ impl FileDescription for AnonSocket { ecx.check_and_update_readiness(&peer_fd)?; } - return Ok(Ok(actual_read_size)); + let result = Ok(actual_read_size); + ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest) } fn write<'tcx>( @@ -194,20 +201,23 @@ impl FileDescription for AnonSocket { _self_ref: &FileDescriptionRef, _communicate_allowed: bool, bytes: &[u8], + dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result> { + ) -> InterpResult<'tcx> { let write_size = bytes.len(); // Always succeed on write size 0. // ("If count is zero and fd refers to a file other than a regular file, the results are not specified.") if write_size == 0 { - return Ok(Ok(0)); + let result = Ok(0); + return ecx.return_written_byte_count_or_error(result, dest); } // We are writing to our peer's readbuf. let Some(peer_fd) = self.peer_fd().upgrade() else { // If the upgrade from Weak to Rc fails, it indicates that all read ends have been // closed. - return Ok(Err(Error::from(ErrorKind::BrokenPipe))); + let result = Err(Error::from(ErrorKind::BrokenPipe)); + return ecx.return_written_byte_count_or_error(result, dest); }; let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf else { @@ -221,7 +231,8 @@ impl FileDescription for AnonSocket { if available_space == 0 { if self.is_nonblock { // Non-blocking socketpair with a full buffer. - return Ok(Err(Error::from(ErrorKind::WouldBlock))); + let result = Err(Error::from(ErrorKind::WouldBlock)); + return ecx.return_written_byte_count_or_error(result, dest); } else { // Blocking socketpair with a full buffer. throw_unsup_format!("socketpair write: blocking isn't supported yet"); @@ -242,7 +253,8 @@ impl FileDescription for AnonSocket { // The kernel does this even if the fd was already readable before, so we follow suit. ecx.check_and_update_readiness(&peer_fd)?; - return Ok(Ok(actual_write_size)); + let result = Ok(actual_write_size); + ecx.return_written_byte_count_or_error(result, dest) } } From 503b6af065bfc5eeb9adeda6116ea80660b57daa Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 17 Sep 2024 16:16:38 +0800 Subject: [PATCH 2/7] Use &[u8] instead of Vec and improve docs --- src/tools/miri/src/shims/unix/fd.rs | 26 ++++++++++++++----- src/tools/miri/src/shims/unix/fs.rs | 4 +-- .../miri/src/shims/unix/unnamed_socket.rs | 8 +++--- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 5f719991b37..4b87ce60c8b 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -26,7 +26,9 @@ 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. + /// `ptr` is the pointer to the user supplied read buffer. + /// `len` indicates how many bytes the user requested. + /// `dest` is where the return value should be stored. fn read<'tcx>( &self, _self_ref: &FileDescriptionRef, @@ -40,6 +42,8 @@ pub trait FileDescription: std::fmt::Debug + Any { } /// Writes as much as possible from the given buffer, and returns the number of bytes written. + /// `bytes` is the buffer of bytes supplied by the caller to be written. + /// `dest` is where the return value should be stored. fn write<'tcx>( &self, _self_ref: &FileDescriptionRef, @@ -53,6 +57,9 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Reads as much as possible into the given buffer from a given offset, /// and returns the number of bytes read. + /// `ptr` is the pointer to the user supplied read buffer. + /// `len` indicates how many bytes the user requested. + /// `dest` is where the return value should be stored. fn pread<'tcx>( &self, _communicate_allowed: bool, @@ -67,6 +74,8 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Writes as much as possible from the given buffer starting at a given offset, /// and returns the number of bytes written. + /// `bytes` is the buffer of bytes supplied by the caller to be written. + /// `dest` is where the return value should be stored. fn pwrite<'tcx>( &self, _communicate_allowed: bool, @@ -143,7 +152,7 @@ impl FileDescription for io::Stdin { helpers::isolation_abort_error("`read` from stdin")?; } let result = Read::read(&mut { self }, &mut bytes); - ecx.return_read_bytes_and_count(ptr, bytes, result, dest) + ecx.return_read_bytes_and_count(ptr, &bytes, result, dest) } fn is_tty(&self, communicate_allowed: bool) -> bool { @@ -641,12 +650,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(()) } - /// This function either writes to the user supplied buffer and to dest place, or sets the - /// last libc error and writes -1 to dest. + /// 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: Vec, + bytes: &[u8], result: io::Result, dest: &MPlaceTy<'tcx>, ) -> InterpResult<'tcx> { @@ -657,7 +669,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // 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. + // 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(()); } @@ -669,7 +681,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - /// This function writes the number of written bytes to dest place, or sets the + /// 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, diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index cf709d5e6da..e1ac9b0adfb 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -42,7 +42,7 @@ impl FileDescription for FileHandle { assert!(communicate_allowed, "isolation should have prevented even opening a file"); let mut bytes = vec![0; usize::try_from(len).unwrap()]; let result = (&mut &self.file).read(&mut bytes); - ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest) + ecx.return_read_bytes_and_count(ptr, &bytes, result, dest) } fn write<'tcx>( @@ -83,7 +83,7 @@ impl FileDescription for FileHandle { res }; let result = f(); - ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest) + ecx.return_read_bytes_and_count(ptr, &bytes, result, dest) } fn pwrite<'tcx>( diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 76350254e30..d5b061ea3cd 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -137,7 +137,7 @@ impl FileDescription for AnonSocket { // Always succeed on read size 0. if request_byte_size == 0 { let result = Ok(0); - return ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest); + return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest); } let Some(readbuf) = &self.readbuf else { @@ -151,7 +151,7 @@ impl FileDescription for AnonSocket { // Socketpair with no peer and empty buffer. // 0 bytes successfully read indicates end-of-file. let result = Ok(0); - return ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest); + return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest); } else { if self.is_nonblock { // Non-blocking socketpair with writer and empty buffer. @@ -160,7 +160,7 @@ impl FileDescription for AnonSocket { // POSIX.1-2001 allows either error to be returned for this case. // Since there is no ErrorKind for EAGAIN, WouldBlock is used. let result = Err(Error::from(ErrorKind::WouldBlock)); - return ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest); + return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest); } else { // Blocking socketpair with writer and empty buffer. // FIXME: blocking is currently not supported @@ -193,7 +193,7 @@ impl FileDescription for AnonSocket { } let result = Ok(actual_read_size); - ecx.return_read_bytes_and_count(ptr, bytes.to_vec(), result, dest) + ecx.return_read_bytes_and_count(ptr, &bytes, result, dest) } fn write<'tcx>( From d29be1f90a3dd6f7964151ffc05b0c95088104ab Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 17 Sep 2024 17:40:20 +0800 Subject: [PATCH 3/7] Pass pointer and len to FileDescription::write and change the type of len in read to usize --- src/tools/miri/src/shims/unix/fd.rs | 46 +++++++++++-------- src/tools/miri/src/shims/unix/fs.rs | 20 ++++---- .../miri/src/shims/unix/linux/eventfd.rs | 23 +++++----- .../miri/src/shims/unix/unnamed_socket.rs | 18 ++++---- 4 files changed, 60 insertions(+), 47 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 4b87ce60c8b..f032eeab468 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -34,7 +34,7 @@ pub trait FileDescription: std::fmt::Debug + Any { _self_ref: &FileDescriptionRef, _communicate_allowed: bool, _ptr: Pointer, - _len: u64, + _len: usize, _dest: &MPlaceTy<'tcx>, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { @@ -42,13 +42,15 @@ pub trait FileDescription: std::fmt::Debug + Any { } /// Writes as much as possible from the given buffer, and returns the number of bytes written. - /// `bytes` is the buffer of bytes supplied by the caller to be written. + /// `ptr` is the pointer to the user supplied read buffer. + /// `len` indicates how many bytes the user requested. /// `dest` is where the return value should be stored. 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> { @@ -65,7 +67,7 @@ pub trait FileDescription: std::fmt::Debug + Any { _communicate_allowed: bool, _offset: u64, _ptr: Pointer, - _len: u64, + _len: usize, _dest: &MPlaceTy<'tcx>, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { @@ -74,12 +76,14 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Writes as much as possible from the given buffer starting at a given offset, /// and returns the number of bytes written. - /// `bytes` is the buffer of bytes supplied by the caller to be written. + /// `ptr` is the pointer to the user supplied read buffer. + /// `len` indicates how many bytes the user requested. /// `dest` is where the return value should be stored. fn pwrite<'tcx>( &self, _communicate_allowed: bool, - _bytes: &[u8], + _ptr: Pointer, + _len: usize, _offset: u64, _dest: &MPlaceTy<'tcx>, _ecx: &mut MiriInterpCx<'tcx>, @@ -142,11 +146,11 @@ impl FileDescription for io::Stdin { _self_ref: &FileDescriptionRef, communicate_allowed: bool, ptr: Pointer, - len: u64, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let mut bytes = vec![0; usize::try_from(len).unwrap()]; + 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")?; @@ -169,12 +173,14 @@ impl FileDescription for io::Stdout { &self, _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &[u8], + 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))?.to_owned(); // We allow writing to stderr even with isolation enabled. - let result = Write::write(&mut { self }, bytes); + let result = Write::write(&mut { self }, &bytes); // Stdout is buffered, flush to make sure it appears on the // screen. This is the write() syscall of the interpreted // program, we want it to correspond to a write() syscall on @@ -198,13 +204,15 @@ impl FileDescription for io::Stderr { &self, _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &[u8], + 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))?.to_owned(); // We allow writing to stderr even with isolation enabled. // No need to flush, stderr is not buffered. - let result = Write::write(&mut { self }, bytes); + let result = Write::write(&mut { self }, &bytes); ecx.return_written_byte_count_or_error(result, dest) } @@ -226,12 +234,13 @@ impl FileDescription for NullOutput { &self, _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &[u8], + _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. - let result = Ok(bytes.len()); + let result = Ok(len); ecx.return_written_byte_count_or_error(result, dest) } } @@ -591,7 +600,7 @@ 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 => fd.read(&fd, communicate, buf, usize::try_from(count).unwrap(), dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { let einval = this.eval_libc("EINVAL"); @@ -599,7 +608,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_int(-1, dest)?; return Ok(()); }; - fd.pread(communicate, offset, buf, count, dest, this)? + fd.pread(communicate, offset, buf, usize::try_from(count).unwrap(), dest, this)? } }; Ok(()) @@ -627,7 +636,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { .min(u64::try_from(isize::MAX).unwrap()); 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 { let res: i32 = this.fd_not_found()?; @@ -636,7 +644,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; match offset { - None => fd.write(&fd, communicate, &bytes, dest, this)?, + None => fd.write(&fd, communicate, buf, usize::try_from(count).unwrap(), dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { let einval = this.eval_libc("EINVAL"); @@ -644,7 +652,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_int(-1, dest)?; return Ok(()); }; - fd.pwrite(communicate, &bytes, offset, dest, this)? + fd.pwrite(communicate, buf, usize::try_from(count).unwrap(), offset, dest, this)? } }; Ok(()) diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index e1ac9b0adfb..0e80a45f48d 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -35,12 +35,12 @@ impl FileDescription for FileHandle { _self_ref: &FileDescriptionRef, communicate_allowed: bool, ptr: Pointer, - len: u64, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - let mut bytes = vec![0; usize::try_from(len).unwrap()]; + let mut bytes = vec![0; len]; let result = (&mut &self.file).read(&mut bytes); ecx.return_read_bytes_and_count(ptr, &bytes, result, dest) } @@ -49,12 +49,14 @@ impl FileDescription for FileHandle { &self, _self_ref: &FileDescriptionRef, communicate_allowed: bool, - bytes: &[u8], + ptr: Pointer, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - let result = (&mut &self.file).write(bytes); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); + let result = (&mut &self.file).write(&bytes); ecx.return_written_byte_count_or_error(result, dest) } @@ -63,12 +65,12 @@ impl FileDescription for FileHandle { communicate_allowed: bool, offset: u64, ptr: Pointer, - len: u64, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - let mut bytes = vec![0; usize::try_from(len).unwrap()]; + let mut bytes = vec![0; len]; // Emulates pread using seek + read + seek to restore cursor position. // Correctness of this emulation relies on sequential nature of Miri execution. // The closure is used to emulate `try` block, since we "bubble" `io::Error` using `?`. @@ -89,7 +91,8 @@ impl FileDescription for FileHandle { fn pwrite<'tcx>( &self, communicate_allowed: bool, - bytes: &[u8], + ptr: Pointer, + len: usize, offset: u64, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, @@ -99,10 +102,11 @@ impl FileDescription for FileHandle { // Correctness of this emulation relies on sequential nature of Miri execution. // The closure is used to emulate `try` block, since we "bubble" `io::Error` using `?`. let file = &mut &self.file; + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); let mut f = || { let cursor_pos = file.stream_position()?; file.seek(SeekFrom::Start(offset))?; - let res = file.write(bytes); + let res = file.write(&bytes); // Attempt to restore cursor position even if the write has failed file.seek(SeekFrom::Start(cursor_pos)) .expect("failed to restore file position, this shouldn't be possible"); diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index 2375533fd17..e1531fc2fa8 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -4,8 +4,6 @@ use std::io; use std::io::{Error, ErrorKind}; use std::mem; -use rustc_target::abi::Endian; - use crate::shims::unix::fd::FileDescriptionRef; use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; @@ -63,14 +61,14 @@ impl FileDescription for Event { self_ref: &FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, - len: u64, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { // eventfd read at the size of u64. let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ecx.machine.layouts.u64); // Check the size of slice, and return error only if the size of the slice < 8. - if len < U64_ARRAY_SIZE.try_into().unwrap() { + if len < U64_ARRAY_SIZE { let result = Err(Error::from(ErrorKind::InvalidInput)); return return_read_bytes_and_count_ev(&buf_place, None, result, dest, ecx); } @@ -114,20 +112,21 @@ impl FileDescription for Event { &self, self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &[u8], + ptr: Pointer, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { // Check the size of slice, and return error only if the size of the slice < 8. - let Some(bytes) = bytes.first_chunk::() else { + if len < U64_ARRAY_SIZE { let result = Err(Error::from(ErrorKind::InvalidInput)); return ecx.return_written_byte_count_or_error(result, dest); - }; - // Convert from bytes to int according to host endianness. - let num = match ecx.tcx.sess.target.endian { - Endian::Little => u64::from_le_bytes(*bytes), - Endian::Big => u64::from_be_bytes(*bytes), - }; + } + + // Read the user supplied value from the pointer. + let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ecx.machine.layouts.u64); + let num = ecx.read_scalar(&buf_place)?.to_u64()?; + // u64::MAX as input is invalid because the maximum value of counter is u64::MAX - 1. if num == u64::MAX { let result = Err(Error::from(ErrorKind::InvalidInput)); diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index d5b061ea3cd..2cb9bb9b2dc 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -7,6 +7,8 @@ use std::collections::VecDeque; use std::io; use std::io::{Error, ErrorKind, Read}; +use rustc_target::abi::Size; + use crate::shims::unix::fd::{FileDescriptionRef, WeakFileDescriptionRef}; use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; @@ -127,15 +129,14 @@ impl FileDescription for AnonSocket { _self_ref: &FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, - len: u64, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let request_byte_size = len; - let mut bytes = vec![0; usize::try_from(len).unwrap()]; + let mut bytes = vec![0; len]; // Always succeed on read size 0. - if request_byte_size == 0 { + if len == 0 { let result = Ok(0); return ecx.return_read_bytes_and_count(ptr, &bytes, result, dest); } @@ -200,14 +201,14 @@ impl FileDescription for AnonSocket { &self, _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - bytes: &[u8], + ptr: Pointer, + len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let write_size = bytes.len(); // Always succeed on write size 0. // ("If count is zero and fd refers to a file other than a regular file, the results are not specified.") - if write_size == 0 { + if len == 0 { let result = Ok(0); return ecx.return_written_byte_count_or_error(result, dest); } @@ -243,7 +244,8 @@ impl FileDescription for AnonSocket { writebuf.clock.join(clock); } // Do full write / partial write based on the space available. - let actual_write_size = write_size.min(available_space); + let actual_write_size = len.min(available_space); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); writebuf.buf.extend(&bytes[..actual_write_size]); // Need to stop accessing peer_fd so that it can be notified. From fb1193078da1c3d8f41d744d8979e4ea76a8d120 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Sep 2024 16:22:57 +0200 Subject: [PATCH 4/7] further tweak FileDescription comments --- src/tools/miri/src/shims/unix/fd.rs | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index f032eeab468..c0f179ccdce 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -25,10 +25,9 @@ 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. - /// `ptr` is the pointer to the user supplied read buffer. - /// `len` indicates how many bytes the user requested. - /// `dest` is where the return value should be stored. + /// 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, @@ -41,10 +40,9 @@ pub trait FileDescription: std::fmt::Debug + Any { throw_unsup_format!("cannot read from {}", self.name()); } - /// Writes as much as possible from the given buffer, and returns the number of bytes written. - /// `ptr` is the pointer to the user supplied read buffer. - /// `len` indicates how many bytes the user requested. - /// `dest` is where the return value should be stored. + /// 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, @@ -57,11 +55,9 @@ pub trait FileDescription: std::fmt::Debug + Any { 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. - /// `ptr` is the pointer to the user supplied read buffer. - /// `len` indicates how many bytes the user requested. - /// `dest` is where the return value should be stored. + /// 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, @@ -74,11 +70,10 @@ pub trait FileDescription: std::fmt::Debug + Any { 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 the user requested. - /// `dest` is where the return value should be stored. + /// `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, From 5dee646aea06ec06d35c0fbd13489e83996be0b5 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Sep 2024 16:37:00 +0200 Subject: [PATCH 5/7] read, write: move cast-to-usize logic up and deduplicate it --- src/tools/miri/src/shims/unix/fd.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index c0f179ccdce..1ab6f921cfb 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -579,6 +579,7 @@ 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`. @@ -595,7 +596,7 @@ 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, usize::try_from(count).unwrap(), dest, this)?, + 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"); @@ -603,7 +604,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_int(-1, dest)?; return Ok(()); }; - fd.pread(communicate, offset, buf, usize::try_from(count).unwrap(), dest, this)? + fd.pread(communicate, offset, buf, count, dest, this)? } }; Ok(()) @@ -629,6 +630,7 @@ 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`. @@ -639,7 +641,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; match offset { - None => fd.write(&fd, communicate, buf, usize::try_from(count).unwrap(), dest, this)?, + 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"); @@ -647,7 +649,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { this.write_int(-1, dest)?; return Ok(()); }; - fd.pwrite(communicate, buf, usize::try_from(count).unwrap(), offset, dest, this)? + fd.pwrite(communicate, buf, count, offset, dest, this)? } }; Ok(()) From ed24426824170a7dca7edb3fd3fb4a2aead867ab Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Sep 2024 16:39:02 +0200 Subject: [PATCH 6/7] remove some unnecessary to_owned --- src/tools/miri/src/shims/unix/fd.rs | 8 ++++---- src/tools/miri/src/shims/unix/fs.rs | 8 ++++---- src/tools/miri/src/shims/unix/unnamed_socket.rs | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 1ab6f921cfb..6b78ce7ad47 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -173,9 +173,9 @@ impl FileDescription for io::Stdout { dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); + 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); + let result = Write::write(&mut { self }, bytes); // Stdout is buffered, flush to make sure it appears on the // screen. This is the write() syscall of the interpreted // program, we want it to correspond to a write() syscall on @@ -204,10 +204,10 @@ impl FileDescription for io::Stderr { dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); + 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); + let result = Write::write(&mut { self }, bytes); ecx.return_written_byte_count_or_error(result, dest) } diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 0e80a45f48d..a5cf2ab6ebb 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -55,8 +55,8 @@ impl FileDescription for FileHandle { ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); - let result = (&mut &self.file).write(&bytes); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; + let result = (&mut &self.file).write(bytes); ecx.return_written_byte_count_or_error(result, dest) } @@ -102,11 +102,11 @@ impl FileDescription for FileHandle { // Correctness of this emulation relies on sequential nature of Miri execution. // The closure is used to emulate `try` block, since we "bubble" `io::Error` using `?`. let file = &mut &self.file; - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; let mut f = || { let cursor_pos = file.stream_position()?; file.seek(SeekFrom::Start(offset))?; - let res = file.write(&bytes); + let res = file.write(bytes); // Attempt to restore cursor position even if the write has failed file.seek(SeekFrom::Start(cursor_pos)) .expect("failed to restore file position, this shouldn't be possible"); diff --git a/src/tools/miri/src/shims/unix/unnamed_socket.rs b/src/tools/miri/src/shims/unix/unnamed_socket.rs index 2cb9bb9b2dc..db6872319ea 100644 --- a/src/tools/miri/src/shims/unix/unnamed_socket.rs +++ b/src/tools/miri/src/shims/unix/unnamed_socket.rs @@ -245,7 +245,7 @@ impl FileDescription for AnonSocket { } // Do full write / partial write based on the space available. let actual_write_size = len.min(available_space); - let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?.to_owned(); + let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; writebuf.buf.extend(&bytes[..actual_write_size]); // Need to stop accessing peer_fd so that it can be notified. From d70fd882ef813190bf909de421358eec629bd088 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Sep 2024 16:49:21 +0200 Subject: [PATCH 7/7] simplify eventfd handling a bit --- .../miri/src/shims/unix/linux/eventfd.rs | 79 +++++++------------ 1 file changed, 30 insertions(+), 49 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index e1531fc2fa8..d1d461daa99 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -2,16 +2,12 @@ use std::cell::{Cell, RefCell}; use std::io; use std::io::{Error, ErrorKind}; -use std::mem; use crate::shims::unix::fd::FileDescriptionRef; use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; use crate::{concurrency::VClock, *}; -// We'll only do reads and writes in chunks of size u64. -const U64_ARRAY_SIZE: usize = mem::size_of::(); - /// Maximum value that the eventfd counter can hold. const MAX_COUNTER: u64 = u64::MAX - 1; @@ -65,35 +61,45 @@ impl FileDescription for Event { dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - // eventfd read at the size of u64. - let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ecx.machine.layouts.u64); + // 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 < U64_ARRAY_SIZE { - let result = Err(Error::from(ErrorKind::InvalidInput)); - return return_read_bytes_and_count_ev(&buf_place, None, result, dest, ecx); + if len < ty.size.bytes_usize() { + ecx.set_last_error_from_io_error(Error::from(ErrorKind::InvalidInput))?; + ecx.write_int(-1, dest)?; + return Ok(()); } + // eventfd read at the size of u64. + let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ty); + // Block when counter == 0. let counter = self.counter.get(); if counter == 0 { if self.is_nonblock { - let result = Err(Error::from(ErrorKind::WouldBlock)); - return return_read_bytes_and_count_ev(&buf_place, None, result, dest, ecx); - } else { - //FIXME: blocking is not supported - throw_unsup_format!("eventfd: blocking is unsupported"); + ecx.set_last_error_from_io_error(Error::from(ErrorKind::WouldBlock))?; + ecx.write_int(-1, dest)?; + return Ok(()); } + + throw_unsup_format!("eventfd: blocking is unsupported"); } else { // Synchronize with all prior `write` calls to this FD. ecx.acquire_clock(&self.clock.borrow()); - let result = Ok(U64_ARRAY_SIZE); + + // Give old counter value to userspace, and set counter value to 0. + ecx.write_int(counter, &buf_place)?; self.counter.set(0); + // When any of the event happened, we check and update the status of all supported event // types for current file description. ecx.check_and_update_readiness(self_ref)?; - return_read_bytes_and_count_ev(&buf_place, Some(counter), result, dest, ecx) + // Tell userspace how many bytes we wrote. + ecx.write_int(buf_place.layout.size.bytes(), dest)?; } + + Ok(()) } /// A write call adds the 8-byte integer value supplied in @@ -117,14 +123,16 @@ impl FileDescription for Event { dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { + // 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 < U64_ARRAY_SIZE { + if len < ty.layout.size.bytes_usize() { let result = Err(Error::from(ErrorKind::InvalidInput)); return ecx.return_written_byte_count_or_error(result, dest); } // Read the user supplied value from the pointer. - let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ecx.machine.layouts.u64); + let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ty); let num = ecx.read_scalar(&buf_place)?.to_u64()?; // u64::MAX as input is invalid because the maximum value of counter is u64::MAX - 1. @@ -142,22 +150,20 @@ impl FileDescription for Event { } self.counter.set(new_count); } - None | Some(u64::MAX) => { + None | Some(u64::MAX) => if self.is_nonblock { let result = Err(Error::from(ErrorKind::WouldBlock)); return ecx.return_written_byte_count_or_error(result, dest); } else { - //FIXME: blocking is not supported throw_unsup_format!("eventfd: blocking is unsupported"); - } - } + }, }; // When any of the event happened, we check and update the status of all supported event // types for current file description. ecx.check_and_update_readiness(self_ref)?; - let result = Ok(U64_ARRAY_SIZE); - ecx.return_written_byte_count_or_error(result, dest) + // Return how many bytes we read. + ecx.write_int(buf_place.layout.size.bytes(), dest) } } @@ -222,28 +228,3 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_i32(fd_value)) } } - -/// This function either writes to the user supplied buffer and to dest place, or sets the -/// last libc error and writes -1 to dest. This is only used by eventfd. -fn return_read_bytes_and_count_ev<'tcx>( - buf_place: &MPlaceTy<'tcx>, - read_val: Option, - result: io::Result, - dest: &MPlaceTy<'tcx>, - ecx: &mut MiriInterpCx<'tcx>, -) -> InterpResult<'tcx> { - match result.map(|c| i64::try_from(c).unwrap()) { - Ok(read_bytes) => { - // Write to the user supplied buffer. - ecx.write_int(read_val.unwrap(), buf_place)?; - // Write to the function return value place. - ecx.write_int(read_bytes, dest)?; - return Ok(()); - } - Err(e) => { - ecx.set_last_error_from_io_error(e)?; - ecx.write_int(-1, dest)?; - return Ok(()); - } - } -}