From 0c8f1f42206bf389162100f4ef0fe5d43c992b71 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Thu, 17 Jul 2025 08:44:32 +0200 Subject: [PATCH] Fix async-signal safety in SIGTERM handler Cherry-picked from - 941701da3d8 (Restore some async-signal discipline to SIGTERM, 2025-06-15) - 81d45caa76e (Restore terminal state on SIGTERM again, 2025-06-21) Also, be more careful in terminal_protocols_disable_ifn about accessing reader_current_data(), as pointed out in 65a4cb5245a (Revert "Restore terminal state on SIGTERM again", 2025-07-19). See #11597 --- src/builtins/fg.rs | 2 +- src/builtins/read.rs | 2 +- src/exec.rs | 4 ++-- src/input_common.rs | 4 ++-- src/key.rs | 8 ++++--- src/parser.rs | 2 +- src/reader.rs | 54 +++++++++++++++++++++++++------------------- 7 files changed, 43 insertions(+), 33 deletions(-) diff --git a/src/builtins/fg.rs b/src/builtins/fg.rs index 675a06ebd..112222ffa 100644 --- a/src/builtins/fg.rs +++ b/src/builtins/fg.rs @@ -148,7 +148,7 @@ pub fn fg(parser: &Parser, streams: &mut IoStreams, argv: &mut [&wstr]) -> Optio let job_group = job.group(); job_group.set_is_foreground(true); if job.entitled_to_terminal() { - crate::input_common::terminal_protocols_disable_ifn(); + crate::input_common::terminal_protocols_disable_ifn(false); } let tmodes = job_group.tmodes.borrow(); if job_group.wants_terminal() && tmodes.is_some() { diff --git a/src/builtins/read.rs b/src/builtins/read.rs index 1ec26c5f0..7c9d38cac 100644 --- a/src/builtins/read.rs +++ b/src/builtins/read.rs @@ -248,7 +248,7 @@ fn read_interactive( reader_readline(parser, nchars) }; - terminal_protocols_disable_ifn(); + terminal_protocols_disable_ifn(false); if let Some(line) = mline { *buff = line; if nchars > 0 && nchars < buff.len() { diff --git a/src/exec.rs b/src/exec.rs index 0b9e9b7ba..b52f6259e 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -39,7 +39,7 @@ print_exit_warning_for_jobs, InternalProc, Job, JobGroupRef, Pid, 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(false); + safe_restore_term_mode(false); // Bounce to launch_process. This never returns. safe_launch_process(p, &actual_cmd, &argv, &*envp); } diff --git a/src/input_common.rs b/src/input_common.rs index 469f3784f..c94d8052c 100644 --- a/src/input_common.rs +++ b/src/input_common.rs @@ -749,7 +749,7 @@ pub fn terminal_protocols_enable_ifn() { reader_current_data().map(|data| data.save_screen_state()); } -pub(crate) fn terminal_protocols_disable_ifn() { +pub(crate) fn terminal_protocols_disable_ifn(in_signal_handler: bool) { if !TERMINAL_PROTOCOLS.load(Ordering::Acquire) { return; } @@ -784,7 +784,7 @@ pub(crate) fn terminal_protocols_disable_ifn() { if IS_TMUX.load() { let _ = write_loop(&STDOUT_FILENO, "\x1b[?1004l".as_bytes()); } - if is_main_thread() { + if !in_signal_handler && is_main_thread() { reader_current_data().map(|data| data.save_screen_state()); } TERMINAL_PROTOCOLS.store(false, Ordering::Release); diff --git a/src/key.rs b/src/key.rs index 9ae9a09ca..2a7cad1b6 100644 --- a/src/key.rs +++ b/src/key.rs @@ -3,7 +3,7 @@ use crate::{ common::{escape_string, EscapeFlags, EscapeStringStyle}, fallback::fish_wcwidth, - reader::TERMINAL_MODE_ON_STARTUP, + reader::safe_get_terminal_mode_on_startup, wchar::{decode_byte_from_char, prelude::*}, wutil::{fish_is_pua, fish_wcstoul}, }; @@ -169,8 +169,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]) { - return Backspace; + if let Some(tm) = safe_get_terminal_mode_on_startup() { + if c == char::from(tm.c_cc[VERASE]) { + return Backspace; + } } if c == char::from(127) { // when it's not backspace diff --git a/src/parser.rs b/src/parser.rs index 7afca7f58..7f96f4445 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -614,7 +614,7 @@ pub fn eval_node( let mut execution_context = ExecutionContext::new(ps.clone(), block_io.clone(), Rc::clone(&line_counter)); - terminal_protocols_disable_ifn(); + terminal_protocols_disable_ifn(false); // Check the exec count so we know if anything got executed. let prev_exec_count = self.libdata().exec_count; diff --git a/src/reader.rs b/src/reader.rs index 68b450871..496982ad3 100644 --- a/src/reader.rs +++ b/src/reader.rs @@ -34,8 +34,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, AtomicU32, AtomicU8}; +use std::sync::atomic::{AtomicI32, AtomicPtr, AtomicU32, AtomicU8, Ordering}; use std::sync::{Arc, Mutex, MutexGuard}; use std::time::{Duration, Instant}; @@ -152,9 +151,9 @@ enum ExitState { pub static SHELL_MODES: Lazy> = Lazy::new(|| Mutex::new(unsafe { std::mem::zeroed() })); -/// Mode on startup, which we restore on exit. -pub static TERMINAL_MODE_ON_STARTUP: Lazy> = - Lazy::new(|| Mutex::new(unsafe { std::mem::zeroed() })); +/// The valid terminal modes on startup. This is set once and not modified after. +/// Warning: this is read from the SIGTERM handler! Hence the raw global. +static TERMINAL_MODE_ON_STARTUP: AtomicPtr = AtomicPtr::new(std::ptr::null_mut()); /// Mode we use to execute programs. static TTY_MODES_FOR_EXTERNAL_CMDS: Lazy> = @@ -171,6 +170,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> { @@ -813,8 +818,15 @@ fn read_ni(parser: &Parser, fd: RawFd, io: &IoChain) -> i32 { /// 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(); - unsafe { libc::tcgetattr(STDIN_FILENO, &mut *terminal_mode_on_startup) }; + // Note this field is read by a signal handler, so do it atomically, with a leaked mode. + let mut terminal_mode_on_startup = unsafe { std::mem::zeroed::() }; + 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()); @@ -826,7 +838,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. @@ -838,7 +850,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, @@ -850,9 +861,11 @@ 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(in_signal_handler: bool, restore_foreground_pgroup: bool) { - restore_term_mode(in_signal_handler); - crate::input_common::terminal_protocols_disable_ifn(); + safe_restore_term_mode(in_signal_handler); + crate::input_common::terminal_protocols_disable_ifn(in_signal_handler); if restore_foreground_pgroup { restore_term_foreground_process_group_for_exit(); } @@ -861,20 +874,15 @@ pub fn reader_deinit(in_signal_handler: bool, 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(in_signal_handler: bool) { +/// THIS FUNCTION IS CALLED FROM A SIGNAL HANDLER. IT MUST BE ASYNC-SIGNAL-SAFE. +pub fn safe_restore_term_mode(in_signal_handler: bool) { if !is_interactive_session() || unsafe { libc::getpgrp() != libc::tcgetpgrp(STDIN_FILENO) } { return; } - - if unsafe { - libc::tcsetattr( - STDIN_FILENO, - TCSANOW, - &*TERMINAL_MODE_ON_STARTUP.lock().unwrap(), - ) == -1 - } && errno().0 == EIO - { - redirect_tty_output(in_signal_handler); + if let Some(modes) = safe_get_terminal_mode_on_startup() { + if unsafe { libc::tcsetattr(STDIN_FILENO, TCSANOW, modes) == -1 } && errno().0 == EIO { + redirect_tty_output(in_signal_handler); + } } } @@ -5812,7 +5820,7 @@ fn compute_and_apply_completions(&mut self, c: ReadlineCmd) { token_range.end += cmdsub_range.start; // Wildcard expansion and completion below check for cancellation. - terminal_protocols_disable_ifn(); + terminal_protocols_disable_ifn(false); // Check if we have a wildcard within this string; if so we first attempt to expand the // wildcard; if that succeeds we don't then apply user completions (#8593).