From 660b1a2ccd7d392db5cb2ddc46038c9605be30fc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Dec 2024 15:25:34 +0100 Subject: [PATCH] FD handling: avoid unnecessary dynamic downcasts --- src/lib.rs | 2 + src/shims/files.rs | 253 +++++++++++++++------------ src/shims/unix/fd.rs | 9 +- src/shims/unix/fs.rs | 36 ++-- src/shims/unix/linux_like/epoll.rs | 46 +++-- src/shims/unix/linux_like/eventfd.rs | 60 +++---- src/shims/unix/unnamed_socket.rs | 84 ++++----- 7 files changed, 243 insertions(+), 247 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index e02d51afce..d2808395a0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,8 @@ #![feature(strict_overflow_ops)] #![feature(pointer_is_aligned_to)] #![feature(unqualified_local_imports)] +#![feature(derive_coerce_pointee)] +#![feature(arbitrary_self_types)] // Configure clippy and other lints #![allow( clippy::collapsible_else_if, diff --git a/src/shims/files.rs b/src/shims/files.rs index f673b834be..035b82c687 100644 --- a/src/shims/files.rs +++ b/src/shims/files.rs @@ -1,6 +1,7 @@ use std::any::Any; use std::collections::BTreeMap; use std::io::{IsTerminal, Read, SeekFrom, Write}; +use std::marker::CoercePointee; use std::ops::Deref; use std::rc::{Rc, Weak}; use std::{fs, io}; @@ -10,16 +11,126 @@ use rustc_abi::Size; use crate::shims::unix::UnixFileDescription; use crate::*; +/// A unique id for file descriptions. While we could use the address, considering that +/// is definitely unique, the address would expose interpreter internal state when used +/// for sorting things. So instead we generate a unique id per file description is the name +/// for all `dup`licates and is never reused. +#[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] +pub struct FdId(usize); + +#[derive(Debug, Clone)] +struct FdIdWith { + id: FdId, + inner: T, +} + +/// A refcounted pointer to a file description, also tracking the +/// globally unique ID of this file description. +#[repr(transparent)] +#[derive(CoercePointee, Debug)] +pub struct FileDescriptionRef(Rc>); + +impl Clone for FileDescriptionRef { + fn clone(&self) -> Self { + FileDescriptionRef(self.0.clone()) + } +} + +impl Deref for FileDescriptionRef { + type Target = T; + fn deref(&self) -> &T { + &self.0.inner + } +} + +impl FileDescriptionRef { + pub fn id(&self) -> FdId { + self.0.id + } +} + +/// Holds a weak reference to the actual file description. +#[derive(Clone, Debug)] +pub struct WeakFileDescriptionRef(Weak>); + +impl FileDescriptionRef { + pub fn downgrade(this: &Self) -> WeakFileDescriptionRef { + WeakFileDescriptionRef(Rc::downgrade(&this.0)) + } +} + +impl WeakFileDescriptionRef { + pub fn upgrade(&self) -> Option> { + self.0.upgrade().map(FileDescriptionRef) + } +} + +impl VisitProvenance for WeakFileDescriptionRef { + fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { + // A weak reference can never be the only reference to some pointer or place. + // Since the actual file description is tracked by strong ref somewhere, + // it is ok to make this a NOP operation. + } +} + +/// A helper trait to indirectly allow downcasting on `Rc>`. +/// Ideally we'd just add a `FdIdWith: Any` bound to the `FileDescription` trait, +/// but that does not allow upcasting. +pub trait FileDescriptionExt: 'static { + fn into_rc_any(self: FileDescriptionRef) -> Rc; + + /// We wrap the regular `close` function generically, so both handle `Rc::into_inner` + /// and epoll interest management. + fn close_ref<'tcx>( + self: FileDescriptionRef, + communicate_allowed: bool, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>>; +} + +impl FileDescriptionExt for T { + fn into_rc_any(self: FileDescriptionRef) -> Rc { + self.0 + } + + fn close_ref<'tcx>( + self: FileDescriptionRef, + communicate_allowed: bool, + ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result<()>> { + match Rc::into_inner(self.0) { + Some(fd) => { + // Remove entry from the global epoll_event_interest table. + ecx.machine.epoll_interests.remove(fd.id); + + fd.inner.close(communicate_allowed, ecx) + } + None => { + // Not the last reference. + interp_ok(Ok(())) + } + } + } +} + +pub type DynFileDescriptionRef = FileDescriptionRef; + +impl FileDescriptionRef { + pub fn downcast(self) -> Option> { + let inner = self.into_rc_any().downcast::>().ok()?; + Some(FileDescriptionRef(inner)) + } +} + /// Represents an open file description. -pub trait FileDescription: std::fmt::Debug + Any { +pub trait FileDescription: std::fmt::Debug + FileDescriptionExt { fn name(&self) -> &'static str; /// 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, + self: FileDescriptionRef, _communicate_allowed: bool, _ptr: Pointer, _len: usize, @@ -33,8 +144,7 @@ pub trait FileDescription: std::fmt::Debug + Any { /// `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, + self: FileDescriptionRef, _communicate_allowed: bool, _ptr: Pointer, _len: usize, @@ -54,11 +164,15 @@ pub trait FileDescription: std::fmt::Debug + Any { throw_unsup_format!("cannot seek on {}", self.name()); } + /// Close the file descriptor. fn close<'tcx>( - self: Box, + self, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result<()>> { + ) -> InterpResult<'tcx, io::Result<()>> + where + Self: Sized, + { throw_unsup_format!("cannot close {}", self.name()); } @@ -77,21 +191,13 @@ pub trait FileDescription: std::fmt::Debug + Any { } } -impl dyn FileDescription { - #[inline(always)] - pub fn downcast(&self) -> Option<&T> { - (self as &dyn Any).downcast_ref() - } -} - impl FileDescription for io::Stdin { fn name(&self) -> &'static str { "stdin" } fn read<'tcx>( - &self, - _self_ref: &FileDescriptionRef, + self: FileDescriptionRef, communicate_allowed: bool, ptr: Pointer, len: usize, @@ -103,7 +209,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")?; } - let result = Read::read(&mut { self }, &mut bytes); + let result = Read::read(&mut &*self, &mut bytes); match result { Ok(read_size) => ecx.return_read_success(ptr, &bytes, read_size, dest), Err(e) => ecx.set_last_error_and_return(e, dest), @@ -121,8 +227,7 @@ impl FileDescription for io::Stdout { } fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, len: usize, @@ -131,7 +236,7 @@ impl FileDescription for io::Stdout { ) -> InterpResult<'tcx> { let bytes = ecx.read_bytes_ptr_strip_provenance(ptr, Size::from_bytes(len))?; // We allow writing to stderr even with isolation enabled. - let result = Write::write(&mut { self }, bytes); + 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 @@ -155,8 +260,7 @@ impl FileDescription for io::Stderr { } fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, len: usize, @@ -166,7 +270,7 @@ impl FileDescription for io::Stderr { 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); match result { Ok(write_size) => ecx.return_write_success(write_size, dest), Err(e) => ecx.set_last_error_and_return(e, dest), @@ -188,8 +292,7 @@ impl FileDescription for NullOutput { } fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, _ptr: Pointer, len: usize, @@ -201,91 +304,10 @@ impl FileDescription for NullOutput { } } -/// Structure contains both the file description and its unique identifier. -#[derive(Clone, Debug)] -pub struct FileDescWithId { - id: FdId, - file_description: Box, -} - -#[derive(Clone, Debug)] -pub struct FileDescriptionRef(Rc>); - -impl Deref for FileDescriptionRef { - type Target = dyn FileDescription; - - fn deref(&self) -> &Self::Target { - &*self.0.file_description - } -} - -impl FileDescriptionRef { - fn new(fd: impl FileDescription, id: FdId) -> Self { - FileDescriptionRef(Rc::new(FileDescWithId { id, file_description: Box::new(fd) })) - } - - pub fn close<'tcx>( - self, - communicate_allowed: bool, - ecx: &mut MiriInterpCx<'tcx>, - ) -> InterpResult<'tcx, io::Result<()>> { - // Destroy this `Rc` using `into_inner` so we can call `close` instead of - // implicitly running the destructor of the file description. - let id = self.get_id(); - match Rc::into_inner(self.0) { - Some(fd) => { - // Remove entry from the global epoll_event_interest table. - ecx.machine.epoll_interests.remove(id); - - fd.file_description.close(communicate_allowed, ecx) - } - None => interp_ok(Ok(())), - } - } - - pub fn downgrade(&self) -> WeakFileDescriptionRef { - WeakFileDescriptionRef { weak_ref: Rc::downgrade(&self.0) } - } - - pub fn get_id(&self) -> FdId { - self.0.id - } -} - -/// Holds a weak reference to the actual file description. -#[derive(Clone, Debug, Default)] -pub struct WeakFileDescriptionRef { - weak_ref: Weak>, -} - -impl WeakFileDescriptionRef { - pub fn upgrade(&self) -> Option { - if let Some(file_desc_with_id) = self.weak_ref.upgrade() { - return Some(FileDescriptionRef(file_desc_with_id)); - } - None - } -} - -impl VisitProvenance for WeakFileDescriptionRef { - fn visit_provenance(&self, _visit: &mut VisitWith<'_>) { - // A weak reference can never be the only reference to some pointer or place. - // Since the actual file description is tracked by strong ref somewhere, - // it is ok to make this a NOP operation. - } -} - -/// A unique id for file descriptions. While we could use the address, considering that -/// is definitely unique, the address would expose interpreter internal state when used -/// for sorting things. So instead we generate a unique id per file description is the name -/// for all `dup`licates and is never reused. -#[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] -pub struct FdId(usize); - /// The file descriptor table #[derive(Debug)] pub struct FdTable { - pub fds: BTreeMap, + pub fds: BTreeMap, /// Unique identifier for file description, used to differentiate between various file description. next_file_description_id: FdId, } @@ -313,8 +335,9 @@ impl FdTable { fds } - pub fn new_ref(&mut self, fd: impl FileDescription) -> FileDescriptionRef { - let file_handle = FileDescriptionRef::new(fd, self.next_file_description_id); + pub fn new_ref(&mut self, fd: T) -> FileDescriptionRef { + let file_handle = + FileDescriptionRef(Rc::new(FdIdWith { id: self.next_file_description_id, inner: fd })); self.next_file_description_id = FdId(self.next_file_description_id.0.strict_add(1)); file_handle } @@ -325,12 +348,16 @@ impl FdTable { self.insert(fd_ref) } - pub fn insert(&mut self, fd_ref: FileDescriptionRef) -> i32 { + pub fn insert(&mut self, fd_ref: DynFileDescriptionRef) -> i32 { self.insert_with_min_num(fd_ref, 0) } /// Insert a file description, giving it a file descriptor that is at least `min_fd_num`. - pub fn insert_with_min_num(&mut self, file_handle: FileDescriptionRef, min_fd_num: i32) -> i32 { + pub fn insert_with_min_num( + &mut self, + file_handle: DynFileDescriptionRef, + min_fd_num: i32, + ) -> i32 { // Find the lowest unused FD, starting from min_fd. If the first such unused FD is in // between used FDs, the find_map combinator will return it. If the first such unused FD // is after all other used FDs, the find_map combinator will return None, and we will use @@ -356,12 +383,12 @@ impl FdTable { new_fd_num } - pub fn get(&self, fd_num: i32) -> Option { + pub fn get(&self, fd_num: i32) -> Option { let fd = self.fds.get(&fd_num)?; Some(fd.clone()) } - pub fn remove(&mut self, fd_num: i32) -> Option { + pub fn remove(&mut self, fd_num: i32) -> Option { self.fds.remove(&fd_num) } diff --git a/src/shims/unix/fd.rs b/src/shims/unix/fd.rs index e5dead1a26..0b59490308 100644 --- a/src/shims/unix/fd.rs +++ b/src/shims/unix/fd.rs @@ -88,7 +88,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // If old_fd and new_fd point to the same description, then `dup_fd` ensures we keep the underlying file description alive. if let Some(old_new_fd) = this.machine.fds.fds.insert(new_fd_num, fd) { // Ignore close error (not interpreter's) according to dup2() doc. - old_new_fd.close(this.machine.communicate(), this)?.ok(); + old_new_fd.close_ref(this.machine.communicate(), this)?.ok(); } } interp_ok(Scalar::from_i32(new_fd_num)) @@ -122,7 +122,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; let result = fd.as_unix().flock(this.machine.communicate(), parsed_op)?; - drop(fd); // return `0` if flock is successful let result = result.map(|()| 0i32); interp_ok(Scalar::from_i32(this.try_unwrap_io_result(result)?)) @@ -198,7 +197,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Some(fd) = this.machine.fds.remove(fd_num) else { return this.set_last_error_and_return_i32(LibcError("EBADF")); }; - let result = fd.close(this.machine.communicate(), this)?; + let result = fd.close_ref(this.machine.communicate(), this)?; // return `0` if close is successful let result = result.map(|()| 0i32); interp_ok(Scalar::from_i32(this.try_unwrap_io_result(result)?)) @@ -246,7 +245,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(communicate, buf, count, dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); @@ -286,7 +285,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; match offset { - None => fd.write(&fd, communicate, buf, count, dest, this)?, + None => fd.write(communicate, buf, count, dest, this)?, Some(offset) => { let Ok(offset) = u64::try_from(offset) else { return this.set_last_error_and_return(LibcError("EINVAL"), dest); diff --git a/src/shims/unix/fs.rs b/src/shims/unix/fs.rs index 5682fb659e..25594b7803 100644 --- a/src/shims/unix/fs.rs +++ b/src/shims/unix/fs.rs @@ -31,8 +31,7 @@ impl FileDescription for FileHandle { } fn read<'tcx>( - &self, - _self_ref: &FileDescriptionRef, + self: FileDescriptionRef, communicate_allowed: bool, ptr: Pointer, len: usize, @@ -49,8 +48,7 @@ impl FileDescription for FileHandle { } fn write<'tcx>( - &self, - _self_ref: &FileDescriptionRef, + self: FileDescriptionRef, communicate_allowed: bool, ptr: Pointer, len: usize, @@ -76,7 +74,7 @@ impl FileDescription for FileHandle { } fn close<'tcx>( - self: Box, + self, communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -87,7 +85,7 @@ impl FileDescription for FileHandle { // to handle possible errors correctly. let result = self.file.sync_all(); // Now we actually close the file and return the result. - drop(*self); + drop(self.file); interp_ok(result) } else { // We drop the file, this closes it but ignores any errors @@ -96,7 +94,7 @@ impl FileDescription for FileHandle { // `/dev/urandom` which are read-only. Check // https://github.com/rust-lang/miri/issues/999#issuecomment-568920439 // for a deeper discussion. - drop(*self); + drop(self.file); interp_ok(Ok(())) } } @@ -1311,22 +1309,19 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // FIXME: Support ftruncate64 for all FDs - let FileHandle { file, writable } = fd.downcast::().ok_or_else(|| { + let file = fd.downcast::().ok_or_else(|| { err_unsup_format!("`ftruncate64` is only supported on file-backed file descriptors") })?; - if *writable { + if file.writable { if let Ok(length) = length.try_into() { - let result = file.set_len(length); - drop(fd); + let result = file.file.set_len(length); let result = this.try_unwrap_io_result(result.map(|_| 0i32))?; interp_ok(Scalar::from_i32(result)) } else { - drop(fd); this.set_last_error_and_return_i32(LibcError("EINVAL")) } } else { - drop(fd); // The file is not writable this.set_last_error_and_return_i32(LibcError("EINVAL")) } @@ -1358,11 +1353,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return this.set_last_error_and_return_i32(LibcError("EBADF")); }; // Only regular files support synchronization. - let FileHandle { file, writable } = fd.downcast::().ok_or_else(|| { + let file = fd.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); - drop(fd); + let io_result = maybe_sync_file(&file.file, file.writable, File::sync_all); interp_ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?)) } @@ -1382,11 +1376,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return this.set_last_error_and_return_i32(LibcError("EBADF")); }; // Only regular files support synchronization. - let FileHandle { file, writable } = fd.downcast::().ok_or_else(|| { + let file = fd.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); - drop(fd); + let io_result = maybe_sync_file(&file.file, file.writable, File::sync_data); interp_ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?)) } @@ -1425,11 +1418,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return this.set_last_error_and_return_i32(LibcError("EBADF")); }; // Only regular files support synchronization. - let FileHandle { file, writable } = fd.downcast::().ok_or_else(|| { + let file = fd.downcast::().ok_or_else(|| { err_unsup_format!("`sync_data_range` is only supported on file-backed file descriptors") })?; - let io_result = maybe_sync_file(file, *writable, File::sync_data); - drop(fd); + let io_result = maybe_sync_file(&file.file, file.writable, File::sync_data); interp_ok(Scalar::from_i32(this.try_unwrap_io_result(io_result)?)) } diff --git a/src/shims/unix/linux_like/epoll.rs b/src/shims/unix/linux_like/epoll.rs index 5b240351c2..c3f31ee052 100644 --- a/src/shims/unix/linux_like/epoll.rs +++ b/src/shims/unix/linux_like/epoll.rs @@ -5,7 +5,9 @@ use std::rc::{Rc, Weak}; use std::time::Duration; use crate::concurrency::VClock; -use crate::shims::files::{FdId, FileDescription, FileDescriptionRef, WeakFileDescriptionRef}; +use crate::shims::files::{ + DynFileDescriptionRef, FdId, FileDescription, FileDescriptionRef, WeakFileDescriptionRef, +}; use crate::shims::unix::UnixFileDescription; use crate::*; @@ -68,7 +70,7 @@ pub struct EpollEventInterest { /// Ready list of the epoll instance under which this EpollEventInterest is registered. ready_list: Rc, /// The epoll file description that this EpollEventInterest is registered under. - weak_epfd: WeakFileDescriptionRef, + weak_epfd: WeakFileDescriptionRef, } /// EpollReadyEvents reflects the readiness of a file description. @@ -146,7 +148,7 @@ impl FileDescription for Epoll { } fn close<'tcx>( - self: Box, + self, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -281,7 +283,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Some(fd_ref) = this.machine.fds.get(fd) else { return this.set_last_error_and_return_i32(LibcError("EBADF")); }; - let id = fd_ref.get_id(); + let id = fd_ref.id(); if op == epoll_ctl_add || op == epoll_ctl_mod { // Read event bitmask and data from epoll_event passed by caller. @@ -343,7 +345,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { events, data, ready_list: Rc::clone(ready_list), - weak_epfd: epfd.downgrade(), + weak_epfd: FileDescriptionRef::downgrade(&epoll_file_description), })); if op == epoll_ctl_add { @@ -452,18 +454,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let Some(epfd) = this.machine.fds.get(epfd_value) else { return this.set_last_error_and_return(LibcError("EBADF"), dest); }; + let epoll_file_description = epfd + .downcast::() + .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; // Create a weak ref of epfd and pass it to callback so we will make sure that epfd // is not close after the thread unblocks. - let weak_epfd = epfd.downgrade(); + let weak_epfd = FileDescriptionRef::downgrade(&epoll_file_description); // We just need to know if the ready list is empty and borrow the thread_ids out. // The whole logic is wrapped inside a block so we don't need to manually drop epfd later. let ready_list_empty; let mut thread_ids; { - let epoll_file_description = epfd - .downcast::() - .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; ready_list_empty = epoll_file_description.ready_list.mapping.borrow().is_empty(); thread_ids = epoll_file_description.thread_id.borrow_mut(); } @@ -492,7 +494,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { callback!( @capture<'tcx> { epfd_value: i32, - weak_epfd: WeakFileDescriptionRef, + weak_epfd: WeakFileDescriptionRef, dest: MPlaceTy<'tcx>, event: MPlaceTy<'tcx>, } @@ -506,8 +508,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("epoll FD {epfd_value} got closed while blocking.") }; // Remove the current active thread_id from the blocked thread_id list. - epfd.downcast::() - .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))? + epfd .thread_id.borrow_mut() .retain(|&id| id != this.active_thread()); this.write_int(0, &dest)?; @@ -528,17 +529,17 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { /// do not call this function when an FD didn't have anything happen to it! fn check_and_update_readiness( &mut self, - fd_ref: &FileDescriptionRef, + fd_ref: DynFileDescriptionRef, ) -> InterpResult<'tcx, ()> { let this = self.eval_context_mut(); - let id = fd_ref.get_id(); + let id = fd_ref.id(); let mut waiter = Vec::new(); // 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) { for weak_epoll_interest in epoll_interests { if let Some(epoll_interest) = weak_epoll_interest.upgrade() { let is_updated = check_and_update_one_event_interest( - fd_ref, + &fd_ref, epoll_interest.clone(), id, this, @@ -553,10 +554,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // holds a strong ref to epoll_interest. let epfd = epoll_interest.borrow().weak_epfd.upgrade().unwrap(); // FIXME: We can randomly pick a thread to unblock. - - let epoll = epfd.downcast::().unwrap(); - - if let Some(thread_id) = epoll.thread_id.borrow_mut().pop() { + if let Some(thread_id) = epfd.thread_id.borrow_mut().pop() { waiter.push(thread_id); }; } @@ -595,7 +593,7 @@ fn ready_list_next( /// notification was added/updated. Unlike check_and_update_readiness, this function sends a /// notification to only one epoll instance. fn check_and_update_one_event_interest<'tcx>( - fd_ref: &FileDescriptionRef, + fd_ref: &DynFileDescriptionRef, interest: Rc>, id: FdId, ecx: &MiriInterpCx<'tcx>, @@ -628,7 +626,7 @@ fn check_and_update_one_event_interest<'tcx>( /// and the number of returned events into `dest`. fn return_ready_list<'tcx>( epfd_value: i32, - weak_epfd: WeakFileDescriptionRef, + weak_epfd: WeakFileDescriptionRef, dest: &MPlaceTy<'tcx>, events: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, @@ -637,11 +635,7 @@ fn return_ready_list<'tcx>( throw_unsup_format!("epoll FD {epfd_value} got closed while blocking.") }; - let epoll_file_description = epfd - .downcast::() - .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; - - let ready_list = epoll_file_description.get_ready_list(); + let ready_list = epfd.get_ready_list(); let mut ready_list = ready_list.mapping.borrow_mut(); let mut num_of_events: i32 = 0; diff --git a/src/shims/unix/linux_like/eventfd.rs b/src/shims/unix/linux_like/eventfd.rs index ed81207f54..3e5d8869b9 100644 --- a/src/shims/unix/linux_like/eventfd.rs +++ b/src/shims/unix/linux_like/eventfd.rs @@ -20,7 +20,7 @@ const MAX_COUNTER: u64 = u64::MAX - 1; /// /// #[derive(Debug)] -struct Event { +struct EventFd { /// 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: Cell, @@ -32,13 +32,13 @@ struct Event { blocked_write_tid: RefCell>, } -impl FileDescription for Event { +impl FileDescription for EventFd { fn name(&self) -> &'static str { "event" } fn close<'tcx>( - self: Box, + self, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -47,8 +47,7 @@ impl FileDescription for Event { /// Read the counter in the buffer and return the counter if succeeded. fn read<'tcx>( - &self, - self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, len: usize, @@ -65,7 +64,7 @@ impl FileDescription for Event { // Turn the pointer into a place at the right type. let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ty); - eventfd_read(buf_place, dest, self_ref, ecx) + eventfd_read(buf_place, dest, self, ecx) } /// A write call adds the 8-byte integer value supplied in @@ -81,8 +80,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>( - &self, - self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, len: usize, @@ -99,7 +97,7 @@ impl FileDescription for Event { // Turn the pointer into a place at the right type. let buf_place = ecx.ptr_to_mplace_unaligned(ptr, ty); - eventfd_write(buf_place, dest, self_ref, ecx) + eventfd_write(buf_place, dest, self, ecx) } fn as_unix(&self) -> &dyn UnixFileDescription { @@ -107,7 +105,7 @@ impl FileDescription for Event { } } -impl UnixFileDescription for Event { +impl UnixFileDescription for EventFd { fn get_epoll_ready_events<'tcx>(&self) -> InterpResult<'tcx, EpollReadyEvents> { // We only check the status of EPOLLIN and EPOLLOUT flags for eventfd. If other event flags // need to be supported in the future, the check should be added here. @@ -169,7 +167,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let fds = &mut this.machine.fds; - let fd_value = fds.insert_new(Event { + let fd_value = fds.insert_new(EventFd { counter: Cell::new(val.into()), is_nonblock, clock: RefCell::new(VClock::default()), @@ -186,13 +184,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn eventfd_write<'tcx>( buf_place: MPlaceTy<'tcx>, dest: &MPlaceTy<'tcx>, - eventfd_ref: &FileDescriptionRef, + eventfd: FileDescriptionRef, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - // Since we pass the weak file description ref, it is guaranteed to be - // an eventfd file description. - let eventfd = eventfd_ref.downcast::().unwrap(); - // Figure out which value we should add. 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. @@ -210,10 +204,6 @@ fn eventfd_write<'tcx>( // Store new counter value. eventfd.counter.set(new_count); - // The state changed; we check and update the status of all supported event - // types for current file description. - ecx.check_and_update_readiness(eventfd_ref)?; - // Unblock *all* threads previously blocked on `read`. // We need to take out the blocked thread ids and unblock them together, // because `unblock_threads` may block them again and end up re-adding the @@ -224,6 +214,10 @@ fn eventfd_write<'tcx>( ecx.unblock_thread(thread_id, BlockReason::Eventfd)?; } + // The state changed; we check and update the status of all supported event + // types for current file description. + ecx.check_and_update_readiness(eventfd)?; + // Return how many bytes we consumed from the user-provided buffer. return ecx.write_int(buf_place.layout.size.bytes(), dest); } @@ -237,7 +231,7 @@ fn eventfd_write<'tcx>( eventfd.blocked_write_tid.borrow_mut().push(ecx.active_thread()); - let weak_eventfd = eventfd_ref.downgrade(); + let weak_eventfd = FileDescriptionRef::downgrade(&eventfd); ecx.block_thread( BlockReason::Eventfd, None, @@ -246,13 +240,13 @@ fn eventfd_write<'tcx>( num: u64, buf_place: MPlaceTy<'tcx>, dest: MPlaceTy<'tcx>, - weak_eventfd: WeakFileDescriptionRef, + weak_eventfd: WeakFileDescriptionRef, } @unblock = |this| { // When we get unblocked, try again. We know the ref is still valid, // otherwise there couldn't be a `write` that unblocks us. let eventfd_ref = weak_eventfd.upgrade().unwrap(); - eventfd_write(buf_place, &dest, &eventfd_ref, this) + eventfd_write(buf_place, &dest, eventfd_ref, this) } ), ); @@ -266,13 +260,9 @@ fn eventfd_write<'tcx>( fn eventfd_read<'tcx>( buf_place: MPlaceTy<'tcx>, dest: &MPlaceTy<'tcx>, - eventfd_ref: &FileDescriptionRef, + eventfd: FileDescriptionRef, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - // Since we pass the weak file description ref to the callback function, it is guaranteed to be - // an eventfd file description. - let eventfd = eventfd_ref.downcast::().unwrap(); - // Set counter to 0, get old value. let counter = eventfd.counter.replace(0); @@ -285,7 +275,7 @@ fn eventfd_read<'tcx>( eventfd.blocked_read_tid.borrow_mut().push(ecx.active_thread()); - let weak_eventfd = eventfd_ref.downgrade(); + let weak_eventfd = FileDescriptionRef::downgrade(&eventfd); ecx.block_thread( BlockReason::Eventfd, None, @@ -293,13 +283,13 @@ fn eventfd_read<'tcx>( @capture<'tcx> { buf_place: MPlaceTy<'tcx>, dest: MPlaceTy<'tcx>, - weak_eventfd: WeakFileDescriptionRef, + weak_eventfd: WeakFileDescriptionRef, } @unblock = |this| { // When we get unblocked, try again. We know the ref is still valid, // otherwise there couldn't be a `write` that unblocks us. let eventfd_ref = weak_eventfd.upgrade().unwrap(); - eventfd_read(buf_place, &dest, &eventfd_ref, this) + eventfd_read(buf_place, &dest, eventfd_ref, this) } ), ); @@ -310,10 +300,6 @@ fn eventfd_read<'tcx>( // Return old counter value into user-space buffer. ecx.write_int(counter, &buf_place)?; - // The state changed; we check and update the status of all supported event - // types for current file description. - ecx.check_and_update_readiness(eventfd_ref)?; - // Unblock *all* threads previously blocked on `write`. // We need to take out the blocked thread ids and unblock them together, // because `unblock_threads` may block them again and end up re-adding the @@ -324,6 +310,10 @@ fn eventfd_read<'tcx>( ecx.unblock_thread(thread_id, BlockReason::Eventfd)?; } + // The state changed; we check and update the status of all supported event + // types for current file description. + ecx.check_and_update_readiness(eventfd)?; + // Tell userspace how many bytes we put into the buffer. return ecx.write_int(buf_place.layout.size.bytes(), dest); } diff --git a/src/shims/unix/unnamed_socket.rs b/src/shims/unix/unnamed_socket.rs index 4285786f06..c6eeb72f14 100644 --- a/src/shims/unix/unnamed_socket.rs +++ b/src/shims/unix/unnamed_socket.rs @@ -31,7 +31,7 @@ struct AnonSocket { /// The `AnonSocket` 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, + peer_fd: OnceCell>, /// Indicates whether the peer has lost data when the file description is closed. /// This flag is set to `true` if the peer's `readbuf` is non-empty at the time /// of closure. @@ -58,7 +58,7 @@ impl Buffer { } impl AnonSocket { - fn peer_fd(&self) -> &WeakFileDescriptionRef { + fn peer_fd(&self) -> &WeakFileDescriptionRef { self.peer_fd.get().unwrap() } } @@ -69,7 +69,7 @@ impl FileDescription for AnonSocket { } fn close<'tcx>( - self: Box, + self, _communicate_allowed: bool, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -78,37 +78,35 @@ impl FileDescription for AnonSocket { // notify the peer that data lost has happened in current file description. if let Some(readbuf) = &self.readbuf { if !readbuf.borrow().buf.is_empty() { - peer_fd.downcast::().unwrap().peer_lost_data.set(true); + peer_fd.peer_lost_data.set(true); } } // Notify peer fd that close has happened, since that can unblock reads and writes. - ecx.check_and_update_readiness(&peer_fd)?; + ecx.check_and_update_readiness(peer_fd)?; } interp_ok(Ok(())) } fn read<'tcx>( - &self, - self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - anonsocket_read(self_ref, len, ptr, dest, ecx) + anonsocket_read(self, len, ptr, dest, ecx) } fn write<'tcx>( - &self, - self_ref: &FileDescriptionRef, + self: FileDescriptionRef, _communicate_allowed: bool, ptr: Pointer, len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - anonsocket_write(self_ref, ptr, len, dest, ecx) + anonsocket_write(self, ptr, len, dest, ecx) } fn as_unix(&self) -> &dyn UnixFileDescription { @@ -118,14 +116,12 @@ impl FileDescription for AnonSocket { /// Write to AnonSocket based on the space available and return the written byte size. fn anonsocket_write<'tcx>( - self_ref: &FileDescriptionRef, + self_ref: FileDescriptionRef, ptr: Pointer, len: usize, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let self_anonsocket = self_ref.downcast::().unwrap(); - // 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 len == 0 { @@ -133,13 +129,13 @@ fn anonsocket_write<'tcx>( } // We are writing to our peer's readbuf. - let Some(peer_fd) = self_anonsocket.peer_fd().upgrade() else { + let Some(peer_fd) = self_ref.peer_fd().upgrade() else { // If the upgrade from Weak to Rc fails, it indicates that all read ends have been // closed. It is an error to write even if there would be space. return ecx.set_last_error_and_return(ErrorKind::BrokenPipe, dest); }; - let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf else { + let Some(writebuf) = &peer_fd.readbuf else { // Writing to the read end of a pipe. return ecx.set_last_error_and_return(IoError::LibcError("EBADF"), dest); }; @@ -147,21 +143,21 @@ fn anonsocket_write<'tcx>( // Let's see if we can write. let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(writebuf.borrow().buf.len()); if available_space == 0 { - if self_anonsocket.is_nonblock { + if self_ref.is_nonblock { // Non-blocking socketpair with a full buffer. return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); } else { + self_ref.blocked_write_tid.borrow_mut().push(ecx.active_thread()); // Blocking socketpair with a full buffer. // Block the current thread; only keep a weak ref for this. - let weak_self_ref = self_ref.downgrade(); + let weak_self_ref = FileDescriptionRef::downgrade(&self_ref); let dest = dest.clone(); - self_anonsocket.blocked_write_tid.borrow_mut().push(ecx.active_thread()); ecx.block_thread( BlockReason::UnnamedSocket, None, callback!( @capture<'tcx> { - weak_self_ref: WeakFileDescriptionRef, + weak_self_ref: WeakFileDescriptionRef, ptr: Pointer, len: usize, dest: MPlaceTy<'tcx>, @@ -170,7 +166,7 @@ fn anonsocket_write<'tcx>( // If we got unblocked, then our peer successfully upgraded its weak // ref to us. That means we can also upgrade our weak ref. let self_ref = weak_self_ref.upgrade().unwrap(); - anonsocket_write(&self_ref, ptr, len, &dest, this) + anonsocket_write(self_ref, ptr, len, &dest, this) } ), ); @@ -190,16 +186,15 @@ fn anonsocket_write<'tcx>( // 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. - // The kernel does this even if the fd was already readable before, so we follow suit. - ecx.check_and_update_readiness(&peer_fd)?; - let peer_anonsocket = peer_fd.downcast::().unwrap(); // Unblock all threads that are currently blocked on peer_fd's read. - let waiting_threads = std::mem::take(&mut *peer_anonsocket.blocked_read_tid.borrow_mut()); + let waiting_threads = std::mem::take(&mut *peer_fd.blocked_read_tid.borrow_mut()); // FIXME: We can randomize the order of unblocking. for thread_id in waiting_threads { ecx.unblock_thread(thread_id, BlockReason::UnnamedSocket)?; } + // 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 ecx.return_write_success(actual_write_size, dest); } @@ -208,31 +203,29 @@ fn anonsocket_write<'tcx>( /// Read from AnonSocket and return the number of bytes read. fn anonsocket_read<'tcx>( - self_ref: &FileDescriptionRef, + self_ref: FileDescriptionRef, len: usize, ptr: Pointer, dest: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let self_anonsocket = self_ref.downcast::().unwrap(); - // Always succeed on read size 0. if len == 0 { return ecx.return_read_success(ptr, &[], 0, dest); } - let Some(readbuf) = &self_anonsocket.readbuf else { + let Some(readbuf) = &self_ref.readbuf else { // FIXME: This should return EBADF, but there's no nice way to do that as there's no // corresponding ErrorKind variant. throw_unsup_format!("reading from the write end of a pipe") }; if readbuf.borrow_mut().buf.is_empty() { - if self_anonsocket.peer_fd().upgrade().is_none() { + if self_ref.peer_fd().upgrade().is_none() { // Socketpair with no peer and empty buffer. // 0 bytes successfully read indicates end-of-file. return ecx.return_read_success(ptr, &[], 0, dest); - } else if self_anonsocket.is_nonblock { + } else if self_ref.is_nonblock { // Non-blocking socketpair with writer and empty buffer. // https://linux.die.net/man/2/read // EAGAIN or EWOULDBLOCK can be returned for socket, @@ -240,17 +233,17 @@ fn anonsocket_read<'tcx>( // Since there is no ErrorKind for EAGAIN, WouldBlock is used. return ecx.set_last_error_and_return(ErrorKind::WouldBlock, dest); } else { + self_ref.blocked_read_tid.borrow_mut().push(ecx.active_thread()); // Blocking socketpair with writer and empty buffer. // Block the current thread; only keep a weak ref for this. - let weak_self_ref = self_ref.downgrade(); + let weak_self_ref = FileDescriptionRef::downgrade(&self_ref); let dest = dest.clone(); - self_anonsocket.blocked_read_tid.borrow_mut().push(ecx.active_thread()); ecx.block_thread( BlockReason::UnnamedSocket, None, callback!( @capture<'tcx> { - weak_self_ref: WeakFileDescriptionRef, + weak_self_ref: WeakFileDescriptionRef, len: usize, ptr: Pointer, dest: MPlaceTy<'tcx>, @@ -259,7 +252,7 @@ fn anonsocket_read<'tcx>( // If we got unblocked, then our peer successfully upgraded its weak // ref to us. That means we can also upgrade our weak ref. let self_ref = weak_self_ref.upgrade().unwrap(); - anonsocket_read(&self_ref, len, ptr, &dest, this) + anonsocket_read(self_ref, len, ptr, &dest, this) } ), ); @@ -287,16 +280,15 @@ fn anonsocket_read<'tcx>( // 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_anonsocket.peer_fd().upgrade() { - ecx.check_and_update_readiness(&peer_fd)?; - let peer_anonsocket = peer_fd.downcast::().unwrap(); + if let Some(peer_fd) = self_ref.peer_fd().upgrade() { // Unblock all threads that are currently blocked on peer_fd's write. - let waiting_threads = - std::mem::take(&mut *peer_anonsocket.blocked_write_tid.borrow_mut()); + let waiting_threads = std::mem::take(&mut *peer_fd.blocked_write_tid.borrow_mut()); // FIXME: We can randomize the order of unblocking. for thread_id in waiting_threads { ecx.unblock_thread(thread_id, BlockReason::UnnamedSocket)?; } + // Notify epoll waiters. + ecx.check_and_update_readiness(peer_fd)?; }; return ecx.return_read_success(ptr, &bytes, actual_read_size, dest); @@ -323,7 +315,7 @@ impl UnixFileDescription for AnonSocket { // Check if is writable. if let Some(peer_fd) = self.peer_fd().upgrade() { - if let Some(writebuf) = &peer_fd.downcast::().unwrap().readbuf { + if let Some(writebuf) = &peer_fd.readbuf { let data_size = writebuf.borrow().buf.len(); let available_space = MAX_SOCKETPAIR_BUFFER_CAPACITY.strict_sub(data_size); if available_space != 0 { @@ -429,8 +421,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }); // 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(); + fd0.peer_fd.set(FileDescriptionRef::downgrade(&fd1)).unwrap(); + fd1.peer_fd.set(FileDescriptionRef::downgrade(&fd0)).unwrap(); // Insert the file description to the fd table, generating the file descriptors. let sv0 = fds.insert(fd0); @@ -497,8 +489,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }); // 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(); + fd0.peer_fd.set(FileDescriptionRef::downgrade(&fd1)).unwrap(); + fd1.peer_fd.set(FileDescriptionRef::downgrade(&fd0)).unwrap(); // Insert the file description to the fd table, generating the file descriptors. let pipefd0 = fds.insert(fd0);