diff --git a/src/exec.rs b/src/exec.rs index f08f70791..92d349bb7 100644 --- a/src/exec.rs +++ b/src/exec.rs @@ -36,7 +36,7 @@ InternalProc, Job, JobGroupRef, Pid, ProcStatus, Process, ProcessType, hup_jobs, is_interactive_session, jobs_requiring_warning_on_exit, no_exec, print_exit_warning_for_jobs, }; -use crate::reader::{reader_run_count, safe_restore_term_mode}; +use crate::reader::{reader_run_count, restore_term_mode}; use crate::redirection::{Dup2List, dup2_list_resolve_chain}; use crate::threads::{ThreadPool, is_forked_child}; use crate::trace::trace_if_enabled_with_args; @@ -449,7 +449,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. - safe_restore_term_mode(); + restore_term_mode(); // Bounce to launch_process. This never returns. safe_launch_process(p, &actual_cmd, &argv, &envp); } diff --git a/src/key.rs b/src/key.rs index b0b0eef12..61f54166b 100644 --- a/src/key.rs +++ b/src/key.rs @@ -1,8 +1,8 @@ use libc::VERASE; use crate::{ - flog::FloggableDebug, localizable_string, reader::safe_get_terminal_mode_on_startup, - wgettext_fmt, wutil::fish_wcstoul, + flog::FloggableDebug, localizable_string, reader::get_terminal_mode_on_startup, wgettext_fmt, + wutil::fish_wcstoul, }; use fish_common::{EscapeFlags, EscapeStringStyle, escape_string}; use fish_fallback::fish_wcwidth; @@ -195,7 +195,7 @@ pub(crate) fn canonicalize_keyed_control_char(c: char) -> char { if c == ' ' { return Space; } - if let Some(tm) = safe_get_terminal_mode_on_startup() { + if let Some(tm) = get_terminal_mode_on_startup() { if c == char::from(tm.c_cc[VERASE]) { return Backspace; } diff --git a/src/panic.rs b/src/panic.rs index 6c310260c..c20be8e73 100644 --- a/src/panic.rs +++ b/src/panic.rs @@ -28,8 +28,10 @@ pub fn panic_handler(main: impl FnOnce() -> i32 + UnwindSafe) -> ! { { return; } - if let Some(at_exit) = AT_EXIT.get() { - (at_exit)(); + if is_main_thread() { + if let Some(at_exit) = AT_EXIT.get() { + (at_exit)(); + } } eprintf!("%s crashed, please report a bug.", get_program_name()); if !is_main_thread() { diff --git a/src/reader/reader.rs b/src/reader/reader.rs index 2e0e43dcb..0c98a530a 100644 --- a/src/reader/reader.rs +++ b/src/reader/reader.rs @@ -103,7 +103,7 @@ }, tty_handoff::{ SCROLL_CONTENT_UP_TERMINFO_CODE, TtyHandoff, XTGETTCAP_QUERY_OS_NAME, - get_tty_protocols_active, initialize_tty_protocols, safe_deactivate_tty_protocols, + deactivate_tty_protocols, get_tty_protocols_active, initialize_tty_protocols, }, wildcard::wildcard_has, wutil::{fstat, perror_nix, wstat}, @@ -173,7 +173,7 @@ fn zeroed_termios() -> Termios { pub static SHELL_MODES: LazyLock> = LazyLock::new(|| Mutex::new(zeroed_termios())); -/// The valid terminal modes on startup. Raw global for async-signal-safe access. +/// The valid terminal modes on startup. static TERMINAL_MODE_ON_STARTUP: OnceLock = OnceLock::new(); /// Mode we use to execute programs. @@ -191,8 +191,8 @@ fn zeroed_termios() -> Termios { /// Set from a signal handler. static EXIT_SIGNAL: AtomicI32 = AtomicI32::new(0); -// 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> { +// Get the terminal mode on startup. +pub fn get_terminal_mode_on_startup() -> Option<&'static libc::termios> { TERMINAL_MODE_ON_STARTUP.get() } @@ -993,7 +993,6 @@ pub fn reader_init(will_restore_foreground_pgroup: bool) { { Ok(modes) => { // Save the initial terminal mode. - // Note this field is read by a signal handler, so do it atomically, with a leaked mode. // TODO: rationalize behavior if initial tcgetattr() fails. TERMINAL_MODE_ON_STARTUP.get_or_init(|| libc::termios::from(modes.clone())); modes @@ -1030,8 +1029,8 @@ pub fn reader_init(will_restore_foreground_pgroup: bool) { } pub fn reader_deinit(restore_foreground_pgroup: bool) { - safe_restore_term_mode(); - safe_deactivate_tty_protocols(); + restore_term_mode(); + deactivate_tty_protocols(); if restore_foreground_pgroup { restore_term_foreground_process_group_for_exit(); } @@ -1040,11 +1039,11 @@ 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 safe_restore_term_mode() { +pub fn restore_term_mode() { if !is_interactive_session() || getpgrp().as_raw() != unsafe { libc::tcgetpgrp(STDIN_FILENO) } { return; } - if let Some(modes) = safe_get_terminal_mode_on_startup() { + if let Some(modes) = get_terminal_mode_on_startup() { unsafe { libc::tcsetattr(STDIN_FILENO, libc::TCSANOW, modes) }; } } @@ -1320,7 +1319,7 @@ pub fn read_generation_count() -> u32 { /// The readers interrupt signal handler. Cancels all currently running blocks. /// This is called from a signal handler! -pub fn reader_handle_sigint() { +pub fn safe_reader_handle_sigint() { INTERRUPTED.store(SIGINT, Ordering::Relaxed); } diff --git a/src/signal.rs b/src/signal.rs index 22b1aee2a..43467874a 100644 --- a/src/signal.rs +++ b/src/signal.rs @@ -1,6 +1,6 @@ use crate::event::{enqueue_signal, is_signal_observed}; use crate::prelude::*; -use crate::reader::{reader_handle_sigint, safe_reader_set_exit_signal}; +use crate::reader::{safe_reader_handle_sigint, safe_reader_set_exit_signal}; use crate::termsize::safe_termsize_invalidate_tty; use crate::topic_monitor::{Generation, GenerationsList, Topic, topic_monitor_principal}; use crate::tty_handoff::safe_mark_tty_invalid; @@ -103,7 +103,7 @@ extern "C" fn fish_signal_handler( if !observed { CANCELLATION_SIGNAL.store(libc::SIGINT, Ordering::Relaxed); } - reader_handle_sigint(); + safe_reader_handle_sigint(); topic_monitor_principal().post(Topic::SigHupIntTerm); } libc::SIGCHLD => { diff --git a/src/tty_handoff.rs b/src/tty_handoff.rs index 740d2eea4..8ff12656a 100644 --- a/src/tty_handoff.rs +++ b/src/tty_handoff.rs @@ -25,7 +25,7 @@ use std::os::fd::BorrowedFd; use std::sync::{ OnceLock, - atomic::{AtomicBool, AtomicPtr, Ordering}, + atomic::{AtomicPtr, Ordering}, }; /// Whether kitty keyboard protocol support is present in the TTY. @@ -104,9 +104,7 @@ enum ProtocolKind { } // Commands to emit to enable or disable TTY protocols. Each of these contains -// the full serialized command sequence as bytes. It's structured in this awkward -// way so that we can use it from a signal handler - no need to allocate or deallocate -// as Kitty support is discovered through tty queries. +// the full serialized command sequence as bytes. struct ProtocolBytes { kitty_keyboard: Box<[u8]>, other: Box<[u8]>, @@ -115,8 +113,6 @@ struct ProtocolBytes { } // The combined set of TTY protocols. -// This is created once at startup and then leaked, so it may be used -// from the SIGTERM handler. struct TtyProtocolsSet { // TTY quirks. quirks: TtyQuirks, @@ -127,10 +123,8 @@ struct TtyProtocolsSet { impl TtyProtocolsSet { // Get commands to enable or disable TTY protocols - // and the KITTY_KEYBOARD_SUPPORTED global variable. - // THIS IS USED FROM A SIGNAL HANDLER. - fn safe_get_commands(&self, enable: bool) -> &[u8] { - let protocol = self.quirks.safe_get_supported_protocol(); + fn get_commands(&self, enable: bool) -> &[u8] { + let protocol = self.quirks.get_supported_protocol(); let cmds = if enable { &self.enablers } else { @@ -156,8 +150,7 @@ fn serialize_commands<'a>(cmds: impl Iterator>) -> Bo impl TtyQuirks { // Determine which keyboard protocol. - // This is used from a signal handler. - fn safe_get_supported_protocol(&self) -> ProtocolKind { + fn get_supported_protocol(&self) -> ProtocolKind { use TtyQuirks::{PreCsiMidnightCommander, PreKittyIterm2, Wezterm}; if *self == PreCsiMidnightCommander { return ProtocolKind::None; @@ -232,7 +225,6 @@ fn get_protocols(self) -> TtyProtocolsSet { } // The global tty protocols. This is set once at startup and not changed thereafter. -// This is an AtomicPtr and not a OnceLock, etc. so that it can be used from a signal handler. static TTY_PROTOCOLS: AtomicPtr = AtomicPtr::new(std::ptr::null_mut()); // Get the TTY protocols, without initializing it. @@ -241,7 +233,7 @@ fn tty_protocols() -> Option<&'static TtyProtocolsSet> { unsafe { TTY_PROTOCOLS.load(Ordering::Acquire).as_ref() } } -// Initialize serialized commands for enabling/disabling TTY protocols in signal handlers. +// Initialize serialized commands for enabling/disabling TTY protocols. pub fn initialize_tty_protocols(vars: &dyn Environment) { // Default missing query responses. KITTY_KEYBOARD_SUPPORTED.get_or_init(|| false); @@ -264,14 +256,13 @@ pub fn initialize_tty_protocols(vars: &dyn Environment) { } // A marker of the current state of the tty protocols. -static TTY_PROTOCOLS_ACTIVE: AtomicBool = AtomicBool::new(false); +static TTY_PROTOCOLS_ACTIVE: RelaxedAtomicBool = RelaxedAtomicBool::new(false); // A marker that the tty has been closed (SIGHUP, etc) and so we should not try to write to it. static TTY_INVALID: RelaxedAtomicBool = RelaxedAtomicBool::new(false); // Enable or disable TTY protocols by writing the appropriate commands to the tty. -// Return true if we emitted any bytes to the tty. -// Note this does NOT intialize the TTY protocls if not already initialized. +// Note this does NOT intialize the TTY protocols if not already initialized. fn set_tty_protocols_active(on_write: fn(), enable: bool) { assert_is_main_thread(); // Have protocols at all? We require someone else to have initialized them. @@ -281,11 +272,11 @@ fn set_tty_protocols_active(on_write: fn(), enable: bool) { // Already set? // Note we don't need atomic swaps as this is only called on the main thread. // Also note we (logically) set and clear this even if we got SIGHUP. - if TTY_PROTOCOLS_ACTIVE.load(Ordering::Relaxed) == enable { + if TTY_PROTOCOLS_ACTIVE.load() == enable { return; } if enable { - TTY_PROTOCOLS_ACTIVE.store(true, Ordering::Release); + TTY_PROTOCOLS_ACTIVE.store(true); } // Did we get SIGHUP? @@ -294,15 +285,15 @@ fn set_tty_protocols_active(on_write: fn(), enable: bool) { } // Write the commands to the tty, ignoring errors. - let commands = protocols.safe_get_commands(enable); + let commands = protocols.get_commands(enable); let _ = write_loop(&libc::STDOUT_FILENO, commands); if !enable { - TTY_PROTOCOLS_ACTIVE.store(false, Ordering::Relaxed); + TTY_PROTOCOLS_ACTIVE.store(false); } // Flog any terminal protocol changes of interest. let mode = if enable { "Enabling" } else { "Disabling" }; - match protocols.quirks.safe_get_supported_protocol() { + match protocols.quirks.get_supported_protocol() { ProtocolKind::KittyKeyboard => flog!(reader, mode, "kitty keyboard protocol"), ProtocolKind::Other => flog!(reader, mode, "other extended keys"), ProtocolKind::WorkAroundWezTerm => flog!(reader, mode, "wezterm; no modifyOtherKeys"), @@ -313,19 +304,21 @@ fn set_tty_protocols_active(on_write: fn(), enable: bool) { // Helper to check if TTY protocols are active. pub fn get_tty_protocols_active() -> bool { - TTY_PROTOCOLS_ACTIVE.load(Ordering::Relaxed) + TTY_PROTOCOLS_ACTIVE.load() } -// Called from a signal handler to deactivate TTY protocols before exiting. -// Only async-signal-safe code can be run here. -pub fn safe_deactivate_tty_protocols() { +// Deactivate TTY protocols before exiting. +pub fn deactivate_tty_protocols() { + if !cfg!(test) { + assert_is_main_thread(); + } // Safety: TTY_PROTOCOLS is never modified after initialization. let protocols = unsafe { TTY_PROTOCOLS.load(Ordering::Acquire).as_ref() }; let Some(protocols) = protocols else { // No protocols set, nothing to do. return; }; - if !TTY_PROTOCOLS_ACTIVE.load(Ordering::Acquire) { + if !TTY_PROTOCOLS_ACTIVE.load() { return; } @@ -334,10 +327,10 @@ pub fn safe_deactivate_tty_protocols() { return; } - let commands = protocols.safe_get_commands(false); + let commands = protocols.get_commands(false); // Safety: just writing data to stdout. let _ = write_loop(&libc::STDOUT_FILENO, commands); - TTY_PROTOCOLS_ACTIVE.store(false, Ordering::Release); + TTY_PROTOCOLS_ACTIVE.store(false); } // Called from a signal handler to mark the tty as invalid (e.g. SIGHUP). diff --git a/src/wutil/mod.rs b/src/wutil/mod.rs index ec3116b4c..4c261b458 100644 --- a/src/wutil/mod.rs +++ b/src/wutil/mod.rs @@ -7,7 +7,7 @@ pub mod wcstod; pub mod wcstoi; -use crate::{fds::BorrowedFdFile, flog, signal::SigChecker, topic_monitor::Topic}; +use crate::{fds::BorrowedFdFile, flog, signal::SigChecker}; use errno::{Errno, set_errno}; use fish_util::{perror, write_to_fd}; use fish_wcstringutil::join_strings;