diff --git a/src/shims/files.rs b/src/shims/files.rs index 8c29cb040b..ea19c2d501 100644 --- a/src/shims/files.rs +++ b/src/shims/files.rs @@ -14,7 +14,7 @@ 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 sorting things. So instead we generate a unique id per file description which is the same /// for all `dup`licates and is never reused. #[derive(Debug, Copy, Clone, Default, Eq, PartialEq, Ord, PartialOrd)] pub struct FdId(usize); @@ -48,6 +48,12 @@ impl FileDescriptionRef { pub fn id(&self) -> FdId { self.0.id } + + /// Returns the raw address of this file description. Useful for equality comparisons. + /// Use `id` instead if this can affect user-visible behavior! + pub fn addr(&self) -> usize { + Rc::as_ptr(&self.0).addr() + } } /// Holds a weak reference to the actual file description. @@ -70,6 +76,11 @@ impl WeakFileDescriptionRef { pub fn upgrade(&self) -> Option> { self.0.upgrade().map(FileDescriptionRef) } + + /// Returns the raw address of this file description. Useful for equality comparisons. + pub fn addr(&self) -> usize { + self.0.as_ptr().addr() + } } impl VisitProvenance for WeakFileDescriptionRef { @@ -105,12 +116,13 @@ impl FileDescriptionExt for T { communicate_allowed: bool, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { + let addr = self.addr(); match Rc::into_inner(self.0) { Some(fd) => { - // Remove entry from the global epoll_event_interest table. - ecx.machine.epoll_interests.remove(fd.id); + // There might have been epolls interested in this FD. Remove that. + ecx.machine.epoll_interests.remove_epolls(fd.id); - fd.inner.close(communicate_allowed, ecx) + fd.inner.destroy(addr, communicate_allowed, ecx) } None => { // Not the last reference. @@ -183,9 +195,12 @@ pub trait FileDescription: std::fmt::Debug + FileDescriptionExt { throw_unsup_format!("cannot seek on {}", self.name()); } - /// Close the file descriptor. - fn close<'tcx>( + /// Destroys the file description. Only called when the last duplicate file descriptor is closed. + /// + /// `self_addr` is the address that this file description used to be stored at. + fn destroy<'tcx>( self, + _self_addr: usize, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> @@ -362,8 +377,9 @@ impl FileDescription for FileHandle { interp_ok((&mut &self.file).seek(offset)) } - fn close<'tcx>( + fn destroy<'tcx>( self, + _self_addr: usize, communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { diff --git a/src/shims/unix/linux_like/epoll.rs b/src/shims/unix/linux_like/epoll.rs index 3133c14929..d832d10782 100644 --- a/src/shims/unix/linux_like/epoll.rs +++ b/src/shims/unix/linux_like/epoll.rs @@ -1,28 +1,33 @@ use std::cell::RefCell; use std::collections::BTreeMap; use std::io; -use std::rc::{Rc, Weak}; use std::time::Duration; use rustc_abi::FieldIdx; use crate::concurrency::VClock; use crate::shims::files::{ - DynFileDescriptionRef, FdId, FileDescription, FileDescriptionRef, WeakFileDescriptionRef, + DynFileDescriptionRef, FdId, FdNum, FileDescription, FileDescriptionRef, WeakFileDescriptionRef, }; use crate::shims::unix::UnixFileDescription; use crate::*; +type EpollEventKey = (FdId, FdNum); + /// An `Epoll` file descriptor connects file handles and epoll events #[derive(Debug, Default)] struct Epoll { /// A map of EpollEventInterests registered under this epoll instance. /// Each entry is differentiated using FdId and file descriptor value. - interest_list: RefCell>>>, + 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. - ready_list: ReadyList, + /// We keep this separate from `interest_list` for two reasons: there might be many + /// interests but only a few of them ready (so with a separate list it is more efficient + /// to find a ready event), and having separate `RefCell` lets us mutate the `interest_list` + /// while unblocking threads which might mutate the `ready_list`. + ready_list: RefCell>, /// A list of thread ids blocked on this epoll instance. blocked_tid: RefCell>, } @@ -33,12 +38,17 @@ impl VisitProvenance for Epoll { } } +/// Returns the range of all EpollEventKey for the given FD ID. +fn range_for_id(id: FdId) -> std::ops::RangeInclusive { + (id, 0)..=(id, i32::MAX) +} + /// EpollEventInstance contains information that will be returned by epoll_wait. #[derive(Debug)] pub struct EpollEventInstance { - /// Xor-ed event types that happened to the file description. + /// Bitmask of event types that happened to the file description. events: u32, - /// Original data retrieved from `epoll_event` during `epoll_ctl`. + /// User-defined data associated with the interest that triggered this instance. data: u64, /// The release clock associated with this event. clock: VClock, @@ -59,13 +69,8 @@ impl EpollEventInstance { /// see the man page: /// /// -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] pub struct EpollEventInterest { - /// The file descriptor value of the file description registered. - /// This is only used for ready_list, to inform userspace which FD triggered an event. - /// For that, it is crucial to preserve the original FD number. - /// This FD number must never be "dereferenced" to a file description inside Miri. - fd_num: i32, /// The events bitmask retrieved from `epoll_event`. events: u32, /// The data retrieved from `epoll_event`. @@ -73,13 +78,10 @@ pub struct EpollEventInterest { /// but only u64 is supported for now. /// data: u64, - /// The epoll file description that this EpollEventInterest is registered under. - /// This is weak to avoid cycles, but an upgrade is always guaranteed to succeed - /// because only the `Epoll` holds a strong ref to a `EpollEventInterest`. - weak_epfd: WeakFileDescriptionRef, } /// EpollReadyEvents reflects the readiness of a file description. +#[derive(Debug)] pub struct EpollReadyEvents { /// The associated file is available for read(2) operations, in the sense that a read will not block. /// (I.e., returning EOF is considered "ready".) @@ -99,11 +101,6 @@ pub struct EpollReadyEvents { pub epollerr: bool, } -#[derive(Debug, Default)] -struct ReadyList { - mapping: RefCell>, -} - impl EpollReadyEvents { pub fn new() -> Self { EpollReadyEvents { @@ -147,11 +144,18 @@ impl FileDescription for Epoll { "epoll" } - fn close<'tcx>( - self, + fn destroy<'tcx>( + mut self, + self_addr: usize, _communicate_allowed: bool, - _ecx: &mut MiriInterpCx<'tcx>, + ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { + // If we were interested in some FDs, we can remove that now. + let mut ids = self.interest_list.get_mut().keys().map(|(id, _num)| *id).collect::>(); + ids.dedup(); // they come out of the map sorted + for id in ids { + ecx.machine.epoll_interests.remove(id, self_addr); + } interp_ok(Ok(())) } @@ -163,41 +167,54 @@ impl FileDescription for Epoll { impl UnixFileDescription for Epoll {} /// The table of all EpollEventInterest. -/// The BTreeMap key is the FdId of an active file description registered with -/// any epoll instance. The value is a list of EpollEventInterest associated -/// with that file description. -pub struct EpollInterestTable(BTreeMap>>>); +/// This tracks, for each file description, which epoll instances have an interest in events +/// for this file description. +pub struct EpollInterestTable(BTreeMap>>); impl EpollInterestTable { pub(crate) fn new() -> Self { EpollInterestTable(BTreeMap::new()) } - pub fn insert_epoll_interest(&mut self, id: FdId, fd: Weak>) { - match self.0.get_mut(&id) { - Some(fds) => { - fds.push(fd); - } - None => { - let vec = vec![fd]; - self.0.insert(id, vec); - } - } + fn insert(&mut self, id: FdId, epoll: WeakFileDescriptionRef) { + let epolls = self.0.entry(id).or_default(); + epolls.push(epoll); } - pub fn get_epoll_interest(&self, id: FdId) -> Option<&Vec>>> { - self.0.get(&id) + fn remove(&mut self, id: FdId, epoll_addr: usize) { + let epolls = self.0.entry(id).or_default(); + // FIXME: linear scan. Keep the list sorted so we can do binary search? + let idx = epolls + .iter() + .position(|old_ref| old_ref.addr() == epoll_addr) + .expect("trying to remove an epoll that's not in the list"); + epolls.remove(idx); } - pub fn get_epoll_interest_mut( - &mut self, - id: FdId, - ) -> Option<&mut Vec>>> { - self.0.get_mut(&id) + fn get_epolls(&self, id: FdId) -> Option<&Vec>> { + self.0.get(&id) } - pub fn remove(&mut self, id: FdId) { - self.0.remove(&id); + pub fn remove_epolls(&mut self, id: FdId) { + if let Some(epolls) = self.0.remove(&id) { + for epoll in epolls.iter().filter_map(|e| e.upgrade()) { + // This is a still-live epoll with interest in this FD. Remove all + // relevent interests. + epoll + .interest_list + .borrow_mut() + .extract_if(range_for_id(id), |_, _| true) + // Consume the iterator. + .for_each(|_| ()); + // Also remove all events from the ready list that refer to this FD. + epoll + .ready_list + .borrow_mut() + .extract_if(range_for_id(id), |_, _| true) + // Consume the iterator. + .for_each(|_| ()); + } + } } } @@ -264,9 +281,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let epollhup = this.eval_libc_u32("EPOLLHUP"); let epollerr = this.eval_libc_u32("EPOLLERR"); - // Throw EINVAL if epfd and fd have the same value. + // Throw EFAULT if epfd and fd have the same value. if epfd_value == fd { - return this.set_last_error_and_return_i32(LibcError("EINVAL")); + return this.set_last_error_and_return_i32(LibcError("EFAULT")); } // Check if epfd is a valid epoll file descriptor. @@ -326,69 +343,52 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } + // Add new interest to list. let epoll_key = (id, fd); - - // Check the existence of fd in the interest list. + let new_interest = EpollEventInterest { events, data }; if op == epoll_ctl_add { - if interest_list.contains_key(&epoll_key) { + if interest_list.range(range_for_id(id)).next().is_none() { + // This is the first time this FD got added to this epoll. + // Remember that in the global list so we get notified about FD events. + this.machine.epoll_interests.insert(id, FileDescriptionRef::downgrade(&epfd)); + } + if interest_list.insert(epoll_key, new_interest).is_some() { + // We already had interest in this. return this.set_last_error_and_return_i32(LibcError("EEXIST")); } } else { - if !interest_list.contains_key(&epoll_key) { + // Modify the existing interest. + let Some(interest) = interest_list.get_mut(&epoll_key) else { return this.set_last_error_and_return_i32(LibcError("ENOENT")); - } - } + }; + *interest = new_interest; + }; - if op == epoll_ctl_add { - // Create an epoll_interest. - let interest = Rc::new(RefCell::new(EpollEventInterest { - fd_num: fd, - events, - data, - weak_epfd: FileDescriptionRef::downgrade(&epfd), - })); - // Notification will be returned for current epfd if there is event in the file - // descriptor we registered. - check_and_update_one_event_interest(&fd_ref, &interest, id, this)?; - - // Insert an epoll_interest to global epoll_interest list. - this.machine.epoll_interests.insert_epoll_interest(id, Rc::downgrade(&interest)); - interest_list.insert(epoll_key, interest); - } else { - // Modify the existing interest. - let epoll_interest = interest_list.get_mut(&epoll_key).unwrap(); - { - let mut epoll_interest = epoll_interest.borrow_mut(); - epoll_interest.events = events; - epoll_interest.data = data; - } - // Updating an FD interest triggers events. - check_and_update_one_event_interest(&fd_ref, epoll_interest, id, this)?; - } + // Deliver events for the new interest. + send_ready_events_to_interests( + this, + &epfd, + fd_ref.as_unix(this).get_epoll_ready_events()?.get_event_bitmask(this), + std::iter::once((&epoll_key, &new_interest)), + )?; interp_ok(Scalar::from_i32(0)) } else if op == epoll_ctl_del { let epoll_key = (id, fd); // Remove epoll_event_interest from interest_list. - let Some(epoll_interest) = interest_list.remove(&epoll_key) else { + if interest_list.remove(&epoll_key).is_none() { + // We did not have interest in this. return this.set_last_error_and_return_i32(LibcError("ENOENT")); }; - // All related Weak will fail to upgrade after the drop. - drop(epoll_interest); + // If this was the last interest in this FD, remove us from the global list + // of who is interested in this FD. + if interest_list.range(range_for_id(id)).next().is_none() { + this.machine.epoll_interests.remove(id, epfd.addr()); + } // Remove related epoll_interest from ready list. - epfd.ready_list.mapping.borrow_mut().remove(&epoll_key); - - // Remove dangling EpollEventInterest from its global table. - // .unwrap() below should succeed because the file description id must have registered - // at least one epoll_interest, if not, it will fail when removing epoll_interest from - // interest list. - this.machine - .epoll_interests - .get_epoll_interest_mut(id) - .unwrap() - .retain(|event| event.upgrade().is_some()); + epfd.ready_list.borrow_mut().remove(&epoll_key); interp_ok(Scalar::from_i32(0)) } else { @@ -462,7 +462,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; // We just need to know if the ready list is empty and borrow the thread_ids out. - let ready_list_empty = epfd.ready_list.mapping.borrow().is_empty(); + let ready_list_empty = epfd.ready_list.borrow().is_empty(); if timeout == 0 || !ready_list_empty { // If the ready list is not empty, or the timeout is 0, we can return immediately. return_ready_list(&epfd, dest, &event, this)?; @@ -518,103 +518,80 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { interp_ok(()) } - /// 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. + /// For a specific file description, get its ready events and send it to everyone who registered + /// interest in this FD. 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( - &mut self, - fd_ref: DynFileDescriptionRef, - ) -> InterpResult<'tcx, ()> { + fn epoll_send_fd_ready_events(&mut self, fd_ref: DynFileDescriptionRef) -> InterpResult<'tcx> { let this = self.eval_context_mut(); 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, &epoll_interest, id, this)?; - if is_updated { - // Edge-triggered notification only notify one thread even if there are - // multiple threads blocked on the same epfd. - - // This unwrap can never fail because if the current epoll instance were - // closed, the upgrade of weak_epoll_interest - // above would fail. This guarantee holds because only the epoll instance - // 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. - if let Some(thread_id) = epfd.blocked_tid.borrow_mut().pop() { - waiter.push(thread_id); - }; - } - } - } - } - waiter.sort(); - waiter.dedup(); - for thread_id in waiter { - this.unblock_thread(thread_id, BlockReason::Epoll)?; + // Figure out who is interested in this. We need to clone this list since we can't prove + // that `send_ready_events_to_interest` won't mutate it. + let Some(epolls) = this.machine.epoll_interests.get_epolls(id) else { + return interp_ok(()); + }; + let epolls = epolls + .iter() + .map(|weak| { + weak.upgrade() + .expect("someone forgot to remove the garbage from `machine.epoll_interests`") + }) + .collect::>(); + let event_bitmask = fd_ref.as_unix(this).get_epoll_ready_events()?.get_event_bitmask(this); + for epoll in epolls { + send_ready_events_to_interests( + this, + &epoll, + event_bitmask, + epoll.interest_list.borrow().range(range_for_id(id)), + )?; } + interp_ok(()) } } -/// This function takes in ready list and returns EpollEventInstance with file description -/// that is not closed. -fn ready_list_next( - ecx: &MiriInterpCx<'_>, - ready_list: &mut BTreeMap<(FdId, i32), EpollEventInstance>, -) -> Option { - while let Some((epoll_key, epoll_event_instance)) = ready_list.pop_first() { - // This ensures that we only return events that we are interested. The FD might have been closed since - // the event was generated, in which case we are not interested anymore. - // When a file description is fully closed, it gets removed from `machine.epoll_interests`, - // so we skip events whose FD is not in that map anymore. - if ecx.machine.epoll_interests.get_epoll_interest(epoll_key.0).is_some() { - return Some(epoll_event_instance); +/// Send the latest ready events for one particular FD (identified by `event_key`) to everyone in +/// the `interests` list, if they are interested in this kind of event. +fn send_ready_events_to_interests<'tcx, 'a>( + ecx: &mut MiriInterpCx<'tcx>, + epoll: &Epoll, + event_bitmask: u32, + interests: impl Iterator, +) -> InterpResult<'tcx> { + let mut wakeup = false; + for (&event_key, interest) in interests { + // This checks if any of the events specified in epoll_event_interest.events + // match those in ready_events. + let flags = interest.events & event_bitmask; + if flags == 0 { + continue; } - } - None -} - -/// This helper function checks whether an epoll notification should be triggered for a specific -/// epoll_interest and, if necessary, triggers the notification, and returns whether the -/// 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: &DynFileDescriptionRef, - interest: &RefCell, - id: FdId, - ecx: &MiriInterpCx<'tcx>, -) -> InterpResult<'tcx, bool> { - // Get the bitmask of ready events for a file description. - let ready_events_bitmask = fd_ref.as_unix(ecx).get_epoll_ready_events()?.get_event_bitmask(ecx); - let epoll_event_interest = interest.borrow(); - let epfd = epoll_event_interest.weak_epfd.upgrade().unwrap(); - // This checks if any of the events specified in epoll_event_interest.events - // match those in ready_events. - let flags = epoll_event_interest.events & ready_events_bitmask; - // If there is any event that we are interested in being specified as ready, - // insert an epoll_return to the ready list. - if flags != 0 { - let epoll_key = (id, epoll_event_interest.fd_num); - let mut ready_list = epfd.ready_list.mapping.borrow_mut(); - let mut event_instance = EpollEventInstance::new(flags, epoll_event_interest.data); - // If we are tracking data races, remember the current clock so we can sync with it later. + // Geenrate a new event instance, with the flags that this one is interested in. + let mut new_instance = EpollEventInstance::new(flags, interest.data); ecx.release_clock(|clock| { - event_instance.clock.clone_from(clock); + new_instance.clock.clone_from(clock); })?; - // Triggers the notification by inserting it to the ready list. - ready_list.insert(epoll_key, event_instance); - interp_ok(true) - } else { - interp_ok(false) + // Add event to ready list for this epoll instance. + // Tests confirm that we have to *overwrite* the old instance for the same key. + let mut ready_list = epoll.ready_list.borrow_mut(); + ready_list.insert(event_key, new_instance); + wakeup = true; + } + if wakeup { + // Wake up threads that may have been waiting for events on this epoll. + // Do this only once for all the interests. + // Edge-triggered notification only notify one thread even if there are + // multiple threads blocked on the same epoll. + if let Some(thread_id) = epoll.blocked_tid.borrow_mut().pop() { + ecx.unblock_thread(thread_id, BlockReason::Epoll)?; + } } + + interp_ok(()) } /// Stores the ready list of the `epfd` epoll instance into `events` (which must be an array), @@ -625,12 +602,12 @@ fn return_ready_list<'tcx>( events: &MPlaceTy<'tcx>, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx> { - let mut ready_list = epfd.ready_list.mapping.borrow_mut(); + let mut ready_list = epfd.ready_list.borrow_mut(); let mut num_of_events: i32 = 0; let mut array_iter = ecx.project_array_fields(events)?; while let Some(des) = array_iter.next(ecx)? { - if let Some(epoll_event_instance) = ready_list_next(ecx, &mut ready_list) { + if let Some((_, epoll_event_instance)) = ready_list.pop_first() { ecx.write_int_fields_named( &[ ("events", epoll_event_instance.events.into()), diff --git a/src/shims/unix/linux_like/eventfd.rs b/src/shims/unix/linux_like/eventfd.rs index 5d4f207d36..ce51f25793 100644 --- a/src/shims/unix/linux_like/eventfd.rs +++ b/src/shims/unix/linux_like/eventfd.rs @@ -37,8 +37,9 @@ impl FileDescription for EventFd { "event" } - fn close<'tcx>( + fn destroy<'tcx>( self, + _self_addr: usize, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -216,7 +217,7 @@ fn eventfd_write<'tcx>( // The state changed; we check and update the status of all supported event // types for current file description. - ecx.check_and_update_readiness(eventfd)?; + ecx.epoll_send_fd_ready_events(eventfd)?; // Return how many bytes we consumed from the user-provided buffer. return finish.call(ecx, Ok(buf_place.layout.size.bytes_usize())); @@ -311,7 +312,7 @@ fn eventfd_read<'tcx>( // The state changed; we check and update the status of all supported event // types for current file description. - ecx.check_and_update_readiness(eventfd)?; + ecx.epoll_send_fd_ready_events(eventfd)?; // Tell userspace how many bytes we put into the buffer. return finish.call(ecx, Ok(buf_place.layout.size.bytes_usize())); diff --git a/src/shims/unix/unnamed_socket.rs b/src/shims/unix/unnamed_socket.rs index 81703d6e17..cfd453df26 100644 --- a/src/shims/unix/unnamed_socket.rs +++ b/src/shims/unix/unnamed_socket.rs @@ -82,8 +82,9 @@ impl FileDescription for AnonSocket { } } - fn close<'tcx>( + fn destroy<'tcx>( self, + _self_addr: usize, _communicate_allowed: bool, ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -96,7 +97,7 @@ impl FileDescription for AnonSocket { } } // Notify peer fd that close has happened, since that can unblock reads and writes. - ecx.check_and_update_readiness(peer_fd)?; + ecx.epoll_send_fd_ready_events(peer_fd)?; } interp_ok(Ok(())) } @@ -276,7 +277,7 @@ fn anonsocket_write<'tcx>( } // 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)?; + ecx.epoll_send_fd_ready_events(peer_fd)?; return finish.call(ecx, Ok(write_size)); } @@ -369,7 +370,7 @@ fn anonsocket_read<'tcx>( ecx.unblock_thread(thread_id, BlockReason::UnnamedSocket)?; } // Notify epoll waiters. - ecx.check_and_update_readiness(peer_fd)?; + ecx.epoll_send_fd_ready_events(peer_fd)?; }; return finish.call(ecx, Ok(read_size)); diff --git a/src/shims/windows/fs.rs b/src/shims/windows/fs.rs index e4ec1b0130..936bbf06eb 100644 --- a/src/shims/windows/fs.rs +++ b/src/shims/windows/fs.rs @@ -24,8 +24,9 @@ impl FileDescription for DirHandle { interp_ok(self.path.metadata()) } - fn close<'tcx>( + fn destroy<'tcx>( self, + _self_addr: usize, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { @@ -50,8 +51,9 @@ impl FileDescription for MetadataHandle { interp_ok(Ok(self.meta.clone())) } - fn close<'tcx>( + fn destroy<'tcx>( self, + _self_addr: usize, _communicate_allowed: bool, _ecx: &mut MiriInterpCx<'tcx>, ) -> InterpResult<'tcx, io::Result<()>> { diff --git a/tests/fail-dep/libc/libc-epoll-data-race.rs b/tests/fail-dep/libc/libc-epoll-data-race.rs index f6ec5be61b..3b1217cda1 100644 --- a/tests/fail-dep/libc/libc-epoll-data-race.rs +++ b/tests/fail-dep/libc/libc-epoll-data-race.rs @@ -28,13 +28,10 @@ fn check_epoll_wait(epfd: i32, expected_notifications: &[(u32, u expected_notifications.len().try_into().unwrap(), "got wrong number of notifications" ); - let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; - for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { - let event = return_event.events; - let data = return_event.u64; - assert_eq!(event, expected_event.0, "got wrong events"); - assert_eq!(data, expected_event.1, "got wrong data"); - } + let got_notifications = + unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; + let got_notifications = got_notifications.iter().map(|e| (e.events, e.u64)).collect::>(); + assert_eq!(got_notifications, expected_notifications, "got wrong notifications"); } fn main() { diff --git a/tests/pass-dep/libc/libc-epoll-blocking.rs b/tests/pass-dep/libc/libc-epoll-blocking.rs index c97206487a..81d4e501c4 100644 --- a/tests/pass-dep/libc/libc-epoll-blocking.rs +++ b/tests/pass-dep/libc/libc-epoll-blocking.rs @@ -16,6 +16,7 @@ fn main() { test_epoll_block_then_unblock(); test_notification_after_timeout(); test_epoll_race(); + wakeup_on_new_interest(); } // Using `as` cast since `EPOLLET` wraps around @@ -35,18 +36,10 @@ fn check_epoll_wait( if res < 0 { panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); } - assert_eq!( - res, - expected_notifications.len().try_into().unwrap(), - "got wrong number of notifications" - ); - let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; - for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { - let event = return_event.events; - let data = return_event.u64; - assert_eq!(event, expected_event.0, "got wrong events"); - assert_eq!(data, expected_event.1, "got wrong data"); - } + let got_notifications = + unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; + let got_notifications = got_notifications.iter().map(|e| (e.events, e.u64)).collect::>(); + assert_eq!(got_notifications, expected_notifications, "got wrong notifications"); } // This test allows epoll_wait to block, then unblock without notification. @@ -179,3 +172,41 @@ fn test_epoll_race() { }; thread1.join().unwrap(); } + +/// Ensure that a blocked thread gets woken up when new interested are registered with the +/// epoll it is blocked on. +fn wakeup_on_new_interest() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create a socketpair instance. + let mut fds = [-1, -1]; + 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(); + let res = unsafe { libc_utils::write_all(fds[0], data as *const libc::c_void, 5) }; + assert_eq!(res, 5); + + // Block a thread on the epoll instance. + let t = std::thread::spawn(move || { + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = u64::try_from(fds[1]).unwrap(); + check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); + }); + // Ensure the thread is blocked. + std::thread::yield_now(); + + // Register fd[1] with EPOLLIN|EPOLLOUT|EPOLLET|EPOLLRDHUP + let mut ev = libc::epoll_event { + 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) }; + assert_eq!(res, 0); + + // This should wake up the thread. + t.join().unwrap(); +} diff --git a/tests/pass-dep/libc/libc-epoll-no-blocking.rs b/tests/pass-dep/libc/libc-epoll-no-blocking.rs index 7130790b86..c39192766d 100644 --- a/tests/pass-dep/libc/libc-epoll-no-blocking.rs +++ b/tests/pass-dep/libc/libc-epoll-no-blocking.rs @@ -41,18 +41,10 @@ fn check_epoll_wait(epfd: i32, expected_notifications: &[(u32, u if res < 0 { panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); } - assert_eq!( - res, - expected_notifications.len().try_into().unwrap(), - "got wrong number of notifications" - ); - let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; - for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { - let event = return_event.events; - let data = return_event.u64; - assert_eq!(event, expected_event.0, "got wrong events"); - assert_eq!(data, expected_event.1, "got wrong data"); - } + let got_notifications = + unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; + let got_notifications = got_notifications.iter().map(|e| (e.events, e.u64)).collect::>(); + assert_eq!(got_notifications, expected_notifications, "got wrong notifications"); } fn test_epoll_socketpair() { @@ -102,7 +94,7 @@ fn test_epoll_socketpair() { // 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 | libc::EPOLLHUP).unwrap(); + u32::try_from(libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLHUP | libc::EPOLLRDHUP).unwrap(); let expected_value = u64::try_from(fds[1]).unwrap(); check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]); } @@ -230,7 +222,7 @@ fn test_two_same_fd_in_same_epoll_instance() { let res = unsafe { libc_utils::write_all(fds[0], data as *const libc::c_void, 5) }; assert_eq!(res, 5); - //Two notification should be received. + // Two notification should be received. let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value = 5 as u64; check_epoll_wait::<8>( @@ -288,7 +280,7 @@ fn test_epoll_socketpair_both_sides() { let res = unsafe { libc_utils::write_all(fds[1], data as *const libc::c_void, 5) }; assert_eq!(res, 5); - //Two notification should be received. + // Two notification should be received. let expected_event0 = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value0 = fds[0] as u64; let expected_event1 = u32::try_from(libc::EPOLLOUT).unwrap(); @@ -448,12 +440,13 @@ fn test_socketpair_read() { let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_eq!(res, 0); - // Write 5 bytes to fds[1]. - let data = "abcde".as_bytes().as_ptr(); - let res = unsafe { libc_utils::write_all(fds[1], data as *const libc::c_void, 5) }; - assert_eq!(res, 5); + // Write a bunch of data bytes to fds[1]. + let data = [42u8; 40000]; + let res = + unsafe { libc_utils::write_all(fds[1], data.as_ptr() as *const libc::c_void, data.len()) }; + assert_eq!(res, data.len() as isize); - //Two notification should be received. + // Two notification should be received. let expected_event0 = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); let expected_value0 = fds[0] as u64; let expected_event1 = u32::try_from(libc::EPOLLOUT).unwrap(); @@ -463,36 +456,26 @@ fn test_socketpair_read() { &[(expected_event0, expected_value0), (expected_event1, expected_value1)], ); - // Read 3 bytes from fds[0]. - let mut buf: [u8; 3] = [0; 3]; + // Read a lof of databytes from fds[0]. + // If we make this read too small, Linux won't actually trigger a notification. + // So to ensure the test works on real systems, we make a sizable read. + let mut buf = [0; 38000]; let res = unsafe { libc_utils::read_all(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; - assert_eq!(res, 3); - assert_eq!(buf, "abc".as_bytes()); + assert_eq!(res, buf.len() as isize); - // Notification will be provided in Miri. - // But in real systems, no notification will be provided here, since Linux prefers to avoid - // wakeups that are likely to lead to only small amounts of data being read/written. - // We make the test work in both cases, thus documenting the difference in behavior. + // fds[1] can be written now. let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value = fds[1] as u64; - if cfg!(miri) { - check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]); - } else { - check_epoll_wait::<8>(epfd, &[]); - } + check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]); // Read until the buffer is empty. - let mut buf: [u8; 2] = [0; 2]; + let rest = data.len() - buf.len(); let res = - unsafe { libc_utils::read_all(fds[0], buf.as_mut_ptr().cast(), buf.len() as libc::size_t) }; - assert_eq!(res, 2); - assert_eq!(buf, "de".as_bytes()); + unsafe { libc_utils::read_all(fds[0], buf.as_mut_ptr().cast(), rest as libc::size_t) }; + assert_eq!(res, rest as isize); - // Notification will be provided. - // In real system, notification will be provided too. - let expected_event = u32::try_from(libc::EPOLLOUT).unwrap(); - let expected_value = fds[1] as u64; + // Again we get a notification that fds[1] can be written. check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]); } @@ -512,7 +495,7 @@ fn test_no_notification_for_unregister_flag() { events: (libc::EPOLLOUT | libc::EPOLLET) as _, u64: u64::try_from(fds[0]).unwrap(), }; - let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[0], &mut ev) }; assert_eq!(res, 0); // Write to fd[1]. @@ -599,7 +582,7 @@ fn test_epoll_lost_events() { let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) }; assert_eq!(res, 0); - //Two notification should be received. But we only provide buffer for one event. + // Two notification should be received. But we only provide buffer for one event. let expected_event0 = u32::try_from(libc::EPOLLOUT).unwrap(); let expected_value0 = fds[0] as u64; check_epoll_wait::<1>(epfd, &[(expected_event0, expected_value0)]); @@ -641,7 +624,8 @@ fn test_ready_list_fetching_logic() { check_epoll_wait::<1>(epfd, &[(expected_event1, expected_value1)]); } -// In epoll_ctl, if the value of epfd equals to fd, EINVAL should be returned. +// In epoll_ctl, if the value of epfd equals to fd, EFAULT should be returned. +// (The docs say loops cause EINVAL, but experiments show it is EFAULT.) fn test_epoll_ctl_epfd_equal_fd() { // Create an epoll instance. let epfd = unsafe { libc::epoll_create1(0) }; @@ -650,7 +634,7 @@ fn test_epoll_ctl_epfd_equal_fd() { let array_ptr = std::ptr::without_provenance_mut::(0x100); let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, epfd, array_ptr) }; let e = std::io::Error::last_os_error(); - assert_eq!(e.raw_os_error(), Some(libc::EINVAL)); + assert_eq!(e.raw_os_error(), Some(libc::EFAULT)); assert_eq!(res, -1); }