h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-05-21 19:41:14 -03:00
Code Issues Packages Projects Releases Wiki Activity

Save history on SIGTERM and SIGHUP before exit

Browse Source 
Previously, SIGTERM immediately re-raised with SIG_DFL, killing
fish without saving history. SIGHUP deferred via a flag but never
re-raised, so the parent saw a normal exit instead of signal death.

Unify both signals: the handler stores the signal number in a single
AtomicI32, the reader loop exits normally, throwing_main() saves
history and re-raises with SIG_DFL so the parent sees WIFSIGNALED.

Fixes #10300

Closes #12615
This commit is contained in:
Yakov Till
2026-04-03 07:29:44 +02:00
committed by Johannes Altmanninger
parent 8ae71c80f4
commit b99ae291d6
10 changed files with 115 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.rst
View File

@@ -15,6 +15,7 @@ Improved terminal support
Other improvements
------------------
- History is no longer corrupted with NUL bytes when fish receives SIGTERM or SIGHUP (:issue:`10300`).
- ``fish_update_completions`` now handles groff ``\X'...'`` device control escapes, fixing completion generation for man pages produced by help2man 1.50 and later (such as coreutils 9.10).
For distributors and developers

12
src/bin/fish.rs
View File

@@ -50,7 +50,7 @@
Pid, get_login, is_interactive_session, mark_login, mark_no_exec, proc_init,
set_interactive_session,
},
reader::{reader_init, reader_read, term_copy_modes},
reader::{reader_exit_signal, reader_init, reader_read, term_copy_modes},
signal::{signal_clear_cancel, signal_unblock_all},
threads::{self},
topic_monitor,
@@ -625,6 +625,16 @@ fn throwing_main() -> i32 {
}
history::save_all();
// If we deferred a fatal signal, re-raise it now so the parent sees WIFSIGNALED.
let exit_sig = reader_exit_signal();
if exit_sig != 0 {
unsafe {
libc::signal(exit_sig, libc::SIG_DFL);
libc::raise(exit_sig);
}
}
if opts.print_rusage_self {
print_rusage_self();
}

5
src/builtins/fish_key_reader.rs
View File

