Fix signal handling on Unix systems

This removes the the signal handling machinery in tty::unix, and
replaces it with functionality from signal-hook, which should be more
robust. Signals caught by signal-hook wake up the existing I/O event
loop, which then delegates back to the PTY to handle them.

In particular, this allows `SIGCHLD` (i.e. child process exits) to shut
down the terminal promptly, instead of sometimes leaving the window
lingering.

Fixes #915.
Fixes #1276.
Fixes #1313.

As a side effect, this fixes a very rare bug on Linux, where a `read`
from the PTY on the master side would sometimes "fail" with `EIO` if the
child closed the client side at a particular moment. This was subject to
a race condition, and was very difficult to trigger in practice.

3 files changed, 122 insertions, 96 deletions

diff --git a/src/tty/mod.rs b/src/tty/mod.rs
index cedb010e..ea2dd0c4 100644
--- a/src/tty/mod.rs
+++ b/src/tty/mod.rs
@@ -40,7 +40,7 @@ pub trait EventedReadWrite {
     fn register(
         &mut self,
         _: &mio::Poll,
-        _: &mut dyn Iterator<Item = &usize>,
+        _: &mut dyn Iterator<Item = mio::Token>,
         _: mio::Ready,
         _: mio::PollOpt,
     ) -> io::Result<()>;
@@ -53,6 +53,29 @@ pub trait EventedReadWrite {
     fn write_token(&self) -> mio::Token;
 }
 
+/// Events concerning TTY child processes
+#[derive(PartialEq)]
+pub enum ChildEvent {
+    /// Indicates the child has exited
+    Exited
+}
+
+/// A pseudoterminal (or PTY)
+///
+/// This is a refinement of EventedReadWrite that also provides a channel through which we can be
+/// notified if the PTY child process does something we care about (other than writing to the TTY).
+/// In particular, this allows for race-free child exit notification on UNIX (cf. `SIGCHLD`).
+pub trait EventedPty : EventedReadWrite {
+    #[cfg(unix)]
+    fn child_event_token(&self) -> mio::Token;
+
+    /// Tries to retrieve an event
+    ///
+    /// Returns `Some(event)` on success, or `None` if there are no events to retrieve.
+    #[cfg(unix)]
+    fn next_child_event(&mut self) -> Option<ChildEvent>;
+}
+
 // Setup environment variables
 pub fn setup_env(config: &Config) {
     // Default to 'alacritty' terminfo if it is available, otherwise
diff --git a/src/tty/unix.rs b/src/tty/unix.rs
index 2a07fc58..d8392dcd 100644
--- a/src/tty/unix.rs
+++ b/src/tty/unix.rs
@@ -15,54 +15,33 @@
 //! tty related functionality
 //!
 
-use crate::tty::EventedReadWrite;
+use crate::tty::{EventedReadWrite, EventedPty, ChildEvent};
 use crate::term::SizeInfo;
 use crate::display::OnResize;
 use crate::config::{Config, Shell};
 use crate::cli::Options;
 use mio;
 
-use libc::{self, c_int, pid_t, winsize, SIGCHLD, TIOCSCTTY, WNOHANG};
+use libc::{self, c_int, pid_t, winsize, TIOCSCTTY};
+use nix::pty::openpty;
+use signal_hook::{self as sighook, iterator::Signals};
 
-use std::os::unix::io::{FromRawFd, RawFd};
+use std::os::unix::{process::CommandExt, io::{FromRawFd, AsRawFd, RawFd}};
 use std::fs::File;
-use std::os::unix::process::CommandExt;
-use std::process::{Command, Stdio};
+use std::process::{Command, Stdio, Child};
 use std::ffi::CStr;
 use std::ptr;
 use mio::unix::EventedFd;
 use std::io;
-use std::os::unix::io::AsRawFd;
-
+use std::sync::atomic::{AtomicUsize, Ordering};
 
 /// Process ID of child process
 ///
 /// Necessary to put this in static storage for `sigchld` to have access
-pub static mut PID: pid_t = 0;
-
-/// Exit flag
-///
-/// Calling exit() in the SIGCHLD handler sometimes causes opengl to deadlock,
-/// and the process hangs. Instead, this flag is set, and its status can be
-/// checked via `process_should_exit`.
-static mut SHOULD_EXIT: bool = false;
-
-extern "C" fn sigchld(_a: c_int) {
-    let mut status: c_int = 0;
-    unsafe {
-        let p = libc::waitpid(PID, &mut status, WNOHANG);
-        if p < 0 {
-            die!("Waiting for pid {} failed: {}\n", PID, errno());
-        }
-
-        if PID == p {
-            SHOULD_EXIT = true;
-        }
-    }
-}
+static PID: AtomicUsize = AtomicUsize::new(0);
 
-pub fn process_should_exit() -> bool {
-    unsafe { SHOULD_EXIT }
+pub fn child_pid() -> pid_t {
+    PID.load(Ordering::Relaxed) as pid_t
 }
 
 /// Get the current value of errno
@@ -71,50 +50,15 @@ fn errno() -> c_int {
 }
 
 /// Get raw fds for master/slave ends of a new pty
-#[cfg(target_os = "linux")]
-fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
-    let mut master: c_int = 0;
-    let mut slave: c_int = 0;
-
-    let win = winsize {
-        ws_row: libc::c_ushort::from(rows),
-        ws_col: libc::c_ushort::from(cols),
-        ws_xpixel: 0,
-        ws_ypixel: 0,
-    };
-
-    let res = unsafe {
-        libc::openpty(&mut master, &mut slave, ptr::null_mut(), ptr::null(), &win)
-    };
-
-    if res < 0 {
-        die!("openpty failed");
-    }
-
-    (master, slave)
-}
-
-#[cfg(any(target_os = "macos",target_os = "freebsd",target_os = "openbsd"))]
-fn openpty(rows: u8, cols: u8) -> (c_int, c_int) {
-    let mut master: c_int = 0;
-    let mut slave: c_int = 0;
-
-    let mut win = winsize {
-        ws_row: libc::c_ushort::from(rows),
-        ws_col: libc::c_ushort::from(cols),
-        ws_xpixel: 0,
-        ws_ypixel: 0,
-    };
-
-    let res = unsafe {
-        libc::openpty(&mut master, &mut slave, ptr::null_mut(), ptr::null_mut(), &mut win)
-    };
+fn make_pty(size: winsize) -> (RawFd, RawFd) {
+    let mut win_size = size;
+    win_size.ws_xpixel = 0;
+    win_size.ws_ypixel = 0;
 
-    if res < 0 {
-        die!("openpty failed");
-    }
+    let ends = openpty(Some(&win_size), None)
+        .expect("openpty failed");
 
-    (master, slave)
+    (ends.master, ends.slave)
 }
 
 /// Really only needed on BSD, but should be fine elsewhere
@@ -185,9 +129,12 @@ fn get_pw_entry(buf: &mut [i8; 1024]) -> Passwd<'_> {
 }
 
 pub struct Pty {
+    child: Child,
     pub fd: File,
-    pub raw_fd: RawFd,
     token: mio::Token,
+    signals: Signals,
+    signals_token: mio::Token,
+
 }
 
 impl Pty {
@@ -215,11 +162,11 @@ pub fn new<T: ToWinsize>(
     size: &T,
     window_id: Option<usize>,
 ) -> Pty {
-    let win = size.to_winsize();
+    let win_size = size.to_winsize();
     let mut buf = [0; 1024];
     let pw = get_pw_entry(&mut buf);
 
-    let (master, slave) = openpty(win.ws_row as _, win.ws_col as _);
+    let (master, slave) = make_pty(win_size);
 
     let default_shell = if cfg!(target_os = "macos") {
         let shell_name = pw.shell.rsplit('/').next().unwrap();
@@ -292,15 +239,14 @@ pub fn new<T: ToWinsize>(
         builder.current_dir(dir.as_path());
     }
 
+    // Prepare signal handling before spawning child
+    let signals = Signals::new(&[sighook::SIGCHLD]).expect("error preparing signal handling");
+
     match builder.spawn() {
         Ok(child) => {
-            unsafe {
-                // Set PID for SIGCHLD handler
-                PID = child.id() as _;
+            // Remember child PID so other modules can use it
+            PID.store(child.id() as usize, Ordering::Relaxed);
 
-                // Handle SIGCHLD
-                libc::signal(SIGCHLD, sigchld as _);
-            }
             unsafe {
                 // Maybe this should be done outside of this function so nonblocking
                 // isn't forced upon consumers. Although maybe it should be?
@@ -308,9 +254,11 @@ pub fn new<T: ToWinsize>(
             }
 
             let pty = Pty {
-                fd: unsafe {File::from_raw_fd(master) },
-                raw_fd: master,
-                token: mio::Token::from(0)
+                child,
+                fd: unsafe { File::from_raw_fd(master) },
+                token: mio::Token::from(0),
+                signals,
+                signals_token: mio::Token::from(0),
             };
             pty.resize(size);
             pty
@@ -329,32 +277,53 @@ impl EventedReadWrite for Pty {
     fn register(
         &mut self,
         poll: &mio::Poll,
-        token: &mut dyn Iterator<Item = &usize>,
+        token: &mut dyn Iterator<Item = mio::Token>,
         interest: mio::Ready,
         poll_opts: mio::PollOpt,
     ) -> io::Result<()> {
-        self.token = (*token.next().unwrap()).into();
+        self.token = token.next().unwrap();
         poll.register(
-            &EventedFd(&self.raw_fd),
+            &EventedFd(&self.fd.as_raw_fd()),
             self.token,
             interest,
             poll_opts
+        )?;
+
+        self.signals_token = token.next().unwrap();
+        poll.register(
+            &self.signals,
+            self.signals_token,
+            mio::Ready::readable(),
+            mio::PollOpt::level()
         )
     }
 
     #[inline]
-    fn reregister(&mut self, poll: &mio::Poll, interest: mio::Ready, poll_opts: mio::PollOpt) -> io::Result<()> {
+    fn reregister(
+        &mut self,
+        poll: &mio::Poll,
+        interest: mio::Ready,
+        poll_opts: mio::PollOpt
+    ) -> io::Result<()> {
         poll.reregister(
-            &EventedFd(&self.raw_fd),
+            &EventedFd(&self.fd.as_raw_fd()),
             self.token,
             interest,
             poll_opts
+        )?;
+
+        poll.reregister(
+            &self.signals,
+            self.signals_token,
+            mio::Ready::readable(),
+            mio::PollOpt::level()
         )
     }
 
     #[inline]
     fn deregister(&mut self, poll: &mio::Poll) -> io::Result<()> {
-        poll.deregister(&EventedFd(&self.raw_fd))
+        poll.deregister(&EventedFd(&self.fd.as_raw_fd()))?;
+        poll.deregister(&self.signals)
     }
 
     #[inline]
@@ -378,6 +347,38 @@ impl EventedReadWrite for Pty {
     }
 }
 
+impl EventedPty for Pty {
+    #[inline]
+    fn next_child_event(&mut self) -> Option<ChildEvent> {
+        self.signals
+            .pending()
+            .next()
+            .and_then(|signal| {
+                if signal != sighook::SIGCHLD {
+                    return None;
+                }
+
+                match self.child.try_wait() {
+                    Err(e) => {
+                        error!("Error checking child process termination: {}", e);
+                        None
+                    },
+                    Ok(None) => None,
+                    Ok(_)    => Some(ChildEvent::Exited),
+                }
+            })
+    }
+
+    #[inline]
+    fn child_event_token(&self) -> mio::Token {
+        self.signals_token
+    }
+}
+
+pub fn process_should_exit() -> bool {
+    false
+}
+
 /// Types that can produce a `libc::winsize`
 pub trait ToWinsize {
     /// Get a `libc::winsize`
diff --git a/src/tty/windows/mod.rs b/src/tty/windows/mod.rs
index 5e0aee84..2f5caa93 100644
--- a/src/tty/windows/mod.rs
+++ b/src/tty/windows/mod.rs
@@ -28,7 +28,7 @@ use crate::cli::Options;
 use crate::config::Config;
 use crate::display::OnResize;
 use crate::term::SizeInfo;
-use crate::tty::EventedReadWrite;
+use crate::tty::{EventedReadWrite, EventedPty};
 
 mod conpty;
 mod winpty;
@@ -232,12 +232,12 @@ impl<'a> EventedReadWrite for Pty<'a> {
     fn register(
         &mut self,
         poll: &mio::Poll,
-        token: &mut dyn Iterator<Item = &usize>,
+        token: &mut dyn Iterator<Item = mio::Token>,
         interest: mio::Ready,
         poll_opts: mio::PollOpt,
     ) -> io::Result<()> {
-        self.read_token = (*token.next().unwrap()).into();
-        self.write_token = (*token.next().unwrap()).into();
+        self.read_token = token.next().unwrap();
+        self.write_token = token.next().unwrap();
 
         if interest.is_readable() {
             poll.register(
@@ -339,3 +339,5 @@ impl<'a> EventedReadWrite for Pty<'a> {
         self.write_token
     }
 }
+
+impl<'a> EventedPty for Pty<'a> { }