fix EPOLL_CTL_MOD

Author: RalfJung

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

@@ -44,7 +44,7 @@ fn range_for_id(id: FdId) -> std::ops::RangeInclusive<EpollEventKey> {
 }
 
 /// EpollEventInstance contains information that will be returned by epoll_wait.
-#[derive(Debug)]
+#[derive(Debug, Default)]
 pub struct EpollEventInstance {
     /// Bitmask of event types that happened to the file description.
     events: u32,
@@ -54,12 +54,6 @@ pub struct EpollEventInstance {
     clock: VClock,
 }
 
-impl EpollEventInstance {
-    pub fn new(data: u64) -> EpollEventInstance {
-        EpollEventInstance { events: 0, data, clock: Default::default() }
-    }
-}
-
 /// EpollEventInterest registers the file description information to an epoll
 /// instance during a successful `epoll_ctl` call. It also stores additional
 /// information needed to check and update readiness state for `epoll_wait`.
@@ -345,8 +339,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 );
             }
 
-            // Add new interest to list.
+            // Add new interest to list. Experiments show that we need to reset all state
+            // on `EPOLL_CTL_MOD`, including the edge tracking.
             let epoll_key = (id, fd);
+            let new_interest = EpollEventInterest { events, data, prev_events: 0 };
             let new_interest = if op == epoll_ctl_add {
                 if interest_list.range(range_for_id(id)).next().is_none() {
                     // This is the first time this FD got added to this epoll.
@@ -358,25 +354,24 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                         // We already had interest in this.
                         return this.set_last_error_and_return_i32(LibcError("EEXIST"));
                     }
-                    btree_map::Entry::Vacant(e) =>
-                        e.insert(EpollEventInterest { events, data, prev_events: 0 }),
+                    btree_map::Entry::Vacant(e) => e.insert(new_interest),
                 }
             } else {
                 // 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.events = events;
-                // FIXME what about the data?
+                *interest = new_interest;
                 interest
             };
 
             // Deliver events for the new interest.
+            let force_edge = true; // makes no difference since we reset `prev_events`
             send_ready_events_to_interests(
                 this,
                 &epfd,
                 fd_ref.as_unix(this).get_epoll_ready_events()?.get_event_bitmask(this),
-                /* force_edge */ false,
+                force_edge,
                 std::iter::once((&epoll_key, new_interest)),
             )?;
 
@@ -588,7 +583,8 @@ fn send_ready_events_to_interests<'tcx, 'a>(
             ready_list.remove(&event_key);
             continue;
         }
-        // Generate new instance, or update existing one.
+        // Generate new instance, or update existing one. It is crucial that whe we are done,
+        // if an interest exists in the ready list, then it matches the latest events and data!
         let instance = match ready_list.entry(event_key) {
             btree_map::Entry::Occupied(e) => e.into_mut(),
             btree_map::Entry::Vacant(e) => {
@@ -598,10 +594,11 @@ fn send_ready_events_to_interests<'tcx, 'a>(
                     // prior entry to update; just skip it.
                     continue;
                 }
-                e.insert(EpollEventInstance::new(interest.data))
+                e.insert(EpollEventInstance::default())
             }
         };
         instance.events = flags;
+        instance.data = interest.data;
         ecx.release_clock(|clock| {
             instance.clock.join(clock);
         })?;

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

@@ -105,6 +105,7 @@ fn test_epoll_socketpair() {
 
 // This test first registers a file description with a flag that does not lead to notification,
 // then EPOLL_CTL_MOD to add another flag that will lead to notification.
+// Also check that the new data value set via MOD is applied properly.
 fn test_epoll_ctl_mod() {
     // Create an epoll instance.
     let epfd = unsafe { libc::epoll_create1(0) };
@@ -115,28 +116,49 @@ fn test_epoll_ctl_mod() {
     let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) };
     assert_eq!(res, 0);
 
-    // Register fd[1] with EPOLLIN|EPOLLET.
-    let mut ev = libc::epoll_event {
-        events: (libc::EPOLLIN | libc::EPOLLET) as _,
-        u64: u64::try_from(fds[1]).unwrap(),
-    };
+    // Register fd[1] with EPOLLIN|EPOLLET, and data of "0".
+    let mut ev = libc::epoll_event { events: (libc::EPOLLIN | libc::EPOLLET) as _, u64: 0 };
     let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds[1], &mut ev) };
     assert_eq!(res, 0);
 
     // Check result from epoll_wait. No notification would be returned.
     check_epoll_wait::<8>(epfd, &[]);
 
-    // Use EPOLL_CTL_MOD to change to EPOLLOUT flag.
-    let mut ev = libc::epoll_event {
-        events: (libc::EPOLLOUT | libc::EPOLLET) as _,
-        u64: u64::try_from(fds[1]).unwrap(),
-    };
+    // Use EPOLL_CTL_MOD to change to EPOLLOUT flag and data.
+    let mut ev = libc::epoll_event { events: (libc::EPOLLOUT | libc::EPOLLET) as _, u64: 1 };
     let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_MOD, fds[1], &mut ev) };
     assert_eq!(res, 0);
 
-    // Check result from epoll_wait. EPOLLOUT notification is expected.
+    // Check result from epoll_wait. EPOLLOUT notification and new data is expected.
     let expected_event = u32::try_from(libc::EPOLLOUT).unwrap();
-    let expected_value = u64::try_from(fds[1]).unwrap();
+    let expected_value = 1;
+    check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]);
+
+    // Write to fds[1] and read from fds[0] to make the notification ready again
+    // (relying on there always being an event when the buffer gets emptied).
+    let data = "abc".as_bytes();
+    let res = unsafe { libc_utils::write_all(fds[1], data.as_ptr().cast(), data.len()) };
+    assert_eq!(res, 3);
+    let mut buf = [0u8; 3];
+    let res = unsafe { libc_utils::read_all(fds[0], buf.as_mut_ptr().cast(), buf.len()) };
+    assert_eq!(res, 3);
+
+    // Now that the event is already ready, change the "data" value.
+    let mut ev = libc::epoll_event { events: (libc::EPOLLOUT | libc::EPOLLET) as _, u64: 2 };
+    let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_MOD, fds[1], &mut ev) };
+    assert_eq!(res, 0);
+
+    // Receive event, with latest data value.
+    let expected_event = u32::try_from(libc::EPOLLOUT).unwrap();
+    let expected_value = 2;
+    check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]);
+
+    // Do another update that changes nothing.
+    let mut ev = libc::epoll_event { events: (libc::EPOLLOUT | libc::EPOLLET) as _, u64: 2 };
+    let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_MOD, fds[1], &mut ev) };
+    assert_eq!(res, 0);
+
+    // This re-triggers the event, even if it's the same flags as before.
     check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)]);
 }