Restore some async-signal discipline to SIGTERM

The changes to enable terminal protocols (CSI-U, etc) also attempts to
re-disable these when fish exits. In particular it attempts to disable these
from a SIGTERM handler.

Unfortunately none of that machinery is async-signal safe. Indeed our SIGTERM
handler has gotten rather sketchy, with taking a mutex and some other stuff.

Remove the async-signal-unsafe stuff and make SIGTERM manifestly safe.
Unfortunately this means that terminal protocols will remain set after SIGTERM
but that's probably unavoidable.

src/exec.rs

@@ -40,7 +40,7 @@
     print_exit_warning_for_jobs, InternalProc, Job, JobGroupRef, ProcStatus, Process, ProcessType,
     TtyTransfer,
 };
-use crate::reader::{reader_run_count, restore_term_mode};
+use crate::reader::{reader_run_count, safe_restore_term_mode};
 use crate::redirection::{dup2_list_resolve_chain, Dup2List};
 use crate::threads::{iothread_perform_cant_wait, is_forked_child};
 use crate::trace::trace_if_enabled_with_args;
@@ -437,7 +437,7 @@ fn launch_process_nofork(vars: &EnvStack, p: &Process) -> ! {
     let actual_cmd = wcs2zstring(&p.actual_cmd);
 
     // Ensure the terminal modes are what they were before we changed them.
-    restore_term_mode();
+    safe_restore_term_mode();
     // Bounce to launch_process. This never returns.
     safe_launch_process(p, &actual_cmd, &argv, &envp);
 }

src/key.rs

@@ -5,7 +5,7 @@
     fallback::fish_wcwidth,
     flog::FloggableDebug,
     future_feature_flags::{test as feature_test, FeatureFlag},
-    reader::TERMINAL_MODE_ON_STARTUP,
+    reader::safe_get_terminal_mode_on_startup,
     wchar::{decode_byte_from_char, prelude::*},
     wutil::{fish_is_pua, fish_wcstoul},
 };
