From ecdc9ce1ddac2b059c21cea7bf957deb68a9a8fd Mon Sep 17 00:00:00 2001
From: Johannes Altmanninger <aclopte@gmail.com>
Date: Sun, 24 Mar 2024 11:00:30 +0100
Subject: [PATCH] Install a panic handler to avoid dropping crash stacktraces
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When fish crashes due to a panic, the terminal window is closed.  Some
terminals keep the window around when the crash is due to a fatal signal,
but today we don't exit via fatal signal on panic.

There is the option to set «panic = "abort"» in Cargo.toml, which
would give us coredumps but also worse stacktraces on stderr.
More importantly it means that we don't unwind, so destructors are skipped
I don't think we want that because we should use destructors to
restore the terminal state.

On crash in interactive fish, read one more line before exiting, so the
stack trace is always visible.

In future, we should move this "read one line before exiting" logic to where
we call "panic!", so I can attach a debugger and see the stacktrace.
---
 src/bin/fish.rs            | 12 ++++++++----
 src/bin/fish_indent.rs     |  6 +++++-
 src/bin/fish_key_reader.rs |  5 +++++
 src/lib.rs                 |  1 +
 src/panic.rs               | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 52 insertions(+), 5 deletions(-)
 create mode 100644 src/panic.rs

diff --git a/src/bin/fish.rs b/src/bin/fish.rs
index bfaf72e7d..6100c3877 100644
--- a/src/bin/fish.rs
+++ b/src/bin/fish.rs
@@ -32,18 +32,18 @@ use fish::{
         scoped_push_replacer, str2wcstring, wcs2string, PACKAGE_NAME, PROFILING_ACTIVE,
         PROGRAM_NAME,
     },
-    env::Statuses,
     env::{
         environment::{env_init, EnvStack, Environment},
-        ConfigPaths, EnvMode,
+        ConfigPaths, EnvMode, Statuses,
     },
     eprintf,
     event::{self, Event},
     flog::{self, activate_flog_categories_by_pattern, set_flog_file_fd, FLOG, FLOGF},
-    fprintf, function, future_feature_flags as features, history,
-    history::start_private_mode,
+    fprintf, function, future_feature_flags as features,
+    history::{self, start_private_mode},
     io::IoChain,
     nix::{getpid, isatty},
+    panic::panic_handler,
     parse_constants::{ParseErrorList, ParseTreeFlags},
     parse_tree::ParsedSource,
     parse_util::parse_util_detect_errors_in_ast,
@@ -465,6 +465,10 @@ fn cstr_from_osstr(s: &OsStr) -> CString {
 }
 
 fn main() {
+    panic_handler(throwing_main)
+}
+
+fn throwing_main() {
     let mut args: Vec<WString> = env::args_os()
         .map(|osstr| str2wcstring(osstr.as_bytes()))
         .collect();
diff --git a/src/bin/fish_indent.rs b/src/bin/fish_indent.rs
index 78ac29de5..799ac1740 100644
--- a/src/bin/fish_indent.rs
+++ b/src/bin/fish_indent.rs
@@ -11,6 +11,7 @@ use std::io::{stdin, Read, Write};
 use std::os::unix::ffi::OsStrExt;
 use std::sync::atomic::Ordering;
 
+use fish::panic::panic_handler;
 use libc::{LC_ALL, STDOUT_FILENO};
 
 use fish::ast::{
@@ -737,8 +738,11 @@ fn char_is_escaped(text: &wstr, idx: usize) -> bool {
 }
 
 fn main() {
-    PROGRAM_NAME.set(L!("fish_indent")).unwrap();
+    panic_handler(throwing_main)
+}
 
+fn throwing_main() {
+    PROGRAM_NAME.set(L!("fish_indent")).unwrap();
     topic_monitor_init();
     threads::init();
     // Using the user's default locale could be a problem if it doesn't use UTF-8 encoding. That's
diff --git a/src/bin/fish_key_reader.rs b/src/bin/fish_key_reader.rs
index 7022b461e..9c8f0efd3 100644
--- a/src/bin/fish_key_reader.rs
+++ b/src/bin/fish_key_reader.rs
@@ -25,6 +25,7 @@ use fish::{
     fprintf,
     input::input_terminfo_get_name,
     input_common::{CharEvent, InputEventQueue, InputEventQueuer},
+    panic::panic_handler,
     print_help::print_help,
     printf,
     proc::set_interactive_session,
@@ -364,6 +365,10 @@ fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> bool {
 }
 
 fn main() {
+    panic_handler(throwing_main)
+}
+
+fn throwing_main() {
     PROGRAM_NAME.set(L!("fish_key_reader")).unwrap();
     let mut continuous_mode = false;
     let mut verbose = false;
diff --git a/src/lib.rs b/src/lib.rs
index 27fb10cea..66338cf4b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -68,6 +68,7 @@ pub mod null_terminated_array;
 pub mod operation_context;
 pub mod output;
 pub mod pager;
+pub mod panic;
 pub mod parse_constants;
 pub mod parse_execution;
 pub mod parse_tree;
diff --git a/src/panic.rs b/src/panic.rs
new file mode 100644
index 000000000..597276856
--- /dev/null
+++ b/src/panic.rs
@@ -0,0 +1,33 @@
+use std::panic::{catch_unwind, UnwindSafe};
+
+use libc::STDIN_FILENO;
+
+use crate::{
+    common::{read_blocked, PROGRAM_NAME},
+    nix::isatty,
+};
+
+pub fn panic_handler(main: impl FnOnce() + UnwindSafe) -> ! {
+    if !isatty(STDIN_FILENO) {
+        main();
+        unreachable!();
+    }
+    if catch_unwind(main).is_ok() {
+        unreachable!();
+    }
+    printf!(
+        "%s with PID %d crashed, please report a bug. Press Enter to exit",
+        PROGRAM_NAME.get().unwrap(),
+        unsafe { libc::getpid() }
+    );
+    let mut buf = [0_u8; 1];
+    loop {
+        let Ok(n) = read_blocked(STDIN_FILENO, &mut buf) else {
+            std::process::exit(110);
+        };
+        if n == 0 || matches!(buf[0], b'q' | b'\n' | b'\r') {
+            printf!("\n");
+            std::process::exit(110);
+        }
+    }
+}