Fix epoll bug in release mode on Rust 1.20+

[Susurrus]

Seems to have occurred within a Rust release after 1.13, so let's test all of them since then!

[Susurrus]

Progress! Looks like epoll works before 1.20 and fails from then onwards. Nothing immediately sticks out to me in the release notes, gonna hop onto #rust and ask.

[Susurrus]

This is the only thing that seems to be really fundamental that's changed. I'd prefer not to have to test Rust right before this merged and right after, but maybe that's the proper next step.

[Arnavion]

diff --git a/src/sys/epoll.rs b/src/sys/epoll.rs
index 5ab766d..69effe7 100644
--- a/src/sys/epoll.rs
+++ b/src/sys/epoll.rs
@@ -65,16 +65,6 @@ impl EpollEvent {
     }
 }

-impl<'a> Into<&'a mut EpollEvent> for Option<&'a mut EpollEvent> {
-    #[inline]
-    fn into(self) -> &'a mut EpollEvent {
-        match self {
-            Some(epoll_event) => epoll_event,
-            None => unsafe { &mut *ptr::null_mut::<EpollEvent>() }
-        }
-    }
-}
-
 #[inline]
 pub fn epoll_create() -> Result<RawFd> {
     let res = unsafe { libc::epoll_create(1024) };
@@ -91,15 +81,21 @@ pub fn epoll_create1(flags: EpollCreateFlags) -> Result<RawFd> {

 #[inline]
 pub fn epoll_ctl<'a, T>(epfd: RawFd, op: EpollOp, fd: RawFd, event: T) -> Result<()>
-    where T: Into<&'a mut EpollEvent>
+    where T: Into<Option<&'a mut EpollEvent>>
 {
-    let event: &mut EpollEvent = event.into();
-    if event as *const EpollEvent == ptr::null() && op != EpollOp::EpollCtlDel {
-        Err(Error::Sys(Errno::EINVAL))
-    } else {
-        let res = unsafe { libc::epoll_ctl(epfd, op as c_int, fd, &mut event.event) };
-        Errno::result(res).map(drop)
-    }
+    let mut event: Option<&mut EpollEvent> = event.into();
+    let res = unsafe {
+        if let Some(ref mut event) = event {
+            libc::epoll_ctl(epfd, op as c_int, fd, &mut event.event)
+        } else {
+            if op != EpollOp::EpollCtlDel {
+                return Err(Error::Sys(Errno::EINVAL));
+            }
+
+            libc::epoll_ctl(epfd, op as c_int, fd, ptr::null_mut())
+        }
+    };
+    Errno::result(res).map(drop)
 }

 #[inline]

[Susurrus]

It's passed everything but Apple tests now because Travis is super backlogged. Hopefully it completes soon, but this should be GTG already. @asomers Care to give this a look-over?

[Susurrus]

Alright, we're all green here. @asomers If I don't hear back from you, I'll push this tonight.

[asomers]

Awesome that you fixed it! Still, I don't fully understand why the old code failed. Was the problem that event.into() as *const EpollEvent == ptr::null() would always evaluate false, even when event was None, because the behavior of &mut *ptr::null_mut::<T> is undefined?

[Arnavion]

because the behavior of &mut *ptr::null_mut::<T> is undefined?

Yes. *ptr::null_mut() is UB.

[asomers]

Too bad the compiler can't detect that. Thanks for helping to fix it! We've known about this bug since the summer, but never understood what was going on.

bors r+

[bors]

Timed out

[Susurrus]

bors retry

[Susurrus]

Guess that doesn't work.

bors r+ asomers

[bors]

Timed out

[Susurrus]

Ugh!

bors r+

[bors]

Build succeeded

• buildbot/nix-rust/nix amd64_fbsd11
• buildbot/nix-rust/nix i386_fbsd11
• continuous-integration/travis-ci/push

[jonhoo]

This changes the epoll API, so should probably be added to the changelog, no?

[Susurrus]

@jonhoo This didn't change the API for epoll_ctl at all. You can see there was no test code changed in this PR, and there are at least 4 calls to epoll_ctl under /test. Technically it changed the API for &EpollEvent, but that is likely unused outside of the function call for this function, so I don't think it's worth mentioning.

[jonhoo]

Ahh, specifically because Option<T>: From<T> -- got it!