@@ -179,8 +179,10 @@ pub(crate) fn canonicalize_keyed_control_char(c: char) -> char {
     if c == ' ' {
         return Space;
     }
-    if c == char::from(TERMINAL_MODE_ON_STARTUP.lock().unwrap().c_cc[VERASE]) {
+    if let Some(tm) = safe_get_terminal_mode_on_startup() {
-        return Backspace;
+        if c == char::from(tm.c_cc[VERASE]) {
+            return Backspace;
+        }
     }
     if c == char::from(127) {
         // when it's not backspace

src/reader.rs

@@ -39,8 +39,7 @@
 use std::rc::Rc;
 #[cfg(target_has_atomic = "64")]
 use std::sync::atomic::AtomicU64;
-use std::sync::atomic::Ordering;
+use std::sync::atomic::{AtomicI32, AtomicPtr, AtomicU32, AtomicU8, Ordering};
-use std::sync::atomic::{AtomicI32, AtomicU32, AtomicU8};
 use std::sync::{Arc, Mutex, MutexGuard};
 use std::time::{Duration, Instant};
 
@@ -177,9 +176,9 @@ enum ExitState {
 pub static SHELL_MODES: Lazy<Mutex<libc::termios>> =
     Lazy::new(|| Mutex::new(unsafe { std::mem::zeroed() }));
 
-/// Mode on startup, which we restore on exit.
+/// The valid terminal modes on startup. This is set once and not modified after.
-pub static TERMINAL_MODE_ON_STARTUP: Lazy<Mutex<libc::termios>> =
+/// Warning: this is read from the SIGTERM handler! Hence the raw global.
-    Lazy::new(|| Mutex::new(unsafe { std::mem::zeroed() }));
+static TERMINAL_MODE_ON_STARTUP: AtomicPtr<libc::termios> = AtomicPtr::new(std::ptr::null_mut());
 
 /// Mode we use to execute programs.
 static TTY_MODES_FOR_EXTERNAL_CMDS: Lazy<Mutex<libc::termios>> =
@@ -196,6 +195,12 @@ enum ExitState {
 /// This is set from a signal handler.
 static SIGHUP_RECEIVED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
 
+// Get the terminal mode on startup. This is "safe" because it's async-signal safe.
+pub fn safe_get_terminal_mode_on_startup() -> Option<&'static libc::termios> {
+    // Safety: set atomically and not modified after.
+    unsafe { TERMINAL_MODE_ON_STARTUP.load(Ordering::Acquire).as_ref() }
+}
+
 /// A singleton snapshot of the reader state. This is factored out for thread-safety reasons:
 /// it may be fetched on a background thread.
 fn commandline_state_snapshot() -> MutexGuard<'static, CommandlineState> {
@@ -857,8 +862,15 @@ fn read_ni(parser: &Parser, fd: RawFd, io: &IoChain) -> Result<(), ErrorCode> {
 /// Initialize the reader.
 pub fn reader_init(will_restore_foreground_pgroup: bool) {
     // Save the initial terminal mode.
-    let mut terminal_mode_on_startup = TERMINAL_MODE_ON_STARTUP.lock().unwrap();
+    // Note this field is read by a signal handler, so do it atomically, with a leaked mode.
-    unsafe { libc::tcgetattr(STDIN_FILENO, &mut *terminal_mode_on_startup) };
+    let mut terminal_mode_on_startup = unsafe { std::mem::zeroed::<libc::termios>() };
+    let ret = unsafe { libc::tcgetattr(libc::STDIN_FILENO, &mut terminal_mode_on_startup) };
+    // TODO: rationalize behavior if initial tcgetattr() fails.
+    if ret == 0 {
+        // Must be mut because AtomicPtr doesn't have const variant.
+        let leaked: *mut libc::termios = Box::leak(Box::new(terminal_mode_on_startup));
+        TERMINAL_MODE_ON_STARTUP.store(leaked, Ordering::Release);
+    }
 
     #[cfg(not(test))]
     assert!(AT_EXIT.get().is_none());
@@ -866,7 +878,7 @@ pub fn reader_init(will_restore_foreground_pgroup: bool) {
 
     // Set the mode used for program execution, initialized to the current mode.
     let mut tty_modes_for_external_cmds = TTY_MODES_FOR_EXTERNAL_CMDS.lock().unwrap();
-    *tty_modes_for_external_cmds = *terminal_mode_on_startup;
+    *tty_modes_for_external_cmds = terminal_mode_on_startup;
     term_fix_external_modes(&mut tty_modes_for_external_cmds);
 
     // Disable flow control by default.
@@ -878,7 +890,6 @@ pub fn reader_init(will_restore_foreground_pgroup: bool) {
 
     term_fix_modes(&mut shell_modes());
 
-    drop(terminal_mode_on_startup);
     drop(tty_modes_for_external_cmds);
 
     // Set up our fixed terminal modes once,
@@ -893,7 +904,7 @@ pub fn reader_init(will_restore_foreground_pgroup: bool) {
 // TODO(pca): this is run in our "AT_EXIT" handler from a SIGTERM handler.
 // It must be made async-signal-safe (or not invoked).
 pub fn reader_deinit(restore_foreground_pgroup: bool) {
-    restore_term_mode();
+    safe_restore_term_mode();
     crate::input_common::terminal_protocols_disable_ifn();
     if restore_foreground_pgroup {
         restore_term_foreground_process_group_for_exit();
@@ -903,18 +914,14 @@ pub fn reader_deinit(restore_foreground_pgroup: bool) {
 /// Restore the term mode if we own the terminal and are interactive (#8705).
 /// It's important we do this before restore_foreground_process_group,
 /// otherwise we won't think we own the terminal.
-pub fn restore_term_mode() {
+/// THIS FUNCTION IS CALLED FROM A SIGNAL HANDLER. IT MUST BE ASYNC-SIGNAL-SAFE.
+pub fn safe_restore_term_mode() {
     if !is_interactive_session() || getpgrp() != unsafe { libc::tcgetpgrp(STDIN_FILENO) } {
         return;
     }
-
+    if let Some(modes) = safe_get_terminal_mode_on_startup() {
-    unsafe {
+        unsafe { libc::tcsetattr(STDIN_FILENO, TCSANOW, modes) };
-        libc::tcsetattr(
+    }
-            STDIN_FILENO,
-            TCSANOW,
-            &*TERMINAL_MODE_ON_STARTUP.lock().unwrap(),
-        )
-    };
 }
 
 /// Change the history file for the current command reading context.

src/signal.rs

@@ -4,8 +4,7 @@
 use crate::common::exit_without_destructors;
 use crate::event::{enqueue_signal, is_signal_observed};
 use crate::nix::getpid;
-use crate::panic::AT_EXIT;
+use crate::reader::{reader_handle_sigint, reader_sighup, safe_restore_term_mode};
-use crate::reader::{reader_handle_sigint, reader_sighup};
 use crate::termsize::TermsizeContainer;
 use crate::topic_monitor::{topic_monitor_principal, Generation, GenerationsList, Topic};
 use crate::wchar::prelude::*;
@@ -90,9 +89,7 @@ extern "C" fn fish_signal_handler(
         libc::SIGTERM => {
             // Handle sigterm. The only thing we do is restore the front process ID, then die.
             if !observed {
-                if let Some(at_exit) = AT_EXIT.get() {
+                safe_restore_term_mode();
-                    (at_exit)();
-                }
                 // Safety: signal() and raise() are async-signal-safe.
                 unsafe {
                     libc::signal(libc::SIGTERM, libc::SIG_DFL);