Skip to content

Commit

Permalink
Auto merge of #3891 - tiif:tokiotest, r=RalfJung
Browse files Browse the repository at this point in the history
Fix tokio test ICE

Fixes #3858

It turned out that the issue mentioned [here](#3858 (comment)) is the exact cause of ICE.
So in this PR, I changed the type of ``EpollEventInterest::epfd`` from ``i32`` to ``WeakFileDescriptionRef``.
  • Loading branch information
bors committed Sep 17, 2024
2 parents 83b0c12 + a690748 commit bdab169
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 5 deletions.
13 changes: 8 additions & 5 deletions src/shims/unix/linux/epoll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -61,8 +64,8 @@ pub struct EpollEventInterest {
data: u64,
/// Ready list of the epoll instance under which this EpollEventInterest is registered.
ready_list: Rc<RefCell<BTreeMap<(FdId, i32), EpollEventInstance>>>,
/// 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.
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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::<Epoll>().unwrap().thread_id.borrow_mut().pop()
Expand Down
38 changes: 38 additions & 0 deletions tests/pass-dep/libc/libc-epoll-no-blocking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}

0 comments on commit bdab169

Please sign in to comment.