From 17cfbc6fa37b7b82a4b4f2fb1637082fec2967ad Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 12:03:42 +0200 Subject: [PATCH 1/7] FD: remove big surrounding RefCell, simplify socketpair --- src/tools/miri/src/shims/unix/fd.rs | 101 ++++++------- src/tools/miri/src/shims/unix/fs.rs | 48 +++--- src/tools/miri/src/shims/unix/linux/epoll.rs | 24 ++- .../miri/src/shims/unix/linux/eventfd.rs | 37 +++-- src/tools/miri/src/shims/unix/socket.rs | 142 ++++++++---------- .../miri/tests/pass-dep/libc/libc-epoll.rs | 4 + 6 files changed, 169 insertions(+), 187 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 98a124b9a5620..1fd1cf4d99e0e 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -2,9 +2,9 @@ //! standard file descriptors (stdin/stdout/stderr). use std::any::Any; -use std::cell::{Ref, RefCell, RefMut}; use std::collections::BTreeMap; use std::io::{self, ErrorKind, IsTerminal, Read, SeekFrom, Write}; +use std::ops::Deref; use std::rc::Rc; use std::rc::Weak; @@ -27,7 +27,7 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Reads as much as possible into the given buffer, and returns the number of bytes read. fn read<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, _bytes: &mut [u8], @@ -38,7 +38,7 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Writes as much as possible from the given buffer, and returns the number of bytes written. fn write<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, _bytes: &[u8], @@ -50,7 +50,7 @@ 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. fn pread<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _bytes: &mut [u8], _offset: u64, @@ -62,7 +62,7 @@ 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. fn pwrite<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _bytes: &[u8], _offset: u64, @@ -74,7 +74,7 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Seeks to the given offset (which can be relative to the beginning, end, or current position). /// Returns the new position from the start of the stream. fn seek<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _offset: SeekFrom, ) -> InterpResult<'tcx, io::Result> { @@ -111,14 +111,9 @@ pub trait FileDescription: std::fmt::Debug + Any { impl dyn FileDescription { #[inline(always)] - pub fn downcast_ref(&self) -> Option<&T> { + pub fn downcast(&self) -> Option<&T> { (self as &dyn Any).downcast_ref() } - - #[inline(always)] - pub fn downcast_mut(&mut self) -> Option<&mut T> { - (self as &mut dyn Any).downcast_mut() - } } impl FileDescription for io::Stdin { @@ -127,7 +122,7 @@ impl FileDescription for io::Stdin { } fn read<'tcx>( - &mut self, + &self, communicate_allowed: bool, _fd_id: FdId, bytes: &mut [u8], @@ -137,7 +132,7 @@ impl FileDescription for io::Stdin { // 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(self, bytes)) + Ok(Read::read(&mut { self }, bytes)) } fn is_tty(&self, communicate_allowed: bool) -> bool { @@ -151,14 +146,14 @@ impl FileDescription for io::Stdout { } fn write<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { // We allow writing to stderr even with isolation enabled. - let result = Write::write(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 @@ -180,7 +175,7 @@ impl FileDescription for io::Stderr { } fn write<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, bytes: &[u8], @@ -206,7 +201,7 @@ impl FileDescription for NullOutput { } fn write<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, bytes: &[u8], @@ -221,26 +216,23 @@ impl FileDescription for NullOutput { #[derive(Clone, Debug)] pub struct FileDescWithId { id: FdId, - file_description: RefCell>, + file_description: Box, } #[derive(Clone, Debug)] pub struct FileDescriptionRef(Rc>); -impl FileDescriptionRef { - fn new(fd: impl FileDescription, id: FdId) -> Self { - FileDescriptionRef(Rc::new(FileDescWithId { - id, - file_description: RefCell::new(Box::new(fd)), - })) - } +impl Deref for FileDescriptionRef { + type Target = dyn FileDescription; - pub fn borrow(&self) -> Ref<'_, dyn FileDescription> { - Ref::map(self.0.file_description.borrow(), |fd| fd.as_ref()) + fn deref(&self) -> &Self::Target { + &*self.0.file_description } +} - pub fn borrow_mut(&self) -> RefMut<'_, dyn FileDescription> { - RefMut::map(self.0.file_description.borrow_mut(), |fd| fd.as_mut()) +impl FileDescriptionRef { + fn new(fd: impl FileDescription, id: FdId) -> Self { + FileDescriptionRef(Rc::new(FileDescWithId { id, file_description: Box::new(fd) })) } pub fn close<'tcx>( @@ -256,7 +248,7 @@ impl FileDescriptionRef { // Remove entry from the global epoll_event_interest table. ecx.machine.epoll_interests.remove(id); - RefCell::into_inner(fd.file_description).close(communicate_allowed, ecx) + fd.file_description.close(communicate_allowed, ecx) } None => Ok(Ok(())), } @@ -277,7 +269,7 @@ impl FileDescriptionRef { ecx: &mut InterpCx<'tcx, MiriMachine<'tcx>>, ) -> InterpResult<'tcx, ()> { use crate::shims::unix::linux::epoll::EvalContextExt; - ecx.check_and_update_readiness(self.get_id(), || self.borrow_mut().get_epoll_ready_events()) + ecx.check_and_update_readiness(self.get_id(), || self.get_epoll_ready_events()) } } @@ -334,11 +326,20 @@ impl FdTable { fds } - /// Insert a new file description to the FdTable. - pub fn insert_new(&mut self, fd: impl FileDescription) -> i32 { + pub fn new_ref(&mut self, fd: impl FileDescription) -> FileDescriptionRef { let file_handle = FileDescriptionRef::new(fd, self.next_file_description_id); self.next_file_description_id = FdId(self.next_file_description_id.0.strict_add(1)); - self.insert_ref_with_min_fd(file_handle, 0) + file_handle + } + + /// Insert a new file description to the FdTable. + pub fn insert_new(&mut self, fd: impl FileDescription) -> i32 { + let fd_ref = self.new_ref(fd); + self.insert(fd_ref) + } + + pub fn insert(&mut self, fd_ref: FileDescriptionRef) -> i32 { + self.insert_ref_with_min_fd(fd_ref, 0) } /// Insert a file description, giving it a file descriptor that is at least `min_fd`. @@ -368,17 +369,7 @@ impl FdTable { new_fd } - pub fn get(&self, fd: i32) -> Option> { - let fd = self.fds.get(&fd)?; - Some(fd.borrow()) - } - - pub fn get_mut(&self, fd: i32) -> Option> { - let fd = self.fds.get(&fd)?; - Some(fd.borrow_mut()) - } - - pub fn get_ref(&self, fd: i32) -> Option { + pub fn get(&self, fd: i32) -> Option { let fd = self.fds.get(&fd)?; Some(fd.clone()) } @@ -397,7 +388,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn dup(&mut self, old_fd: i32) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let Some(dup_fd) = this.machine.fds.get_ref(old_fd) else { + let Some(dup_fd) = this.machine.fds.get(old_fd) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); }; Ok(Scalar::from_i32(this.machine.fds.insert_ref_with_min_fd(dup_fd, 0))) @@ -406,7 +397,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn dup2(&mut self, old_fd: i32, new_fd: i32) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - let Some(dup_fd) = this.machine.fds.get_ref(old_fd) else { + let Some(dup_fd) = this.machine.fds.get(old_fd) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); }; if new_fd != old_fd { @@ -492,7 +483,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } let start = this.read_scalar(&args[2])?.to_i32()?; - match this.machine.fds.get_ref(fd) { + match this.machine.fds.get(fd) { Some(dup_fd) => Ok(Scalar::from_i32(this.machine.fds.insert_ref_with_min_fd(dup_fd, start))), None => Ok(Scalar::from_i32(this.fd_not_found()?)), @@ -565,7 +556,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { 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_ref(fd) else { + let Some(fd) = this.machine.fds.get(fd) else { trace!("read: FD not found"); return Ok(Scalar::from_target_isize(this.fd_not_found()?, this)); }; @@ -576,14 +567,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // `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.borrow_mut().read(communicate, fd.get_id(), &mut bytes, this), + None => fd.read(communicate, fd.get_id(), &mut bytes, 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)); }; - fd.borrow_mut().pread(communicate, &mut bytes, offset, this) + fd.pread(communicate, &mut bytes, offset, this) } }; @@ -629,19 +620,19 @@ 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_ref(fd) else { + let Some(fd) = this.machine.fds.get(fd) else { return Ok(Scalar::from_target_isize(this.fd_not_found()?, this)); }; let result = match offset { - None => fd.borrow_mut().write(communicate, fd.get_id(), &bytes, this), + None => fd.write(communicate, fd.get_id(), &bytes, 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)); }; - fd.borrow_mut().pwrite(communicate, &bytes, offset, this) + fd.pwrite(communicate, &bytes, offset, this) } }; diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 9da36e64a0f99..e6076abedbd49 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -31,29 +31,29 @@ impl FileDescription for FileHandle { } fn read<'tcx>( - &mut self, + &self, communicate_allowed: bool, _fd_id: FdId, bytes: &mut [u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - Ok(self.file.read(bytes)) + Ok((&mut &self.file).read(bytes)) } fn write<'tcx>( - &mut self, + &self, communicate_allowed: bool, _fd_id: FdId, bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - Ok(self.file.write(bytes)) + Ok((&mut &self.file).write(bytes)) } fn pread<'tcx>( - &mut self, + &self, communicate_allowed: bool, bytes: &mut [u8], offset: u64, @@ -63,13 +63,13 @@ impl FileDescription for FileHandle { // 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 `?`. + let file = &mut &self.file; let mut f = || { - let cursor_pos = self.file.stream_position()?; - self.file.seek(SeekFrom::Start(offset))?; - let res = self.file.read(bytes); + let cursor_pos = file.stream_position()?; + file.seek(SeekFrom::Start(offset))?; + let res = file.read(bytes); // Attempt to restore cursor position even if the read has failed - self.file - .seek(SeekFrom::Start(cursor_pos)) + file.seek(SeekFrom::Start(cursor_pos)) .expect("failed to restore file position, this shouldn't be possible"); res }; @@ -77,7 +77,7 @@ impl FileDescription for FileHandle { } fn pwrite<'tcx>( - &mut self, + &self, communicate_allowed: bool, bytes: &[u8], offset: u64, @@ -87,13 +87,13 @@ impl FileDescription for FileHandle { // Emulates pwrite using seek + write + 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 `?`. + let file = &mut &self.file; let mut f = || { - let cursor_pos = self.file.stream_position()?; - self.file.seek(SeekFrom::Start(offset))?; - let res = self.file.write(bytes); + let cursor_pos = file.stream_position()?; + file.seek(SeekFrom::Start(offset))?; + let res = file.write(bytes); // Attempt to restore cursor position even if the write has failed - self.file - .seek(SeekFrom::Start(cursor_pos)) + file.seek(SeekFrom::Start(cursor_pos)) .expect("failed to restore file position, this shouldn't be possible"); res }; @@ -101,12 +101,12 @@ impl FileDescription for FileHandle { } fn seek<'tcx>( - &mut self, + &self, communicate_allowed: bool, offset: SeekFrom, ) -> InterpResult<'tcx, io::Result> { assert!(communicate_allowed, "isolation should have prevented even opening a file"); - Ok(self.file.seek(offset)) + Ok((&mut &self.file).seek(offset)) } fn close<'tcx>( @@ -580,7 +580,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let communicate = this.machine.communicate(); - let Some(mut file_description) = this.machine.fds.get_mut(fd) else { + let Some(file_description) = this.machine.fds.get(fd) else { return Ok(Scalar::from_i64(this.fd_not_found()?)); }; let result = file_description @@ -1276,7 +1276,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // FIXME: Support ftruncate64 for all FDs let FileHandle { file, writable } = - file_description.downcast_ref::().ok_or_else(|| { + file_description.downcast::().ok_or_else(|| { err_unsup_format!("`ftruncate64` is only supported on file-backed file descriptors") })?; @@ -1328,7 +1328,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // Only regular files support synchronization. let FileHandle { file, writable } = - file_description.downcast_ref::().ok_or_else(|| { + file_description.downcast::().ok_or_else(|| { err_unsup_format!("`fsync` is only supported on file-backed file descriptors") })?; let io_result = maybe_sync_file(file, *writable, File::sync_all); @@ -1353,7 +1353,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // Only regular files support synchronization. let FileHandle { file, writable } = - file_description.downcast_ref::().ok_or_else(|| { + file_description.downcast::().ok_or_else(|| { err_unsup_format!("`fdatasync` is only supported on file-backed file descriptors") })?; let io_result = maybe_sync_file(file, *writable, File::sync_data); @@ -1401,7 +1401,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // Only regular files support synchronization. let FileHandle { file, writable } = - file_description.downcast_ref::().ok_or_else(|| { + file_description.downcast::().ok_or_else(|| { err_unsup_format!( "`sync_data_range` is only supported on file-backed file descriptors" ) @@ -1708,7 +1708,7 @@ impl FileMetadata { }; let file = &file_description - .downcast_ref::() + .downcast::() .ok_or_else(|| { err_unsup_format!( "obtaining metadata is only supported on file-backed file descriptors" diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 89616bd0d0723..a1b943f2e82a1 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -12,7 +12,7 @@ use crate::*; struct Epoll { /// A map of EpollEventInterests registered under this epoll instance. /// Each entry is differentiated using FdId and file descriptor value. - interest_list: BTreeMap<(FdId, i32), Rc>>, + interest_list: RefCell>>>, /// A map of EpollEventInstance that will be returned when `epoll_wait` is called. /// Similar to interest_list, the entry is also differentiated using FdId /// and file descriptor value. @@ -226,18 +226,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } // Check if epfd is a valid epoll file descriptor. - let Some(epfd) = this.machine.fds.get_ref(epfd_value) else { + let Some(epfd) = this.machine.fds.get(epfd_value) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); }; - let mut binding = epfd.borrow_mut(); - let epoll_file_description = &mut binding - .downcast_mut::() + let epoll_file_description = epfd + .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_ctl`"))?; - let interest_list = &mut epoll_file_description.interest_list; + let mut interest_list = epoll_file_description.interest_list.borrow_mut(); let ready_list = &epoll_file_description.ready_list; - let Some(file_descriptor) = this.machine.fds.get_ref(fd) else { + let Some(file_descriptor) = this.machine.fds.get(fd) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); }; let id = file_descriptor.get_id(); @@ -399,16 +398,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("epoll_wait: timeout value can only be 0"); } - let Some(epfd) = this.machine.fds.get_ref(epfd) else { + let Some(epfd) = this.machine.fds.get(epfd) else { return Ok(Scalar::from_i32(this.fd_not_found()?)); }; - let mut binding = epfd.borrow_mut(); - let epoll_file_description = &mut binding - .downcast_mut::() + let epoll_file_description = epfd + .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; - let binding = epoll_file_description.get_ready_list(); - let mut ready_list = binding.borrow_mut(); + let ready_list = epoll_file_description.get_ready_list(); + let mut ready_list = ready_list.borrow_mut(); let mut num_of_events: i32 = 0; let mut array_iter = this.project_array_fields(&event)?; diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index 8a11f225b2248..cede3967bc846 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -1,4 +1,5 @@ //! Linux `eventfd` implementation. +use std::cell::{Cell, RefCell}; use std::io; use std::io::{Error, ErrorKind}; use std::mem; @@ -27,9 +28,9 @@ const MAX_COUNTER: u64 = u64::MAX - 1; struct Event { /// The object contains an unsigned 64-bit integer (uint64_t) counter that is maintained by the /// kernel. This counter is initialized with the value specified in the argument initval. - counter: u64, + counter: Cell, is_nonblock: bool, - clock: VClock, + clock: RefCell, } impl FileDescription for Event { @@ -42,8 +43,8 @@ impl FileDescription for Event { // need to be supported in the future, the check should be added here. Ok(EpollReadyEvents { - epollin: self.counter != 0, - epollout: self.counter != MAX_COUNTER, + epollin: self.counter.get() != 0, + epollout: self.counter.get() != MAX_COUNTER, ..EpollReadyEvents::new() }) } @@ -58,7 +59,7 @@ impl FileDescription for Event { /// Read the counter in the buffer and return the counter if succeeded. fn read<'tcx>( - &mut self, + &self, _communicate_allowed: bool, fd_id: FdId, bytes: &mut [u8], @@ -69,7 +70,8 @@ impl FileDescription for Event { return Ok(Err(Error::from(ErrorKind::InvalidInput))); }; // Block when counter == 0. - if self.counter == 0 { + let counter = self.counter.get(); + if counter == 0 { if self.is_nonblock { return Ok(Err(Error::from(ErrorKind::WouldBlock))); } else { @@ -78,13 +80,13 @@ impl FileDescription for Event { } } else { // Synchronize with all prior `write` calls to this FD. - ecx.acquire_clock(&self.clock); + 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 => self.counter.to_le_bytes(), - Endian::Big => self.counter.to_be_bytes(), + Endian::Little => counter.to_le_bytes(), + Endian::Big => counter.to_be_bytes(), }; - self.counter = 0; + 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. @@ -114,7 +116,7 @@ impl FileDescription for Event { /// supplied buffer is less than 8 bytes, or if an attempt is /// made to write the value 0xffffffffffffffff. fn write<'tcx>( - &mut self, + &self, _communicate_allowed: bool, fd_id: FdId, bytes: &[u8], @@ -135,13 +137,13 @@ impl FileDescription for Event { } // If the addition does not let the counter to exceed the maximum value, update the counter. // Else, block. - match self.counter.checked_add(num) { + match self.counter.get().checked_add(num) { Some(new_count @ 0..=MAX_COUNTER) => { // Future `read` calls will synchronize with this write, so update the FD clock. if let Some(clock) = &ecx.release_clock() { - self.clock.join(clock); + self.clock.borrow_mut().join(clock); } - self.counter = new_count; + self.counter.set(new_count); } None | Some(u64::MAX) => { if self.is_nonblock { @@ -219,8 +221,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let fds = &mut this.machine.fds; - let fd_value = - fds.insert_new(Event { counter: val.into(), is_nonblock, clock: VClock::default() }); + let fd_value = fds.insert_new(Event { + counter: Cell::new(val.into()), + is_nonblock, + clock: RefCell::new(VClock::default()), + }); Ok(Scalar::from_i32(fd_value)) } diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index 0f40d9776bbb8..2694391dfbb34 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -1,8 +1,7 @@ -use std::cell::RefCell; +use std::cell::{OnceCell, RefCell}; use std::collections::VecDeque; use std::io; use std::io::{Error, ErrorKind, Read}; -use std::rc::{Rc, Weak}; use crate::shims::unix::fd::{FdId, WeakFileDescriptionRef}; use crate::shims::unix::linux::epoll::EpollReadyEvents; @@ -17,15 +16,12 @@ const MAX_SOCKETPAIR_BUFFER_CAPACITY: usize = 212992; /// Pair of connected sockets. #[derive(Debug)] struct SocketPair { - // By making the write link weak, a `write` can detect when all readers are - // gone, and trigger EPIPE as appropriate. - writebuf: Weak>, - readbuf: Rc>, - /// When a socketpair instance is created, two socketpair file descriptions are generated. - /// The peer_fd field holds a weak reference to the file description of peer socketpair. - // TODO: It might be possible to retrieve writebuf from peer_fd and remove the writebuf - // field above. - peer_fd: WeakFileDescriptionRef, + /// The buffer we are reading from. + readbuf: RefCell, + /// The `SocketPair` file descriptor that is our "peer", and that holds the buffer we are + /// writing to. This is a weak reference because the other side may be closed before us; all + /// future writes will then trigger EPIPE. + peer_fd: OnceCell, is_nonblock: bool, } @@ -39,6 +35,18 @@ struct Buffer { buf_has_writer: bool, } +impl Buffer { + fn new() -> Self { + Buffer { buf: VecDeque::new(), clock: VClock::default(), buf_has_writer: true } + } +} + +impl SocketPair { + fn peer_fd(&self) -> &WeakFileDescriptionRef { + self.peer_fd.get().unwrap() + } +} + impl FileDescription for SocketPair { fn name(&self) -> &'static str { "socketpair" @@ -49,29 +57,29 @@ impl FileDescription for SocketPair { // need to be supported in the future, the check should be added here. let mut epoll_ready_events = EpollReadyEvents::new(); - let readbuf = self.readbuf.borrow(); // Check if it is readable. + let readbuf = self.readbuf.borrow(); if !readbuf.buf.is_empty() { epoll_ready_events.epollin = true; } // Check if is writable. - if let Some(writebuf) = self.writebuf.upgrade() { - let writebuf = writebuf.borrow(); + if let Some(peer_fd) = self.peer_fd().upgrade() { + let writebuf = &peer_fd.downcast::().unwrap().readbuf.borrow(); let data_size = writebuf.buf.len(); let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size); if available_space != 0 { epoll_ready_events.epollout = true; } - } - - // Check if the peer_fd closed - if self.peer_fd.upgrade().is_none() { + } else { + // Peer FD has been closed. epoll_ready_events.epollrdhup = true; - // This is an edge case. Whenever epollrdhup is triggered, epollin will be added - // even though there is no data in the buffer. + // This is an edge case. Whenever epollrdhup is triggered, epollin and epollout will be + // added even though there is no data in the buffer. + // FIXME: Figure out why. This looks like a bug. epoll_ready_events.epollin = true; + epoll_ready_events.epollout = true; } Ok(epoll_ready_events) } @@ -81,15 +89,13 @@ impl FileDescription for SocketPair { _communicate_allowed: bool, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { - // This is used to signal socketfd of other side that there is no writer to its readbuf. - // If the upgrade fails, there is no need to update as all read ends have been dropped. - if let Some(writebuf) = self.writebuf.upgrade() { - writebuf.borrow_mut().buf_has_writer = false; - }; + if let Some(peer_fd) = self.peer_fd().upgrade() { + // This is used to signal socketfd of other side that there is no writer to its readbuf. + // If the upgrade fails, there is no need to update as all read ends have been dropped. + peer_fd.downcast::().unwrap().readbuf.borrow_mut().buf_has_writer = false; - // Notify peer fd that closed has happened. - if let Some(peer_fd) = self.peer_fd.upgrade() { - // When any of the event happened, we check and update the status of all supported events + // Notify peer fd that closed has happened. + // When any of the events happened, we check and update the status of all supported events // types of peer fd. peer_fd.check_and_update_readiness(ecx)?; } @@ -97,20 +103,20 @@ impl FileDescription for SocketPair { } fn read<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, bytes: &mut [u8], ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { let request_byte_size = bytes.len(); - let mut readbuf = self.readbuf.borrow_mut(); // Always succeed on read size 0. if request_byte_size == 0 { return Ok(Ok(0)); } + let mut readbuf = self.readbuf.borrow_mut(); if readbuf.buf.is_empty() { if !readbuf.buf_has_writer { // Socketpair with no writer and empty buffer. @@ -141,8 +147,7 @@ impl FileDescription for SocketPair { // Conveniently, `read` exists on `VecDeque` and has exactly the desired behavior. let actual_read_size = readbuf.buf.read(bytes).unwrap(); - // The readbuf needs to be explicitly dropped because it will cause panic when - // check_and_update_readiness borrows it again. + // Need to drop before others can access the readbuf again. drop(readbuf); // A notification should be provided for the peer file description even when it can @@ -152,7 +157,7 @@ impl FileDescription for SocketPair { // don't know what that *certain number* is, we will provide a notification every time // a read is successful. This might result in our epoll emulation providing more // notifications than the real system. - if let Some(peer_fd) = self.peer_fd.upgrade() { + if let Some(peer_fd) = self.peer_fd().upgrade() { peer_fd.check_and_update_readiness(ecx)?; } @@ -160,7 +165,7 @@ impl FileDescription for SocketPair { } fn write<'tcx>( - &mut self, + &self, _communicate_allowed: bool, _fd_id: FdId, bytes: &[u8], @@ -173,12 +178,13 @@ impl FileDescription for SocketPair { return Ok(Ok(0)); } - let Some(writebuf) = self.writebuf.upgrade() else { + // 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 mut writebuf = writebuf.borrow_mut(); + let mut writebuf = peer_fd.downcast::().unwrap().readbuf.borrow_mut(); let data_size = writebuf.buf.len(); let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size); if available_space == 0 { @@ -198,13 +204,12 @@ impl FileDescription for SocketPair { let actual_write_size = write_size.min(available_space); writebuf.buf.extend(&bytes[..actual_write_size]); - // The writebuf needs to be explicitly dropped because it will cause panic when - // check_and_update_readiness borrows it again. + // Need to stop accessing peer_fd so that it can be notified. drop(writebuf); + // Notification should be provided for peer fd as it became readable. - if let Some(peer_fd) = self.peer_fd.upgrade() { - peer_fd.check_and_update_readiness(ecx)?; - } + peer_fd.check_and_update_readiness(ecx)?; + return Ok(Ok(actual_write_size)); } } @@ -268,51 +273,30 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } - let buffer1 = Rc::new(RefCell::new(Buffer { - buf: VecDeque::new(), - clock: VClock::default(), - buf_has_writer: true, - })); - - let buffer2 = Rc::new(RefCell::new(Buffer { - buf: VecDeque::new(), - clock: VClock::default(), - buf_has_writer: true, - })); - - let socketpair_0 = SocketPair { - writebuf: Rc::downgrade(&buffer1), - readbuf: Rc::clone(&buffer2), - peer_fd: WeakFileDescriptionRef::default(), + // Generate file descriptions. + let fds = &mut this.machine.fds; + let fd0 = fds.new_ref(SocketPair { + readbuf: RefCell::new(Buffer::new()), + peer_fd: OnceCell::new(), is_nonblock: is_sock_nonblock, - }; - let socketpair_1 = SocketPair { - writebuf: Rc::downgrade(&buffer2), - readbuf: Rc::clone(&buffer1), - peer_fd: WeakFileDescriptionRef::default(), + }); + let fd1 = fds.new_ref(SocketPair { + readbuf: RefCell::new(Buffer::new()), + peer_fd: OnceCell::new(), is_nonblock: is_sock_nonblock, - }; - - // Insert the file description to the fd table. - let fds = &mut this.machine.fds; - let sv0 = fds.insert_new(socketpair_0); - let sv1 = fds.insert_new(socketpair_1); - - // Get weak file descriptor and file description id value. - let fd_ref0 = fds.get_ref(sv0).unwrap(); - let fd_ref1 = fds.get_ref(sv1).unwrap(); - let weak_fd_ref0 = fd_ref0.downgrade(); - let weak_fd_ref1 = fd_ref1.downgrade(); + }); - // Update peer_fd and id field. - fd_ref1.borrow_mut().downcast_mut::().unwrap().peer_fd = weak_fd_ref0; + // Make the file descriptions point to each other. + fd0.downcast::().unwrap().peer_fd.set(fd1.downgrade()).unwrap(); + fd1.downcast::().unwrap().peer_fd.set(fd0.downgrade()).unwrap(); - fd_ref0.borrow_mut().downcast_mut::().unwrap().peer_fd = weak_fd_ref1; + // Insert the file description to the fd table, generating the file descriptors. + let sv0 = fds.insert(fd0); + let sv1 = fds.insert(fd1); - // Return socketpair file description value to the caller. + // Return socketpair file descriptors to the caller. let sv0 = Scalar::from_int(sv0, sv.layout.size); let sv1 = Scalar::from_int(sv1, sv.layout.size); - this.write_scalar(sv0, &sv)?; this.write_scalar(sv1, &sv.offset(sv.layout.size, sv.layout, this)?)?; diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index 11a0257dc4e3d..95fdf2f603507 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -19,6 +19,7 @@ fn main() { test_socketpair_read(); } +#[track_caller] fn check_epoll_wait( epfd: i32, mut expected_notifications: Vec<(u32, u64)>, @@ -28,6 +29,9 @@ fn check_epoll_wait( let maxsize = N; let array_ptr = array.as_mut_ptr(); let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), 0) }; + if res < 0 { + panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); + } assert_eq!(res, expected_notifications.len().try_into().unwrap()); let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; let mut return_events = slice.iter(); From 82c39ffda785753d90d1d1b98537e7b9d2d889b3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 12:15:07 +0200 Subject: [PATCH 2/7] buf_has_writer is not needed any more --- src/tools/miri/src/shims/unix/socket.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index 2694391dfbb34..fba63890dd275 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -29,15 +29,11 @@ struct SocketPair { struct Buffer { buf: VecDeque, clock: VClock, - /// Indicates if there is at least one active writer to this buffer. - /// If all writers of this buffer are dropped, buf_has_writer becomes false and we - /// indicate EOF instead of blocking. - buf_has_writer: bool, } impl Buffer { fn new() -> Self { - Buffer { buf: VecDeque::new(), clock: VClock::default(), buf_has_writer: true } + Buffer { buf: VecDeque::new(), clock: VClock::default() } } } @@ -90,10 +86,6 @@ impl FileDescription for SocketPair { ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { if let Some(peer_fd) = self.peer_fd().upgrade() { - // This is used to signal socketfd of other side that there is no writer to its readbuf. - // If the upgrade fails, there is no need to update as all read ends have been dropped. - peer_fd.downcast::().unwrap().readbuf.borrow_mut().buf_has_writer = false; - // Notify peer fd that closed has happened. // When any of the events happened, we check and update the status of all supported events // types of peer fd. @@ -118,8 +110,8 @@ impl FileDescription for SocketPair { let mut readbuf = self.readbuf.borrow_mut(); if readbuf.buf.is_empty() { - if !readbuf.buf_has_writer { - // Socketpair with no writer and empty buffer. + 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)); } else { From 34aec7c2067c1eb959061e1df6e2998c359e2ea8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 12:22:00 +0200 Subject: [PATCH 3/7] make ecx.check_and_update_readiness a truly private helper function --- src/tools/miri/src/shims/unix/fd.rs | 26 ++++++------------ src/tools/miri/src/shims/unix/fs.rs | 6 ++--- src/tools/miri/src/shims/unix/linux/epoll.rs | 21 +++++---------- .../miri/src/shims/unix/linux/eventfd.rs | 27 +++++-------------- src/tools/miri/src/shims/unix/socket.rs | 14 +++++----- 5 files changed, 31 insertions(+), 63 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 1fd1cf4d99e0e..e3b9835e360a6 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -28,8 +28,8 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Reads as much as possible into the given buffer, and returns the number of bytes read. fn read<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, _bytes: &mut [u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -39,8 +39,8 @@ pub trait FileDescription: std::fmt::Debug + Any { /// Writes as much as possible from the given buffer, and returns the number of bytes written. fn write<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, _bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -123,8 +123,8 @@ impl FileDescription for io::Stdin { fn read<'tcx>( &self, + _self_ref: &FileDescriptionRef, communicate_allowed: bool, - _fd_id: FdId, bytes: &mut [u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -147,8 +147,8 @@ impl FileDescription for io::Stdout { fn write<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -176,8 +176,8 @@ impl FileDescription for io::Stderr { fn write<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -202,8 +202,8 @@ impl FileDescription for NullOutput { fn write<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -261,16 +261,6 @@ impl FileDescriptionRef { pub fn get_id(&self) -> FdId { self.0.id } - - /// Function used to retrieve the readiness events of a file description and insert - /// an `EpollEventInstance` into the ready list if the file description is ready. - pub(crate) fn check_and_update_readiness<'tcx>( - &self, - ecx: &mut InterpCx<'tcx, MiriMachine<'tcx>>, - ) -> InterpResult<'tcx, ()> { - use crate::shims::unix::linux::epoll::EvalContextExt; - ecx.check_and_update_readiness(self.get_id(), || self.get_epoll_ready_events()) - } } /// Holds a weak reference to the actual file description. @@ -567,7 +557,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // `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(communicate, fd.get_id(), &mut bytes, this), + None => fd.read(&fd, communicate, &mut bytes, this), Some(offset) => { let Ok(offset) = u64::try_from(offset) else { let einval = this.eval_libc("EINVAL"); @@ -625,7 +615,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; let result = match offset { - None => fd.write(communicate, fd.get_id(), &bytes, this), + None => fd.write(&fd, communicate, &bytes, this), Some(offset) => { let Ok(offset) = u64::try_from(offset) else { let einval = this.eval_libc("EINVAL"); diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index e6076abedbd49..80f4b89bf34d3 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -12,7 +12,7 @@ use rustc_data_structures::fx::FxHashMap; use rustc_target::abi::Size; use crate::shims::os_str::bytes_to_os_str; -use crate::shims::unix::fd::FdId; +use crate::shims::unix::fd::FileDescriptionRef; use crate::shims::unix::*; use crate::*; use shims::time::system_time_to_duration; @@ -32,8 +32,8 @@ impl FileDescription for FileHandle { fn read<'tcx>( &self, + _self_ref: &FileDescriptionRef, communicate_allowed: bool, - _fd_id: FdId, bytes: &mut [u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -43,8 +43,8 @@ impl FileDescription for FileHandle { fn write<'tcx>( &self, + _self_ref: &FileDescriptionRef, communicate_allowed: bool, - _fd_id: FdId, bytes: &[u8], _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index a1b943f2e82a1..1ec2dbe233e2d 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -3,7 +3,7 @@ use std::collections::BTreeMap; use std::io; use std::rc::{Rc, Weak}; -use crate::shims::unix::fd::FdId; +use crate::shims::unix::fd::{FdId, FileDescriptionRef}; use crate::shims::unix::*; use crate::*; @@ -309,7 +309,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } // Readiness will be updated immediately when the epoll_event_interest is added or modified. - file_descriptor.check_and_update_readiness(this)?; + this.check_and_update_readiness(&file_descriptor)?; return Ok(Scalar::from_i32(0)); } else if op == epoll_ctl_del { @@ -432,22 +432,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_i32(num_of_events)) } - /// For a specific unique file descriptor id, get its ready events and update + /// For a specific file description, get its ready events and update /// the corresponding ready list. This function is called whenever a file description - /// is registered with epoll, or when read, write, or close operations are performed, - /// regardless of any changes in readiness. - /// - /// This is an internal helper function and is typically not meant to be used directly. - /// In most cases, `FileDescriptionRef::check_and_update_readiness` should be preferred. - fn check_and_update_readiness( - &self, - id: FdId, - get_ready_events: impl FnOnce() -> InterpResult<'tcx, EpollReadyEvents>, - ) -> InterpResult<'tcx, ()> { + /// is registered with epoll, or when its readiness *might* have changed. + fn check_and_update_readiness(&self, fd_ref: &FileDescriptionRef) -> InterpResult<'tcx, ()> { let this = self.eval_context_ref(); + let id = fd_ref.get_id(); // Get a list of EpollEventInterest that is associated to a specific file description. if let Some(epoll_interests) = this.machine.epoll_interests.get_epoll_interest(id) { - let epoll_ready_events = get_ready_events()?; + let epoll_ready_events = fd_ref.get_epoll_ready_events()?; // Get the bitmask of ready events. let ready_events = epoll_ready_events.get_event_bitmask(this); diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index cede3967bc846..77d16a032aae9 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -4,10 +4,10 @@ use std::io; use std::io::{Error, ErrorKind}; use std::mem; -use fd::FdId; use rustc_target::abi::Endian; -use crate::shims::unix::linux::epoll::EpollReadyEvents; +use crate::shims::unix::fd::FileDescriptionRef; +use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; use crate::{concurrency::VClock, *}; @@ -60,8 +60,8 @@ impl FileDescription for Event { /// Read the counter in the buffer and return the counter if succeeded. fn read<'tcx>( &self, + self_ref: &FileDescriptionRef, _communicate_allowed: bool, - fd_id: FdId, bytes: &mut [u8], ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -89,16 +89,8 @@ impl FileDescription for Event { 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)?; - // We have to use our own FdID in contrast to every other file descriptor out there, because - // we are updating ourselves when writing and reading. Technically `Event` is like socketpair, but - // it does not create two separate file descriptors. Thus we can't re-borrow ourselves via - // `FileDescriptionRef::check_and_update_readiness` while already being mutably borrowed for read/write. - crate::shims::unix::linux::epoll::EvalContextExt::check_and_update_readiness( - ecx, - fd_id, - || self.get_epoll_ready_events(), - )?; return Ok(Ok(U64_ARRAY_SIZE)); } } @@ -117,8 +109,8 @@ impl FileDescription for Event { /// made to write the value 0xffffffffffffffff. fn write<'tcx>( &self, + self_ref: &FileDescriptionRef, _communicate_allowed: bool, - fd_id: FdId, bytes: &[u8], ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -156,15 +148,8 @@ impl FileDescription for Event { }; // 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)?; - // Just like read() above, we use this internal method to not get the second borrow of the - // RefCell of this FileDescription. This is a special case, we should only use - // FileDescriptionRef::check_and_update_readiness in normal case. - crate::shims::unix::linux::epoll::EvalContextExt::check_and_update_readiness( - ecx, - fd_id, - || self.get_epoll_ready_events(), - )?; Ok(Ok(U64_ARRAY_SIZE)) } } diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index fba63890dd275..0f13c8c35adcf 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -3,8 +3,8 @@ use std::collections::VecDeque; use std::io; use std::io::{Error, ErrorKind, Read}; -use crate::shims::unix::fd::{FdId, WeakFileDescriptionRef}; -use crate::shims::unix::linux::epoll::EpollReadyEvents; +use crate::shims::unix::fd::{FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::unix::linux::epoll::{EpollReadyEvents, EvalContextExt as _}; use crate::shims::unix::*; use crate::{concurrency::VClock, *}; @@ -89,15 +89,15 @@ impl FileDescription for SocketPair { // Notify peer fd that closed has happened. // When any of the events happened, we check and update the status of all supported events // types of peer fd. - peer_fd.check_and_update_readiness(ecx)?; + ecx.check_and_update_readiness(&peer_fd)?; } Ok(Ok(())) } fn read<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, bytes: &mut [u8], ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -150,7 +150,7 @@ impl FileDescription for SocketPair { // a read is successful. This might result in our epoll emulation providing more // notifications than the real system. if let Some(peer_fd) = self.peer_fd().upgrade() { - peer_fd.check_and_update_readiness(ecx)?; + ecx.check_and_update_readiness(&peer_fd)?; } return Ok(Ok(actual_read_size)); @@ -158,8 +158,8 @@ impl FileDescription for SocketPair { fn write<'tcx>( &self, + _self_ref: &FileDescriptionRef, _communicate_allowed: bool, - _fd_id: FdId, bytes: &[u8], ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result> { @@ -200,7 +200,7 @@ impl FileDescription for SocketPair { drop(writebuf); // Notification should be provided for peer fd as it became readable. - peer_fd.check_and_update_readiness(ecx)?; + ecx.check_and_update_readiness(&peer_fd)?; return Ok(Ok(actual_write_size)); } From b4ab820e5e20dfca8f3242ac2547f8abf169e30e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 16:54:02 +0200 Subject: [PATCH 4/7] epoll test cleanup --- .../miri/tests/pass-dep/libc/libc-epoll.rs | 176 ++++++------------ 1 file changed, 58 insertions(+), 118 deletions(-) diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index 95fdf2f603507..364a784574a75 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -5,20 +5,24 @@ use std::convert::TryInto; use std::mem::MaybeUninit; fn main() { + test_epoll_socketpair(); + test_epoll_socketpair_both_sides(); + test_socketpair_read(); + test_epoll_eventfd(); + test_event_overwrite(); test_not_fully_closed_fd(); test_closed_fd(); - test_epoll_socketpair_special_case(); test_two_epoll_instance(); test_epoll_ctl_mod(); - test_epoll_socketpair(); - test_epoll_eventfd(); test_epoll_ctl_del(); test_pointer(); test_two_same_fd_in_same_epoll_instance(); - test_socketpair_read(); } +// Using `as` cast since `EPOLLET` wraps around +const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; + #[track_caller] fn check_epoll_wait( epfd: i32, @@ -58,21 +62,17 @@ fn test_epoll_socketpair() { // Create a socketpair instance. let mut fds = [-1, -1]; - let mut res = - unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; assert_eq!(res, 0); // Write to fd[0] let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; assert_eq!(res, 5); - // Register fd[1] with EPOLLIN|EPOLLOUT|EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLRDHUP).unwrap() | epollet; + // Register fd[1] with EPOLLIN|EPOLLOUT|EPOLLET|EPOLLRDHUP let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), + events: (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET | libc::EPOLLRDHUP) as _, u64: u64::try_from(fds[1]).unwrap(), }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; @@ -100,23 +100,17 @@ fn test_epoll_ctl_mod() { // Create a socketpair instance. let mut fds = [-1, -1]; - let mut res = - unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; assert_eq!(res, 0); // Write to fd[0]. let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; assert_eq!(res, 5); // Register fd[1] with EPOLLIN|EPOLLOUT|EPOLLET. - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let mut flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: u64::try_from(fds[1]).unwrap(), - }; + // (Not using checked cast as EPOLLET wraps around.) + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fds[1]).unwrap() }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); @@ -126,9 +120,8 @@ fn test_epoll_ctl_mod() { assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); // Test EPOLLRDHUP. - flags |= u32::try_from(libc::EPOLLRDHUP).unwrap(); let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), + events: (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET | libc::EPOLLRDHUP) as _, u64: u64::try_from(fds[1]).unwrap(), }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_MOD, fds[1], &mut ev) }; @@ -151,23 +144,16 @@ fn test_epoll_ctl_del() { // Create a socketpair instance. let mut fds = [-1, -1]; - let mut res = - unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; assert_eq!(res, 0); // Write to fd[0] let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; assert_eq!(res, 5); // Register fd[1] with EPOLLIN|EPOLLOUT|EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: u64::try_from(fds[1]).unwrap(), - }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fds[1]).unwrap() }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); @@ -185,22 +171,16 @@ fn test_two_epoll_instance() { // Create a socketpair instance. let mut fds = [-1, -1]; - let mut res = - unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; + let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) }; assert_eq!(res, 0); // Write to the socketpair. let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; assert_eq!(res, 5); // Register one side of the socketpair with EPOLLIN | EPOLLOUT | EPOLLET. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: u64::try_from(fds[1]).unwrap(), - }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fds[1]).unwrap() }; let res = unsafe { libc::epoll_ctl(epfd1, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); let res = unsafe { libc::epoll_ctl(epfd2, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; @@ -230,17 +210,15 @@ fn test_two_same_fd_in_same_epoll_instance() { assert_ne!(newfd, -1); // Register both fd to the same epoll instance. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { events: u32::try_from(flags).unwrap(), u64: 5 as u64 }; - let mut res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: 5 as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); - res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, newfd, &mut ev) }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, newfd, &mut ev) }; assert_ne!(res, -1); // Write to the socketpair. let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; assert_eq!(res, 5); //Two notification should be received. @@ -259,9 +237,7 @@ fn test_epoll_eventfd() { // Write to the eventfd instance. let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); - let res: i32 = unsafe { - libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() - }; + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; assert_eq!(res, 8); // Create an epoll instance. @@ -269,13 +245,7 @@ fn test_epoll_eventfd() { assert_ne!(epfd, -1); // Register eventfd with EPOLLIN | EPOLLOUT | EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: u64::try_from(fd).unwrap(), - }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fd).unwrap() }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; assert_ne!(res, -1); @@ -296,20 +266,15 @@ fn test_pointer() { assert_eq!(res, 0); // Register fd[1] with EPOLLIN|EPOLLOUT|EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLRDHUP).unwrap() | epollet; let data = MaybeUninit::::uninit().as_ptr(); - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: data.expose_provenance() as u64, - }; + let mut ev = + libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: data.expose_provenance() as u64 }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); } // When read/write happened on one side of the socketpair, only the other side will be notified. -fn test_epoll_socketpair_special_case() { +fn test_epoll_socketpair_both_sides() { // Create an epoll instance. let epfd = unsafe { libc::epoll_create1(0) }; assert_ne!(epfd, -1); @@ -320,18 +285,16 @@ fn test_epoll_socketpair_special_case() { assert_eq!(res, 0); // Register both fd to the same epoll instance. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { events: u32::try_from(flags).unwrap(), u64: fds[0] as u64 }; - let mut res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; assert_ne!(res, -1); - let mut ev = libc::epoll_event { events: u32::try_from(flags).unwrap(), u64: fds[1] as u64 }; - res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[1] as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); // Write to fds[1]. let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5) }; assert_eq!(res, 5); //Two notification should be received. @@ -346,9 +309,7 @@ fn test_epoll_socketpair_special_case() { // Read from fds[0]. let mut buf: [u8; 5] = [0; 5]; - res = unsafe { - libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t).try_into().unwrap() - }; + let res = unsafe { libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; assert_eq!(res, 5); assert_eq!(buf, "abcde".as_bytes()); @@ -370,21 +331,13 @@ fn test_closed_fd() { let fd = unsafe { libc::eventfd(0, flags) }; // Register eventfd with EPOLLIN | EPOLLOUT | EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: u64::try_from(fd).unwrap(), - }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fd).unwrap() }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; assert_ne!(res, -1); // Write to the eventfd instance. let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); - let res: i32 = unsafe { - libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() - }; + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; assert_eq!(res, 8); // Close the eventfd. @@ -415,13 +368,7 @@ fn test_not_fully_closed_fd() { assert_ne!(newfd, -1); // Register eventfd with EPOLLIN | EPOLLOUT | EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), - u64: u64::try_from(fd).unwrap(), - }; + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: u64::try_from(fd).unwrap() }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; assert_ne!(res, -1); @@ -436,9 +383,7 @@ fn test_not_fully_closed_fd() { // Write to the eventfd instance to produce notification. let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); - let res: i32 = unsafe { - libc::write(newfd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() - }; + let res = unsafe { libc::write(newfd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; assert_eq!(res, 8); // Close the dupped fd. @@ -458,9 +403,7 @@ fn test_event_overwrite() { // Write to the eventfd instance. let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); - let res: i32 = unsafe { - libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8).try_into().unwrap() - }; + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; assert_eq!(res, 8); // Create an epoll instance. @@ -468,11 +411,8 @@ fn test_event_overwrite() { assert_ne!(epfd, -1); // Register eventfd with EPOLLIN | EPOLLOUT | EPOLLET - // EPOLLET is negative number for i32 so casting is needed to do proper bitwise OR for u32. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; let mut ev = libc::epoll_event { - events: u32::try_from(flags).unwrap(), + events: (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _, u64: u64::try_from(fd).unwrap(), }; let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; @@ -480,7 +420,7 @@ fn test_event_overwrite() { // Read from the eventfd instance. let mut buf: [u8; 8] = [0; 8]; - let res: i32 = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), 8).try_into().unwrap() }; + let res = unsafe { libc::read(fd, buf.as_mut_ptr().cast(), 8) }; assert_eq!(res, 8); // Check result from epoll_wait. @@ -502,18 +442,22 @@ fn test_socketpair_read() { assert_eq!(res, 0); // Register both fd to the same epoll instance. - let epollet = libc::EPOLLET as u32; - let flags = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap() | epollet; - let mut ev = libc::epoll_event { events: u32::try_from(flags).unwrap(), u64: fds[0] as u64 }; - let mut res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; + let mut ev = libc::epoll_event { + events: (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _, + u64: fds[0] as u64, + }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; assert_ne!(res, -1); - let mut ev = libc::epoll_event { events: u32::try_from(flags).unwrap(), u64: fds[1] as u64 }; - res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; + let mut ev = libc::epoll_event { + events: (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _, + u64: fds[1] as u64, + }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_ne!(res, -1); // Write 5 bytes to fds[1]. let data = "abcde".as_bytes().as_ptr(); - res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5).try_into().unwrap() }; + let res = unsafe { libc::write(fds[1], data as *const libc::c_void, 5) }; assert_eq!(res, 5); //Two notification should be received. @@ -528,9 +472,7 @@ fn test_socketpair_read() { // Read 3 bytes from fds[0]. let mut buf: [u8; 3] = [0; 3]; - res = unsafe { - libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t).try_into().unwrap() - }; + let res = unsafe { libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; assert_eq!(res, 3); assert_eq!(buf, "abc".as_bytes()); @@ -542,9 +484,7 @@ fn test_socketpair_read() { // Read until the buffer is empty. let mut buf: [u8; 2] = [0; 2]; - res = unsafe { - libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t).try_into().unwrap() - }; + let res = unsafe { libc::read(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; assert_eq!(res, 2); assert_eq!(buf, "de".as_bytes()); From edd1efb136f93d8e29bf15610f6bca2f919d4ad7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 16:55:45 +0200 Subject: [PATCH 5/7] comment and test regarding notifications on writes that dont change readiness --- src/tools/miri/src/shims/unix/linux/epoll.rs | 5 ++++- src/tools/miri/src/shims/unix/socket.rs | 1 + src/tools/miri/tests/pass-dep/libc/libc-epoll.rs | 12 ++++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 1ec2dbe233e2d..ce91712fb2798 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -35,6 +35,7 @@ impl EpollEventInstance { EpollEventInstance { events, data } } } + /// EpollEventInterest registers the file description information to an epoll /// instance during a successful `epoll_ctl` call. It also stores additional /// information needed to check and update readiness state for `epoll_wait`. @@ -434,7 +435,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// For a specific file description, get its ready events and update /// the corresponding ready list. This function is called whenever a file description - /// is registered with epoll, or when its readiness *might* have changed. + /// is registered with epoll, or the buffer it reads from / writes to changed. + /// This *will* report an event if anyone is subscribed to it, without any further + /// filtering, so do not call this function when an FD didn't have anything happen to it! fn check_and_update_readiness(&self, fd_ref: &FileDescriptionRef) -> InterpResult<'tcx, ()> { let this = self.eval_context_ref(); let id = fd_ref.get_id(); diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index 0f13c8c35adcf..cdc9dd0429b40 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -200,6 +200,7 @@ impl FileDescription for SocketPair { drop(writebuf); // Notification should be provided for peer fd as it became readable. + // 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)); diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index 364a784574a75..c27a6a83a0542 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -83,6 +83,18 @@ fn test_epoll_socketpair() { let expected_value = u64::try_from(fds[1]).unwrap(); assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + // Check that this is indeed using "ET" (edge-trigger) semantics: a second epoll should return nothing. + assert!(check_epoll_wait::<8>(epfd, vec![])); + + // Write some more to fd[0]. + let data = "abcde".as_bytes().as_ptr(); + let res = unsafe { libc::write(fds[0], data as *const libc::c_void, 5) }; + assert_eq!(res, 5); + + // This did not change the readiness of fd[1]. And yet, we're seeing the event reported + // again by the kernel, so Miri does the same. + assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + // Close the peer socketpair. let res = unsafe { libc::close(fds[0]) }; assert_eq!(res, 0); From 9184eb54eab8d3968d107d24db65cb16d3e27cab Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 16:58:49 +0200 Subject: [PATCH 6/7] more epoll test cleanup --- .../miri/tests/pass-dep/libc/libc-epoll.rs | 54 +++++++++---------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index c27a6a83a0542..f8bf1bacff3eb 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -24,10 +24,7 @@ fn main() { const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; #[track_caller] -fn check_epoll_wait( - epfd: i32, - mut expected_notifications: Vec<(u32, u64)>, -) -> bool { +fn check_epoll_wait(epfd: i32, expected_notifications: &[(u32, u64)]) -> bool { let epoll_event = libc::epoll_event { events: 0, u64: 0 }; let mut array: [libc::epoll_event; N] = [epoll_event; N]; let maxsize = N; @@ -39,8 +36,9 @@ fn check_epoll_wait( assert_eq!(res, expected_notifications.len().try_into().unwrap()); let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; let mut return_events = slice.iter(); + let mut expected_notifications = expected_notifications.iter(); while let Some(return_event) = return_events.next() { - if let Some(notification) = expected_notifications.pop() { + if let Some(notification) = expected_notifications.next() { let event = return_event.events; let data = return_event.u64; assert_eq!(event, notification.0); @@ -49,10 +47,8 @@ fn check_epoll_wait( return false; } } - if !expected_notifications.is_empty() { - return false; - } - return true; + // There shouldn't be any more expected. + return expected_notifications.next().is_none(); } fn test_epoll_socketpair() { @@ -81,10 +77,10 @@ fn test_epoll_socketpair() { // Check result from epoll_wait. let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); // Check that this is indeed using "ET" (edge-trigger) semantics: a second epoll should return nothing. - assert!(check_epoll_wait::<8>(epfd, vec![])); + assert!(check_epoll_wait::<8>(epfd, &[])); // Write some more to fd[0]. let data = "abcde".as_bytes().as_ptr(); @@ -93,7 +89,7 @@ fn test_epoll_socketpair() { // This did not change the readiness of fd[1]. And yet, we're seeing the event reported // again by the kernel, so Miri does the same. - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); // Close the peer socketpair. let res = unsafe { libc::close(fds[0]) }; @@ -102,7 +98,7 @@ fn test_epoll_socketpair() { // Check result from epoll_wait. let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); } fn test_epoll_ctl_mod() { @@ -129,7 +125,7 @@ fn test_epoll_ctl_mod() { // Check result from epoll_wait. let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); // Test EPOLLRDHUP. let mut ev = libc::epoll_event { @@ -146,7 +142,7 @@ fn test_epoll_ctl_mod() { // Check result from epoll_wait. let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); } fn test_epoll_ctl_del() { @@ -170,7 +166,7 @@ fn test_epoll_ctl_del() { assert_ne!(res, -1); // Test EPOLL_CTL_DEL. - assert!(check_epoll_wait::<0>(epfd, vec![])); + assert!(check_epoll_wait::<0>(epfd, &[])); } // This test is for one fd registered under two different epoll instance. @@ -201,8 +197,8 @@ fn test_two_epoll_instance() { // Notification should be received from both instance of epoll. let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); - assert!(check_epoll_wait::<8>(epfd1, vec![(expected_event, expected_value)])); - assert!(check_epoll_wait::<8>(epfd2, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd1, &[(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd2, &[(expected_event, expected_value)])); } // This test is for two same file description registered under the same epoll instance through dup. @@ -238,7 +234,7 @@ fn test_two_same_fd_in_same_epoll_instance() { let expected_value = 5 as u64; assert!(check_epoll_wait::<8>( epfd, - vec![(expected_event, expected_value), (expected_event, expected_value)] + &[(expected_event, expected_value), (expected_event, expected_value)] )); } @@ -264,7 +260,7 @@ fn test_epoll_eventfd() { // Check result from epoll_wait. let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fd).unwrap(); - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); } fn test_pointer() { @@ -316,7 +312,7 @@ fn test_epoll_socketpair_both_sides() { let expected_value1 = fds[1] as u64; assert!(check_epoll_wait::<8>( epfd, - vec![(expected_event1, expected_value1), (expected_event0, expected_value0)] + &[(expected_event0, expected_value0), (expected_event1, expected_value1)] )); // Read from fds[0]. @@ -328,7 +324,7 @@ fn test_epoll_socketpair_both_sides() { // Notification should be provided for fds[1]. let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value = fds[1] as u64; - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); } // When file description is fully closed, epoll_wait should not provide any notification for @@ -357,7 +353,7 @@ fn test_closed_fd() { assert_eq!(res, 0); // No notification should be provided because the file description is closed. - assert!(check_epoll_wait::<8>(epfd, vec![])); + assert!(check_epoll_wait::<8>(epfd, &[])); } // When a certain file descriptor registered with epoll is closed, but the underlying file description @@ -391,7 +387,7 @@ fn test_not_fully_closed_fd() { // Notification should still be provided because the file description is not closed. let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value = fd as u64; - assert!(check_epoll_wait::<1>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)])); // Write to the eventfd instance to produce notification. let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); @@ -403,7 +399,7 @@ fn test_not_fully_closed_fd() { assert_eq!(res, 0); // No notification should be provided. - assert!(check_epoll_wait::<1>(epfd, vec![])); + assert!(check_epoll_wait::<1>(epfd, &[])); } // Each time a notification is provided, it should reflect the file description's readiness @@ -438,7 +434,7 @@ fn test_event_overwrite() { // Check result from epoll_wait. let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fd).unwrap(); - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); } // An epoll notification will be provided for every succesful read in a socketpair. @@ -479,7 +475,7 @@ fn test_socketpair_read() { let expected_value1 = fds[1] as u64; assert!(check_epoll_wait::<8>( epfd, - vec![(expected_event1, expected_value1), (expected_event0, expected_value0)] + &[(expected_event0, expected_value0), (expected_event1, expected_value1)] )); // Read 3 bytes from fds[0]. @@ -492,7 +488,7 @@ fn test_socketpair_read() { // But in real system, no notification will be provided here. let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value = fds[1] as u64; - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); // Read until the buffer is empty. let mut buf: [u8; 2] = [0; 2]; @@ -504,5 +500,5 @@ fn test_socketpair_read() { // In real system, notification will be provided too. let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value = fds[1] as u64; - assert!(check_epoll_wait::<8>(epfd, vec![(expected_event, expected_value)])); + assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)])); } From 883e4773b30f652e83326df99dc0ec7d652e1f94 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 16 Aug 2024 17:06:37 +0200 Subject: [PATCH 7/7] explain the behavior on closed peers --- src/tools/miri/src/shims/unix/linux/epoll.rs | 12 +++++++----- src/tools/miri/src/shims/unix/socket.rs | 10 ++++------ src/tools/miri/tests/pass-dep/libc/libc-epoll.rs | 1 + 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index ce91712fb2798..d529a9b63e683 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -433,11 +433,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok(Scalar::from_i32(num_of_events)) } - /// For a specific file description, get its ready events and update - /// the corresponding ready list. This function is called whenever a file description - /// is registered with epoll, or the buffer it reads from / writes to changed. - /// This *will* report an event if anyone is subscribed to it, without any further - /// filtering, so do not call this function when an FD didn't have anything happen to it! + /// For a specific file description, get its ready events and update the corresponding ready + /// list. This function should be called whenever an event causes more bytes or an EOF to become + /// newly readable from an FD, and whenever more bytes can be written to an FD or no more future + /// writes are possible. + /// + /// This *will* report an event if anyone is subscribed to it, without any further filtering, so + /// do not call this function when an FD didn't have anything happen to it! fn check_and_update_readiness(&self, fd_ref: &FileDescriptionRef) -> InterpResult<'tcx, ()> { let this = self.eval_context_ref(); let id = fd_ref.get_id(); diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index cdc9dd0429b40..75d4d0dbbe497 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -71,9 +71,9 @@ impl FileDescription for SocketPair { } else { // Peer FD has been closed. epoll_ready_events.epollrdhup = true; - // This is an edge case. Whenever epollrdhup is triggered, epollin and epollout will be - // added even though there is no data in the buffer. - // FIXME: Figure out why. This looks like a bug. + // Since the peer is closed, even if no data is available reads will return EOF and + // writes will return EPIPE. In other words, they won't block, so we mark this as ready + // for read and write. epoll_ready_events.epollin = true; epoll_ready_events.epollout = true; } @@ -86,9 +86,7 @@ impl FileDescription for SocketPair { ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { if let Some(peer_fd) = self.peer_fd().upgrade() { - // Notify peer fd that closed has happened. - // When any of the events happened, we check and update the status of all supported events - // types of peer fd. + // Notify peer fd that close has happened, since that can unblock reads and writes. ecx.check_and_update_readiness(&peer_fd)?; } Ok(Ok(())) diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs index f8bf1bacff3eb..841761e53dd56 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll.rs @@ -96,6 +96,7 @@ fn test_epoll_socketpair() { assert_eq!(res, 0); // Check result from epoll_wait. + // We expect to get a read, write, HUP notification from the close since closing an FD always unblocks reads and writes on its peer. let expected_event = u32::try_from(libc::EPOLLRDHUP | libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); assert!(check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]));