signal: Use signal-hook for registration of signals

This saves some code and gets rid of quite some amount of unsafe code.

Cargo.toml

@@ -25,6 +25,7 @@ tokio-io = "0.1"
 [target.'cfg(unix)'.dependencies]
 libc = "0.2"
 mio-uds = "0.6"
+signal-hook = "0.1"
 
 [dev-dependencies]
 tokio-core = "0.1.17"

src/unix.rs

@@ -8,11 +8,10 @@
 pub extern crate libc;
 extern crate mio;
 extern crate mio_uds;
+extern crate signal_hook;
 
-use std::cell::UnsafeCell;
+use std::io::{self, Error, ErrorKind};
-use std::io;
 use std::io::prelude::*;
-use std::mem;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Mutex, Once, ONCE_INIT};
 
@@ -53,8 +52,7 @@ struct SignalInfo {
     recipients: Mutex<Vec<Box<Sender<c_int>>>>,
 
     init: Once,
-    initialized: UnsafeCell<bool>,
+    initialized: AtomicBool,
-    prev: UnsafeCell<libc::sigaction>,
 }
 
 struct Globals {
@@ -68,9 +66,8 @@ impl Default for SignalInfo {
         SignalInfo {
             pending: AtomicBool::new(false),
             init: ONCE_INIT,
-            initialized: UnsafeCell::new(false),
+            initialized: AtomicBool::new(false),
             recipients: Mutex::new(Vec::new()),
-            prev: UnsafeCell::new(unsafe { mem::zeroed() }),
         }
     }
 }
@@ -101,42 +98,13 @@ fn globals() -> &'static Globals {
 /// 1. Flag that our specific signal was received (e.g. store an atomic flag)
 /// 2. Wake up driver tasks by writing a byte to a pipe
 ///
-/// Those two operations shoudl both be async-signal safe. After that's done we
+/// Those two operations shoudl both be async-signal safe.
-/// just try to call a previous signal handler, if any, to be "good denizens of
+fn action(slot: &SignalInfo, mut sender: &UnixStream) {
-/// the internet"
+    slot.pending.store(true, Ordering::SeqCst);
-extern "C" fn handler(signum: c_int, info: *mut libc::siginfo_t, ptr: *mut libc::c_void) {
-    type FnSigaction = extern "C" fn(c_int, *mut libc::siginfo_t, *mut libc::c_void);
-    type FnHandler = extern "C" fn(c_int);
-    unsafe {
-        let slot = match (*GLOBALS).signals.get(signum as usize) {
-            Some(slot) => slot,
-            None => return,
-        };
-        slot.pending.store(true, Ordering::SeqCst);
 
-        // Send a wakeup, ignore any errors (anything reasonably possible is
+    // Send a wakeup, ignore any errors (anything reasonably possible is
-        // full pipe and then it will wake up anyway).
+    // full pipe and then it will wake up anyway).
-        drop((*GLOBALS).sender.write(&[1]));
+    drop(sender.write(&[1]));
-
-        let fnptr = (*slot.prev.get()).sa_sigaction;
-        if fnptr == 0 || fnptr == libc::SIG_DFL || fnptr == libc::SIG_IGN {
-            return;
-        }
-        let mut sa_flags = (*slot.prev.get()).sa_flags;
-        // android defines SIGINFO with a different type than sa_flags,
-        // this ensures that the variables are of the same type regardless of platform
-        #[allow(unused_assignments)]
-        let mut siginfo = sa_flags;
-        siginfo = libc::SA_SIGINFO as _;
-        sa_flags &= siginfo;
-        if sa_flags == 0 {
-            let action = mem::transmute::<usize, FnHandler>(fnptr);
-            action(signum)
-        } else {
-            let action = mem::transmute::<usize, FnSigaction>(fnptr);
-            action(signum, info, ptr)
-        }
-    }
 }
 
 /// Enable this module to receive signal notifications for the `signal`
@@ -145,40 +113,31 @@ extern "C" fn handler(signum: c_int, info: *mut libc::siginfo_t, ptr: *mut libc:
 /// This will register the signal handler if it hasn't already been registered,
 /// returning any error along the way if that fails.
 fn signal_enable(signal: c_int) -> io::Result<()> {
-    let siginfo = match globals().signals.get(signal as usize) {
+    if signal_hook::FORBIDDEN.contains(&signal) {
+        return Err(Error::new(ErrorKind::Other, format!("Refusing to register signal {}", signal)));
+    }
+
+    let globals = globals();
+    let siginfo = match globals.signals.get(signal as usize) {
         Some(slot) => slot,
         None => return Err(io::Error::new(io::ErrorKind::Other, "signal too large")),
     };
-    unsafe {
+    let mut registered = Ok(());
-        let mut err = None;
+    siginfo.init.call_once(|| {
-        siginfo.init.call_once(|| {
+        registered = unsafe {
-            let mut new: libc::sigaction = mem::zeroed();
+            signal_hook::register(signal, move || action(siginfo, &globals.sender)).map(|_| ())
-            new.sa_sigaction = handler as usize;
+        };
-            let flags = libc::SA_RESTART | libc::SA_NOCLDSTOP;;
+        if registered.is_ok() {
-            // android defines SIGINFO with a different type than sa_flags,
+            siginfo.initialized.store(true, Ordering::Relaxed);
-            // this ensures that the variables are of the same type regardless of platform
-            #[allow(unused_assignments)]
-            let mut sa_siginfo = flags;
-            sa_siginfo = libc::SA_SIGINFO as _;
-            let flags = flags | sa_siginfo;
-            new.sa_flags = flags as _;
-            if libc::sigaction(signal, &new, &mut *siginfo.prev.get()) != 0 {
-                err = Some(io::Error::last_os_error());
-            } else {
-                *siginfo.initialized.get() = true;
-            }
-        });
-        if let Some(err) = err {
-            return Err(err);
-        }
-        if *siginfo.initialized.get() {
-            Ok(())
-        } else {
-            Err(io::Error::new(
-                io::ErrorKind::Other,
-                "failed to register signal handler",
-            ))
         }
+    });
+    registered?;
+    // If the call_once failed, it won't be retried on the next attempt to register the signal. In
+    // such case it is not run, registered is still `Ok(())`, initialized is still false.
+    if siginfo.initialized.load(Ordering::Relaxed) {
+        Ok(())
+    } else {
+        Err(Error::new(ErrorKind::Other, "Failed to register signal handler"))
     }
 }
 
@@ -347,6 +306,13 @@ impl Signal {
     /// A `Signal` stream can be created for a particular signal number
     /// multiple times. When a signal is received then all the associated
     /// channels will receive the signal notification.
+    ///
+    /// # Errors
+    ///
+    /// * If the lower-level C functions fail for some reason.
+    /// * If the previous initialization of this specific signal failed.
+    /// * If the signal is one of
+    ///   [`signal_hook::FORBIDDEN`](https://docs.rs/signal-hook/*/signal_hook/fn.register.html#panics)
     pub fn new(signal: c_int) -> IoFuture<Signal> {
         Signal::with_handle(signal, &Handle::current())
     }