From a69074828cb350bf43a405bb52c2af99d0724247 Mon Sep 17 00:00:00 2001 From: tiif Date: Tue, 17 Sep 2024 17:46:33 +0800 Subject: [PATCH] Tokio ICE fix: Changed the type of EpollEventInterest::epfd from i32 to WeakFileDescriptionRef --- src/shims/unix/linux/epoll.rs | 13 ++++--- tests/pass-dep/libc/libc-epoll-no-blocking.rs | 38 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/src/shims/unix/linux/epoll.rs b/src/shims/unix/linux/epoll.rs index d91ce45e10..3d4fe770e9 100644 --- a/src/shims/unix/linux/epoll.rs +++ b/src/shims/unix/linux/epoll.rs @@ -51,6 +51,9 @@ impl EpollEventInstance { #[derive(Clone, Debug)] 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, @@ -61,8 +64,8 @@ pub struct EpollEventInterest { data: u64, /// Ready list of the epoll instance under which this EpollEventInterest is registered. ready_list: Rc>>, - /// The file descriptor value that this EpollEventInterest is registered under. - epfd_num: i32, + /// The epoll file description that this EpollEventInterest is registered under. + weak_epfd: WeakFileDescriptionRef, } /// EpollReadyEvents reflects the readiness of a file description. @@ -343,7 +346,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { events, data, ready_list: Rc::clone(ready_list), - epfd_num: epfd_value, + weak_epfd: epfd.downgrade(), })); if op == epoll_ctl_add { @@ -553,12 +556,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { if is_updated { // Edge-triggered notification only notify one thread even if there are // multiple threads block on the same epfd. - let epfd = this.machine.fds.get(epoll_interest.borrow().epfd_num).unwrap(); // This unwrap can never fail because if the current epoll instance were - // closed and its epfd value reused, the upgrade of weak_epoll_interest + // 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.downcast::().unwrap().thread_id.borrow_mut().pop() diff --git a/tests/pass-dep/libc/libc-epoll-no-blocking.rs b/tests/pass-dep/libc/libc-epoll-no-blocking.rs index 647b5e6064..3b6727f142 100644 --- a/tests/pass-dep/libc/libc-epoll-no-blocking.rs +++ b/tests/pass-dep/libc/libc-epoll-no-blocking.rs @@ -23,6 +23,7 @@ fn main() { test_ready_list_fetching_logic(); test_epoll_ctl_epfd_equal_fd(); test_epoll_ctl_notification(); + test_issue_3858(); } // Using `as` cast since `EPOLLET` wraps around @@ -683,3 +684,40 @@ fn test_epoll_ctl_notification() { // for this epfd, because there is no I/O event between the two epoll_wait. check_epoll_wait::<1>(epfd0, &[]); } + +// Test for ICE caused by weak epoll interest upgrade succeed, but the attempt to retrieve +// the epoll instance based on the epoll file descriptor value failed. EpollEventInterest +// should store a WeakFileDescriptionRef instead of the file descriptor number, so if the +// epoll instance is duped, it'd still be usable after `close` is called on the original +// epoll file descriptor. +// https://github.com/rust-lang/miri/issues/3858 +fn test_issue_3858() { + // Create an eventfd instance. + let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; + let fd = unsafe { libc::eventfd(0, flags) }; + + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Register eventfd with EPOLLIN | EPOLLET. + let mut ev = libc::epoll_event { + events: (libc::EPOLLIN | libc::EPOLLET) as _, + u64: u64::try_from(fd).unwrap(), + }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; + assert_eq!(res, 0); + + // Dup the epoll instance. + let newfd = unsafe { libc::dup(epfd) }; + assert_ne!(newfd, -1); + + // Close the old epoll instance, so the new FD is now the only FD. + let res = unsafe { libc::close(epfd) }; + assert_eq!(res, 0); + + // Write to the eventfd instance. + let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; + assert_eq!(res, 8); +}