Auto merge of #3818 - tiif:loseevents, r=RalfJung

epoll: iterate through output buffer then fetch an event from ready list

Fixes #3812

src/tools/miri/src/shims/unix/linux/epoll.rs

@@ -436,23 +436,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mut num_of_events: i32 = 0;
         let mut array_iter = this.project_array_fields(&events)?;
 
-        while let Some((epoll_key, epoll_return)) = ready_list.pop_first() {
-            // If the file description is fully close, the entry for corresponding FdID in the
-            // global epoll event interest table would be empty.
-            if this.machine.epoll_interests.get_epoll_interest(epoll_key.0).is_some() {
-                // Return notification to the caller if the file description is not fully closed.
-                if let Some(des) = array_iter.next(this)? {
-                    this.write_int_fields_named(
-                        &[
-                            ("events", epoll_return.events.into()),
-                            ("u64", epoll_return.data.into()),
-                        ],
-                        &des.1,
-                    )?;
-                    num_of_events = num_of_events.checked_add(1).unwrap();
-                } else {
-                    break;
-                }
+        while let Some(des) = array_iter.next(this)? {
+            if let Some(epoll_event_instance) = ready_list_next(this, &mut ready_list) {
+                this.write_int_fields_named(
+                    &[
+                        ("events", epoll_event_instance.events.into()),
+                        ("u64", epoll_event_instance.data.into()),
+                    ],
+                    &des.1,
+                )?;
+                num_of_events = num_of_events.strict_add(1);
+            } else {
+                break;
             }
         }
         Ok(Scalar::from_i32(num_of_events))
@@ -495,3 +490,21 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         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<EpollEventInstance> {
+    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);
+        }
+    }
+    return None;
+}

src/tools/miri/tests/pass-dep/libc/libc-epoll.rs

@@ -20,6 +20,8 @@ fn main() {
     test_pointer();
     test_two_same_fd_in_same_epoll_instance();
     test_epoll_wait_maxevent_zero();
+    test_epoll_lost_events();
+    test_ready_list_fetching_logic();
 }
 
 // Using `as` cast since `EPOLLET` wraps around
@@ -548,3 +550,65 @@ fn test_epoll_wait_maxevent_zero() {
     assert_eq!(e.raw_os_error(), Some(libc::EINVAL));
     assert_eq!(res, -1);
 }
+
+// This is a test for https://github.com/rust-lang/miri/issues/3812,
+// epoll can lose events if they don't fit in the output buffer.
+fn test_epoll_lost_events() {
+    // 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);
+
+    // Register both fd to the same epoll instance.
+    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_eq!(res, 0);
+    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_eq!(res, 0);
+
+    //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)]);
+
+    // Previous event should be returned for the second epoll_wait.
+    let expected_event1 = u32::try_from(libc::EPOLLOUT).unwrap();
+    let expected_value1 = fds[1] as u64;
+    check_epoll_wait::<1>(epfd, &[(expected_event1, expected_value1)]);
+}
+
+// This is testing if closing an fd that is already in ready list will cause an empty entry in
+// returned notification.
+// Related discussion in https://github.com/rust-lang/miri/pull/3818#discussion_r1720679440.
+fn test_ready_list_fetching_logic() {
+    // Create an epoll instance.
+    let epfd = unsafe { libc::epoll_create1(0) };
+    assert_ne!(epfd, -1);
+
+    // Create two eventfd instances.
+    let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC;
+    let fd0 = unsafe { libc::eventfd(0, flags) };
+    let fd1 = unsafe { libc::eventfd(0, flags) };
+
+    // Register both fd to the same epoll instance. At this point, both of them are on the ready list.
+    let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd0 as u64 };
+    let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd0, &mut ev) };
+    assert_eq!(res, 0);
+    let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd1 as u64 };
+    let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd1, &mut ev) };
+    assert_eq!(res, 0);
+
+    // Close fd0 so the first entry in the ready list will be empty.
+    let res = unsafe { libc::close(fd0) };
+    assert_eq!(res, 0);
+
+    // Notification for fd1 should be returned.
+    let expected_event1 = u32::try_from(libc::EPOLLOUT).unwrap();
+    let expected_value1 = fd1 as u64;
+    check_epoll_wait::<1>(epfd, &[(expected_event1, expected_value1)]);
+}