@@ -27,7 +27,8 @@
print_help::print_help,
proc::set_interactive_session,
reader::{
check_exit_loop_maybe_warning, reader_init, reader_sighup, set_shell_modes, terminal_init,
check_exit_loop_maybe_warning, reader_init, safe_reader_set_exit_signal, set_shell_modes,
terminal_init,
},
threads,
topic_monitor::topic_monitor_init,
@@ -97,7 +98,7 @@ fn process_input(
use QueryResultEvent::*;
let kevt = match input_queue.readch() {
CharEvent::Implicit(ImplicitEvent::Eof) => {
reader_sighup();
safe_reader_set_exit_signal(libc::SIGHUP);
continue;
}
CharEvent::Key(kevt) => kevt,

2
src/builtins/wait.rs
View File

@@ -107,7 +107,7 @@ fn wait_for_completion(parser: &Parser, whs: &[WaitHandleRef], any_flag: bool) -
return Ok(SUCCESS);
}
let mut sigint = SigChecker::new_sighupint();
let mut sigint = SigChecker::new_sighupintterm();
loop {
let finished = if any_flag {
whs.iter().any(is_completed)

8
src/proc.rs
View File

@@ -1214,12 +1214,12 @@ fn process_mark_finished_children(parser: &Parser, block_ok: bool, block_io: Opt
if proc.has_pid() {
// Reaps with a pid.
reapgens.set_min_from(Topic::SigChld, &proc.gens);
reapgens.set_min_from(Topic::SigHupInt, &proc.gens);
reapgens.set_min_from(Topic::SigHupIntTerm, &proc.gens);
}
if proc.internal_proc.borrow().is_some() {
// Reaps with an internal process.
reapgens.set_min_from(Topic::InternalExit, &proc.gens);
reapgens.set_min_from(Topic::SigHupInt, &proc.gens);
reapgens.set_min_from(Topic::SigHupIntTerm, &proc.gens);
}
}
}
@@ -1242,7 +1242,7 @@ fn process_mark_finished_children(parser: &Parser, block_ok: bool, block_io: Opt
}
// Always update the signal hup/int gen.
proc.gens.sighupint.set(reapgens.sighupint.get());
proc.gens.sighupintterm.set(reapgens.sighupintterm.get());
// Nothing to do if we did not get a new sigchld.
if proc.gens.sigchld == reapgens.sigchld {
@@ -1311,7 +1311,7 @@ fn process_mark_finished_children(parser: &Parser, block_ok: bool, block_io: Opt
}
// Always update the signal hup/int gen.
proc.gens.sighupint.set(reapgens.sighupint.get());
proc.gens.sighupintterm.set(reapgens.sighupintterm.get());
// Nothing to do if we did not get a new internal exit.
if proc.gens.internal_exit == reapgens.internal_exit {

46
src/reader/reader.rs
View File

@@ -173,8 +173,7 @@ fn zeroed_termios() -> Termios {
pub static SHELL_MODES: LazyLock<Mutex<Termios>> = LazyLock::new(|| Mutex::new(zeroed_termios()));
/// The valid terminal modes on startup.
/// Warning: this is read from the SIGTERM handler! Hence the raw global.
/// The valid terminal modes on startup. Raw global for async-signal-safe access.
static TERMINAL_MODE_ON_STARTUP: OnceLock<libc::termios> = OnceLock::new();
/// Mode we use to execute programs.
@@ -188,9 +187,9 @@ fn zeroed_termios() -> Termios {
/// This variable is set to a signal by the signal handler when ^C is pressed.
static INTERRUPTED: AtomicI32 = AtomicI32::new(0);
/// If set, SIGHUP has been received. This latches to true.
/// This is set from a signal handler.
static SIGHUP_RECEIVED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
/// Stores the signal (SIGHUP or SIGTERM) that should cause fish to exit, or 0 if none.
/// 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> {
@@ -212,8 +211,7 @@ fn redirect_tty_after_sighup() {
use std::fs::OpenOptions;
// If we have received SIGHUP, redirect the tty to avoid a user script triggering SIGTTIN or
// SIGTTOU.
assert!(reader_received_sighup(), "SIGHUP not received");
// SIGTTOU. The caller checks reader_exit_signal() == SIGHUP before calling this.
static TTY_REDIRECTED: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
if TTY_REDIRECTED.swap(true) {
return;
@@ -286,7 +284,7 @@ pub fn terminal_init(vars: &dyn Environment, inputfd: RawFd) -> TerminalInitResu
use ImplicitEvent::{CheckExit, Eof};
use QueryResultEvent::*;
match input_queue.readch() {
Implicit(Eof) => reader_sighup(),
Implicit(Eof) => safe_reader_set_exit_signal(libc::SIGHUP),
Implicit(CheckExit) => {}
CharEvent::QueryResult(Response(QueryResponse::PrimaryDeviceAttribute)) => {
break;
@@ -891,9 +889,8 @@ fn read_i(parser: &Parser) {
reader_pop();
// If we got SIGHUP, ensure the tty is redirected and release tty handoff without
// trying to muck with protocols.
if reader_received_sighup() {
// If we are the top-level reader, then we translate SIGHUP into exit_forced.
// trying to muck with protocols. SIGTERM does not redirect; the terminal is still valid.
if reader_exit_signal() == libc::SIGHUP {
redirect_tty_after_sighup();
}
@@ -1043,7 +1040,6 @@ 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.
/// 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().as_raw() != unsafe { libc::tcgetpgrp(STDIN_FILENO) } {
return;
@@ -1345,14 +1341,20 @@ pub fn reader_test_and_clear_interrupted() -> i32 {
res
}
/// Mark that we encountered SIGHUP and must (soon) exit. This is invoked from a signal handler.
pub fn reader_sighup() {
/// Mark that we received an exit signal (SIGHUP or SIGTERM). Invoked from a signal handler.
pub fn safe_reader_set_exit_signal(sig: i32) {
// Beware, we may be in a signal handler.
SIGHUP_RECEIVED.store(true);
EXIT_SIGNAL.store(sig, Ordering::Relaxed);
}
fn reader_received_sighup() -> bool {
SIGHUP_RECEIVED.load()
/// Return the exit signal we received, or 0 if none.
pub fn reader_exit_signal() -> i32 {
EXIT_SIGNAL.load(Ordering::Relaxed)
}
/// Whether we received SIGHUP or SIGTERM and should exit.
fn reader_received_exit_signal() -> bool {
reader_exit_signal() != 0
}
impl ReaderData {
@@ -2872,7 +2874,7 @@ fn handle_char_event(&mut self, injected_event: Option<CharEvent>) -> ControlFlo
CharEvent::Implicit(implicit_event) => {
use ImplicitEvent::*;
match implicit_event {
Eof => reader_sighup(),
Eof => safe_reader_set_exit_signal(libc::SIGHUP),
CheckExit => (),
FocusIn => {
event::fire_generic(self.parser, L!("fish_focus_in").to_owned(), vec![]);
@@ -5013,10 +5015,7 @@ pub fn fish_is_unwinding_for_exit() -> bool {
let exit_state = EXIT_STATE.load(Ordering::Relaxed);
let exit_state: ExitState = unsafe { std::mem::transmute(exit_state) };
match exit_state {
ExitState::None => {
// Cancel if we got SIGHUP.
reader_received_sighup()
}
ExitState::None => reader_received_exit_signal(),
ExitState::RunningHandlers => {
// We intend to exit but we want to allow these handlers to run.
false
@@ -6517,8 +6516,7 @@ fn try_warn_on_background_jobs(&mut self) -> bool {
/// Check if we should exit the reader loop.
/// Return true if we should exit.
pub fn check_exit_loop_maybe_warning(data: Option<&mut Reader>) -> bool {
// sighup always forces exit.
if reader_received_sighup() {
if reader_received_exit_signal() {
return true;
}

32
src/signal.rs
View File

@@ -1,9 +1,9 @@
use crate::event::{enqueue_signal, is_signal_observed};
use crate::prelude::*;
use crate::reader::{reader_handle_sigint, reader_sighup, safe_restore_term_mode};
use crate::reader::{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_deactivate_tty_protocols, safe_mark_tty_invalid};
use crate::tty_handoff::safe_mark_tty_invalid;
use crate::wutil::fish_wcstoi;
use errno::{errno, set_errno};
use fish_common::exit_without_destructors;
@@ -88,25 +88,15 @@ extern "C" fn fish_signal_handler(
// Respond to a winch signal by telling the termsize container.
safe_termsize_invalidate_tty();
}
libc::SIGHUP => {
libc::SIGHUP | libc::SIGTERM => {
// Exit unless the signal was trapped.
if !observed {
reader_sighup();
safe_mark_tty_invalid();
}
topic_monitor_principal().post(Topic::SigHupInt);
}
libc::SIGTERM => {
// Handle sigterm. The only thing we do is restore the front process ID and disable protocols, then die.
if !observed {
safe_restore_term_mode();
safe_deactivate_tty_protocols();
// Safety: signal() and raise() are async-signal-safe.
unsafe {
libc::signal(libc::SIGTERM, libc::SIG_DFL);
libc::raise(libc::SIGTERM);
safe_reader_set_exit_signal(sig);
if sig == libc::SIGHUP {
safe_mark_tty_invalid();
}
}
topic_monitor_principal().post(Topic::SigHupIntTerm);
}
libc::SIGINT => {
// Cancel unless the signal was trapped.
@@ -114,7 +104,7 @@ extern "C" fn fish_signal_handler(
CANCELLATION_SIGNAL.store(libc::SIGINT, Ordering::Relaxed);
}
reader_handle_sigint();
topic_monitor_principal().post(Topic::SigHupInt);
topic_monitor_principal().post(Topic::SigHupIntTerm);
}
libc::SIGCHLD => {
// A child process stopped or exited.
@@ -177,7 +167,7 @@ fn set_interactive_handlers() {
act.sa_flags = libc::SA_SIGINFO;
sigaction(libc::SIGTTIN, &act, nullptr);
// SIGTERM restores the terminal controlling process before dying.
// SIGTERM defers to allow graceful history save before exit.
act.sa_sigaction = signal_handler;
act.sa_flags = libc::SA_SIGINFO;
sigaction(libc::SIGTERM, &act, nullptr);
@@ -324,8 +314,8 @@ pub fn new(topic: Topic) -> Self {
}
/// Create a new checker for SIGHUP and SIGINT.
pub fn new_sighupint() -> Self {
Self::new(Topic::SigHupInt)
pub fn new_sighupintterm() -> Self {
Self::new(Topic::SigHupIntTerm)
}
/// Check if a sigint has been delivered since the last call to check(), or since the detector

22
src/topic_monitor.rs
View File

@@ -38,15 +38,15 @@
#[repr(u8)]
#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub enum Topic {
SigHupInt = 0, // Corresponds to both SIGHUP and SIGINT signals.
SigChld = 1, // Corresponds to SIGCHLD signal.
InternalExit = 2, // Corresponds to an internal process exit.
SigHupIntTerm = 0, // Corresponds to both SIGHUP and SIGINT signals.
SigChld = 1, // Corresponds to SIGCHLD signal.
InternalExit = 2, // Corresponds to an internal process exit.
}
// XXX: Is it correct to use the default or should the default be invalid_generation?
#[derive(Clone, Debug, Default, PartialEq, PartialOrd, Eq, Ord)]
pub struct GenerationsList {
pub sighupint: Cell<u64>,
pub sighupintterm: Cell<u64>,
pub sigchld: Cell<u64>,
pub internal_exit: Cell<u64>,
}
@@ -56,7 +56,7 @@ pub struct GenerationsList {
impl GenerationsList {
/// Update `self` gen counts to match those of `other`.
pub fn update(&self, other: &Self) {
self.sighupint.set(other.sighupint.get());
self.sighupintterm.set(other.sighupintterm.get());
self.sigchld.set(other.sigchld.get());
self.internal_exit.set(other.internal_exit.get());
}
@@ -70,7 +70,7 @@ impl FloggableDebug for Topic {}
pub const INVALID_GENERATION: Generation = u64::MAX;
pub fn all_topics() -> [Topic; 3] {
[Topic::SigHupInt, Topic::SigChld, Topic::InternalExit]
[Topic::SigHupIntTerm, Topic::SigChld, Topic::InternalExit]
}
impl GenerationsList {
@@ -81,7 +81,7 @@ pub fn new() -> Self {
/// Generation list containing invalid generations only.
pub fn invalid() -> GenerationsList {
GenerationsList {
sighupint: INVALID_GENERATION.into(),
sighupintterm: INVALID_GENERATION.into(),
sigchld: INVALID_GENERATION.into(),
internal_exit: INVALID_GENERATION.into(),
}
@@ -106,7 +106,7 @@ fn describe(&self) -> WString {
/// Sets the generation for `topic` to `value`.
pub fn set(&self, topic: Topic, value: Generation) {
match topic {
Topic::SigHupInt => self.sighupint.set(value),
Topic::SigHupIntTerm => self.sighupintterm.set(value),
Topic::SigChld => self.sigchld.set(value),
Topic::InternalExit => self.internal_exit.set(value),
}
@@ -115,7 +115,7 @@ pub fn set(&self, topic: Topic, value: Generation) {
/// Return the value for a topic.
pub fn get(&self, topic: Topic) -> Generation {
match topic {
Topic::SigHupInt => self.sighupint.get(),
Topic::SigHupIntTerm => self.sighupintterm.get(),
Topic::SigChld => self.sigchld.get(),
Topic::InternalExit => self.internal_exit.get(),
}
@@ -124,7 +124,7 @@ pub fn get(&self, topic: Topic) -> Generation {
/// Return ourselves as an array.
pub fn as_array(&self) -> [Generation; 3] {
[
self.sighupint.get(),
self.sighupintterm.get(),
self.sigchld.get(),
self.internal_exit.get(),
]
@@ -648,7 +648,7 @@ fn test_topic_monitor_torture() {
let monitor = Arc::new(TopicMonitor::default());
const THREAD_COUNT: usize = 64;
let t1 = Topic::SigChld;
let t2 = Topic::SigHupInt;
let t2 = Topic::SigHupIntTerm;
let mut gens_list = vec![GenerationsList::invalid(); THREAD_COUNT];
let post_count = Arc::new(AtomicU64::new(0));
for r#gen in &mut gens_list {

2
src/wutil/mod.rs
View File

@@ -392,7 +392,7 @@ fn do_write(
true
};
let mut sigcheck = SigChecker::new_sighupint();
let mut sigcheck = SigChecker::new_sighupintterm();
let mut success = str2bytes_callback(input, |buff: &[u8]| {
if buff.len() + accumlen > accum_capacity {
// We have to flush.

50
tests/pexpects/sigterm_history.py Normal file
View File

@@ -0,0 +1,50 @@
#!/usr/bin/env python3
from pexpect_helper import SpawnedProc
import os
import pexpect
import signal
import sys
sp = SpawnedProc()
fish_pid = sp.spawn.pid
assert fish_pid is not None
sp.expect_prompt()
# Set up a fish_exit handler that prints a marker to stdout.
sp.sendline("function __test_exit --on-event fish_exit; echo SIGTERM_CLEANUP_OK; end")
sp.expect_prompt()
# Run a command to populate history.
sp.sendline("echo sigterm_history_test_%d" % fish_pid)
sp.expect_prompt()
# Send SIGTERM.
os.kill(fish_pid, signal.SIGTERM)
# Try to catch the cleanup marker before EOF.
idx = sp.spawn.expect(["SIGTERM_CLEANUP_OK", pexpect.EOF], timeout=5)
if idx == 1:
print("FAIL: fish_exit did not fire on SIGTERM (no cleanup marker in output)")
sys.exit(1)
# Drain remaining output.
sp.spawn.expect(pexpect.EOF, timeout=5)
sp.spawn.close()
# Verify death was by signal (re-raised SIGTERM), not normal exit.
if sp.spawn.signalstatus != signal.SIGTERM:
print(
"FAIL: expected SIGTERM, got signal=%s exit=%s"
% (sp.spawn.signalstatus, sp.spawn.exitstatus)
)
sys.exit(1)
# Check history file for NUL bytes.
histfile = os.path.join(os.environ["XDG_DATA_HOME"], "fish", "fish_history")
with open(histfile, "rb") as f:
raw = f.read()
assert len(raw) != 0, "history file is empty"
nul_count = raw.count(b"\x00")
if nul_count > 0:
print("FAIL: history file contains %d NUL bytes" % nul_count)
sys.exit(1)