Use RAII for restoring term modes

In particular, this allows restoring the terminal on crashes, which is
feasible now that we have the panic handler.  Since std::process::exit() skips
destructors, we need to reshuffle some code.  The "exit_without_destructors"
semantics (which std::process::exit() als has) was mostly necessary for C++
since Rust leaks global variables by default.

src/bin/fish.rs

@@ -27,10 +27,9 @@ use fish::{
         BUILTIN_ERR_MISSING, BUILTIN_ERR_UNKNOWN, STATUS_CMD_OK, STATUS_CMD_UNKNOWN,
     },
     common::{
-        escape, exit_without_destructors, get_executable_path,
+        escape, get_executable_path, restore_term_foreground_process_group_for_exit,
-        restore_term_foreground_process_group_for_exit, save_term_foreground_process_group,
+        save_term_foreground_process_group, scoped_push_replacer, str2wcstring, wcs2string,
-        scoped_push_replacer, str2wcstring, wcs2string, PACKAGE_NAME, PROFILING_ACTIVE,
+        ScopeGuard, PACKAGE_NAME, PROFILING_ACTIVE, PROGRAM_NAME,
-        PROGRAM_NAME,
     },
     env::{
         environment::{env_init, EnvStack, Environment},
@@ -54,14 +53,13 @@ use fish::{
         get_login, is_interactive_session, mark_login, mark_no_exec, proc_init,
         set_interactive_session,
     },
-    reader::{reader_init, reader_read, restore_term_mode, term_copy_modes},
+    reader::{reader_init, reader_read, term_copy_modes},
     signal::{signal_clear_cancel, signal_unblock_all},
-    threads::{self, asan_maybe_exit},
+    threads::{self},
     topic_monitor,
     wchar::prelude::*,
     wutil::waccess,
 };
-use std::env;
 use std::ffi::{CString, OsStr, OsString};
 use std::fs::File;
 use std::mem::MaybeUninit;
@@ -69,6 +67,7 @@ use std::os::unix::prelude::*;
 use std::path::{Path, PathBuf};
 use std::sync::atomic::Ordering;
 use std::sync::Arc;
+use std::{env, ops::ControlFlow};
 
 const DOC_DIR: &str = env!("DOCDIR");
 const DATA_DIR: &str = env!("DATADIR");
@@ -313,7 +312,7 @@ fn run_command_list(parser: &Parser, cmds: &[OsString]) -> i32 {
     retval.unwrap()
 }
 
-fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> usize {
+fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> ControlFlow<i32, usize> {
     use fish::wgetopt::{wgetopter_t, wopt, woption, woption_argument_t::*};
 
     const RUSAGE_ARG: char = 1 as char;
@@ -394,7 +393,7 @@ fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> usize {
                     // this is left-justified
                     println!("{:<width$} {}", cat.name, desc, width = name_width);
                 }
-                std::process::exit(0);
+                return ControlFlow::Break(0);
             }
             // "--profile" - this does not activate profiling right away,
             // rather it's done after startup is finished.
@@ -411,7 +410,7 @@ fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> usize {
                     "%s",
                     wgettext_fmt!("%s, version %s\n", PACKAGE_NAME, fish::BUILD_VERSION)
                 );
-                std::process::exit(0);
+                return ControlFlow::Break(0);
             }
             'D' => {
                 // TODO: Option is currently useless.
@@ -422,14 +421,14 @@ fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> usize {
                     "{}",
                     wgettext_fmt!(BUILTIN_ERR_UNKNOWN, "fish", args[w.woptind - 1])
                 );
-                std::process::exit(1)
+                return ControlFlow::Break(1);
             }
             ':' => {
                 eprintln!(
                     "{}",
                     wgettext_fmt!(BUILTIN_ERR_MISSING, "fish", args[w.woptind - 1])
                 );
-                std::process::exit(1)
+                return ControlFlow::Break(1);
             }
             _ => panic!("unexpected retval from wgetopter_t"),
         }
@@ -446,7 +445,7 @@ fn fish_parse_opt(args: &mut [WString], opts: &mut FishCmdOpts) -> usize {
         set_interactive_session(true);
     }
 
-    optind
+    ControlFlow::Continue(optind)
 }
 
 fn cstr_from_osstr(s: &OsStr) -> CString {
@@ -468,7 +467,7 @@ fn main() {
     panic_handler(throwing_main)
 }
 
-fn throwing_main() {
+fn throwing_main() -> i32 {
     let mut args: Vec<WString> = env::args_os()
         .map(|osstr| str2wcstring(osstr.as_bytes()))
         .collect();
@@ -478,7 +477,6 @@ fn throwing_main() {
         .expect("multiple entrypoints setting PROGRAM_NAME");
 
     let mut res = 1;
-    let mut my_optind;
 
     signal_unblock_all();
     topic_monitor::topic_monitor_init();
@@ -503,7 +501,10 @@ fn throwing_main() {
     }
 
     let mut opts = FishCmdOpts::default();
-    my_optind = fish_parse_opt(&mut args, &mut opts);
+    let mut my_optind = match fish_parse_opt(&mut args, &mut opts) {
+        ControlFlow::Continue(optind) => optind,
+        ControlFlow::Break(status) => return status,
+    };
 
     // Direct any debug output right away.
     // --debug-output takes precedence, otherwise $FISH_DEBUG_OUTPUT is used.
@@ -529,7 +530,7 @@ fn throwing_main() {
                 // TODO: should not be debug-print
                 eprintln!("Could not open file {:?}", debug_path);
                 eprintln!("{}", e);
-                std::process::exit(1);
+                return 1;
             }
         };
     }
@@ -587,7 +588,9 @@ fn throwing_main() {
     features::set_from_string(opts.features.as_utfstr());
     proc_init();
     fish::env::misc_init();
-    reader_init();
+    let _restore_term_foreground_process_group =
+        ScopeGuard::new((), |()| restore_term_foreground_process_group_for_exit());
+    let _restore_term = reader_init();
 
     let parser = Parser::principal_parser();
     parser.set_syncs_uvars(!opts.no_config);
@@ -659,7 +662,7 @@ fn throwing_main() {
                 "no-execute mode enabled and no script given. Exiting"
             );
             // above line should always exit
-            std::process::exit(libc::EXIT_FAILURE);
+            return libc::EXIT_FAILURE;
         }
         res = reader_read(parser, libc::STDIN_FILENO, &IoChain::new());
     } else {
@@ -718,10 +721,6 @@ fn throwing_main() {
         vec![exit_status.to_wstring()],
     );
 
-    restore_term_mode();
-    // this is ported, but not adopted
-    restore_term_foreground_process_group_for_exit();
-
     if let Some(profile_output) = opts.profile_output {
         let s = cstr_from_osstr(&profile_output);
         parser.emit_profiling(s.as_bytes());
@@ -732,8 +731,7 @@ fn throwing_main() {
         print_rusage_self();
     }
 
-    asan_maybe_exit(exit_status);
+    exit_status
-    exit_without_destructors(exit_status)
 }
 
 // https://github.com/fish-shell/fish-shell/issues/367

src/bin/fish_indent.rs

@@ -741,7 +741,7 @@ fn main() {
     panic_handler(throwing_main)
 }
 
-fn throwing_main() {
+fn throwing_main() -> i32 {
     PROGRAM_NAME.set(L!("fish_indent")).unwrap();
     topic_monitor_init();
     threads::init();
@@ -815,7 +815,7 @@ fn throwing_main() {
             'P' => DUMP_PARSE_TREE.store(true),
             'h' => {
                 print_help("fish_indent");
-                std::process::exit(STATUS_CMD_OK.unwrap());
+                return STATUS_CMD_OK.unwrap();
             }
             'v' => {
                 printf!(
@@ -826,7 +826,7 @@ fn throwing_main() {
                         fish::BUILD_VERSION
                     )
                 );
-                std::process::exit(STATUS_CMD_OK.unwrap());
+                return STATUS_CMD_OK.unwrap();
             }
             'w' => output_type = OutputType::File,
             'i' => do_indent = false,
@@ -849,7 +849,7 @@ fn throwing_main() {
             'o' => {
                 debug_output = Some(w.woptarg.unwrap());
             }
-            _ => std::process::exit(STATUS_CMD_ERROR.unwrap()),
+            _ => return STATUS_CMD_ERROR.unwrap(),
         }
     }
 
@@ -865,7 +865,7 @@ fn throwing_main() {
         if file.is_null() {
             eprintf!("Could not open file %s\n", debug_output);
             perror("fopen");
-            std::process::exit(-1);
+            return -1;
         }
         let fd = unsafe { libc::fileno(file) };
         set_cloexec(fd, true);
@@ -887,11 +887,11 @@ fn throwing_main() {
                         PROGRAM_NAME.get().unwrap()
                     )
                 );
-                std::process::exit(STATUS_CMD_ERROR.unwrap());
+                return STATUS_CMD_ERROR.unwrap();
             }
             match read_file(stdin()) {
                 Ok(s) => src = s,
-                Err(()) => std::process::exit(STATUS_CMD_ERROR.unwrap()),
+                Err(()) => return STATUS_CMD_ERROR.unwrap(),
             }
         } else {
             let arg = args[i];
@@ -899,7 +899,7 @@ fn throwing_main() {
                 Ok(file) => {
                     match read_file(file) {
                         Ok(s) => src = s,
-                        Err(()) => std::process::exit(STATUS_CMD_ERROR.unwrap()),
+                        Err(()) => return STATUS_CMD_ERROR.unwrap(),
                     }
                     output_location = arg;
                 }
@@ -908,7 +908,7 @@ fn throwing_main() {
                         "%s",
                         wgettext_fmt!("Opening \"%s\" failed: %s\n", arg, err.to_string())
                     );
-                    std::process::exit(STATUS_CMD_ERROR.unwrap());
+                    return STATUS_CMD_ERROR.unwrap();
                 }
             }
         }
@@ -953,7 +953,7 @@ fn throwing_main() {
                                 err.to_string()
                             )
                         );
-                        std::process::exit(STATUS_CMD_ERROR.unwrap());
+                        return STATUS_CMD_ERROR.unwrap();
                     }
                 }
             }
@@ -983,7 +983,7 @@ fn throwing_main() {
         let _ = write_to_fd(&colored_output, STDOUT_FILENO);
         i += 1;
     }
-    std::process::exit(retval)
+    retval
 }
 
 static DUMP_PARSE_TREE: RelaxedAtomicBool = RelaxedAtomicBool::new(false);

src/bin/fish_key_reader.rs

@@ -7,7 +7,9 @@
 //!
 //! Type "exit" or "quit" to terminate the program.
 
+use core::panic;
 use std::{
+    ops::ControlFlow,
     os::unix::prelude::OsStrExt,
     time::{Duration, Instant},
 };
@@ -29,10 +31,7 @@ use fish::{
     print_help::print_help,
     printf,
     proc::set_interactive_session,
-    reader::{
+    reader::{check_exit_loop_maybe_warning, reader_init, reader_test_and_clear_interrupted},
-        check_exit_loop_maybe_warning, reader_init, reader_test_and_clear_interrupted,
-        restore_term_mode,
-    },
     signal::signal_set_handlers,
     threads,
     topic_monitor::topic_monitor_init,
@@ -222,7 +221,7 @@ fn output_elapsed_time(prev_timestamp: Instant, first_char_seen: bool, verbose:
 }
 
 /// Process the characters we receive as the user presses keys.
-fn process_input(continuous_mode: bool, verbose: bool) {
+fn process_input(continuous_mode: bool, verbose: bool) -> i32 {
     let mut first_char_seen = false;
     let mut prev_timestamp = Instant::now()
         .checked_sub(Duration::from_millis(1000))
@@ -243,7 +242,7 @@ fn process_input(continuous_mode: bool, verbose: bool) {
         if evt.as_ref().is_none_or(|evt| !evt.is_char()) {
             output_bind_command(&mut bind_chars);
             if first_char_seen && !continuous_mode {
-                return;
+                return 0;
             }
             continue;
         }
@@ -271,15 +270,16 @@ fn process_input(continuous_mode: bool, verbose: bool) {
 
         first_char_seen = true;
     }
+    0
 }
 
 /// Setup our environment (e.g., tty modes), process key strokes, then reset the environment.
-fn setup_and_process_keys(continuous_mode: bool, verbose: bool) -> ! {
+fn setup_and_process_keys(continuous_mode: bool, verbose: bool) -> i32 {
     set_interactive_session(true);
     topic_monitor_init();
     threads::init();
     env_init(None, true, false);
-    reader_init();
+    let _restore_term = reader_init();
 
     signal_set_handlers(true);
     // We need to set the shell-modes for ICRNL,
@@ -298,12 +298,10 @@ fn setup_and_process_keys(continuous_mode: bool, verbose: bool) -> ! {
         eprintf!("\n");
     }
 
-    process_input(continuous_mode, verbose);
+    process_input(continuous_mode, verbose)
-    restore_term_mode();
-    std::process::exit(0);
 }
 
-fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> bool {
+fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> ControlFlow<i32> {
     let short_opts: &wstr = L!("+chvV");
     let long_opts: &[woption] = &[
         wopt(L!("continuous"), woption_argument_t::no_argument, 'c'),
@@ -324,7 +322,7 @@ fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> bool {
             }
             'h' => {
                 print_help("fish_key_reader");
-                std::process::exit(0);
+                return ControlFlow::Break(0);
             }
             'v' => {
                 printf!(
@@ -335,7 +333,7 @@ fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> bool {
                         fish::BUILD_VERSION
                     )
                 );
-                std::process::exit(0);
+                return ControlFlow::Break(0);
             }
             'V' => {
                 *verbose = true;
@@ -349,7 +347,7 @@ fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> bool {
                         w.argv[w.woptind - 1]
                     )
                 );
-                return false;
+                return ControlFlow::Break(1);
             }
             _ => panic!(),
         }
@@ -358,29 +356,29 @@ fn parse_flags(continuous_mode: &mut bool, verbose: &mut bool) -> bool {
     let argc = args.len() - w.woptind;
     if argc != 0 {
         eprintf!("Expected no arguments, got %d\n", argc);
-        return false;
+        return ControlFlow::Break(1);
     }
 
-    true
+    ControlFlow::Continue(())
 }
 
 fn main() {
     panic_handler(throwing_main)
 }
 
-fn throwing_main() {
+fn throwing_main() -> i32 {
     PROGRAM_NAME.set(L!("fish_key_reader")).unwrap();
     let mut continuous_mode = false;
     let mut verbose = false;
 
-    if !parse_flags(&mut continuous_mode, &mut verbose) {
+    if let ControlFlow::Break(i) = parse_flags(&mut continuous_mode, &mut verbose) {
-        std::process::exit(1);
+        return i;
     }
 
     if unsafe { libc::isatty(STDIN_FILENO) } == 0 {
         eprintf!("Stdin must be attached to a tty.\n");
-        std::process::exit(1);
+        return 1;
     }
 
-    setup_and_process_keys(continuous_mode, verbose);
+    setup_and_process_keys(continuous_mode, verbose)
 }

src/panic.rs

@@ -5,15 +5,21 @@ use libc::STDIN_FILENO;
 use crate::{
     common::{read_blocked, PROGRAM_NAME},
     nix::isatty,
+    threads::asan_maybe_exit,
 };
 
-pub fn panic_handler(main: impl FnOnce() + UnwindSafe) -> ! {
+pub fn panic_handler(main: impl FnOnce() -> i32 + UnwindSafe) -> ! {
+    let exit_status = panic_handler_impl(main);
+    asan_maybe_exit(exit_status);
+    std::process::exit(exit_status)
+}
+
+fn panic_handler_impl(main: impl FnOnce() -> i32 + UnwindSafe) -> i32 {
     if !isatty(STDIN_FILENO) {
-        main();
+        return main();
-        unreachable!();
     }
-    if catch_unwind(main).is_ok() {
+    if let Ok(status) = catch_unwind(main) {
-        unreachable!();
+        return status;
     }
     printf!(
         "%s with PID %d crashed, please report a bug. Press Enter to exit",
@@ -23,11 +29,12 @@ pub fn panic_handler(main: impl FnOnce() + UnwindSafe) -> ! {
     let mut buf = [0_u8; 1];
     loop {
         let Ok(n) = read_blocked(STDIN_FILENO, &mut buf) else {
-            std::process::exit(110);
+            break;
         };
         if n == 0 || matches!(buf[0], b'q' | b'\n' | b'\r') {
             printf!("\n");
-            std::process::exit(110);
+            break;
         }
     }
+    110
 }

src/reader.rs

@@ -41,8 +41,8 @@ use crate::color::RgbColor;
 use crate::common::{
     escape, escape_string, exit_without_destructors, get_ellipsis_char, get_obfuscation_read_char,
     redirect_tty_output, scoped_push_replacer, scoped_push_replacer_ctx, shell_modes, str2wcstring,
-    wcs2string, write_loop, EscapeFlags, EscapeStringStyle, ScopeGuard, PROGRAM_NAME,
+    wcs2string, write_loop, EscapeFlags, EscapeStringStyle, ScopeGuard, ScopeGuarding,
-    UTF8_BOM_WCHAR,
+    PROGRAM_NAME, UTF8_BOM_WCHAR,
 };
 use crate::complete::{
     complete, complete_load, sort_and_prioritize, CompleteFlags, Completion, CompletionList,
@@ -757,7 +757,7 @@ fn read_ni(parser: &Parser, fd: RawFd, io: &IoChain) -> i32 {
 }
 
 /// Initialize the reader.
-pub fn reader_init() {
+pub fn reader_init() -> impl ScopeGuarding<Target = ()> {
     // 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) };
@@ -784,6 +784,9 @@ pub fn reader_init() {
     if is_interactive_session() && unsafe { libc::getpgrp() == libc::tcgetpgrp(STDIN_FILENO) } {
         term_donate(/*quiet=*/ true);
     }
+    ScopeGuard::new((), |()| {
+        restore_term_mode();
+    })
 }
 
 /// Restore the term mode if we own the terminal and are interactive (#8705).

src/tests/mod.rs

@@ -27,6 +27,7 @@ mod topic_monitor;
 mod wgetopt;
 
 pub mod prelude {
+    use crate::common::ScopeGuarding;
     use crate::env::{env_init, misc_init};
     use crate::reader::reader_init;
     use crate::signal::signal_reset_handlers;
@@ -60,7 +61,7 @@ pub mod prelude {
         EnvStack::principal().set_pwd_from_getcwd();
     }
 
-    pub fn test_init() {
+    pub fn test_init() -> impl ScopeGuarding<Target = ()> {
         static DONE: OnceCell<()> = OnceCell::new();
         DONE.get_or_init(|| {
             set_current_dir(env!("FISH_BUILD_DIR")).unwrap();
@@ -75,7 +76,6 @@ pub mod prelude {
             proc_init();
             env_init(None, true, false);
             misc_init();
-            reader_init();
 
             // Set default signal handlers, so we can ctrl-C out of this.
             signal_reset_handlers();
@@ -83,6 +83,7 @@ pub mod prelude {
             // Set PWD from getcwd - fixes #5599
             EnvStack::principal().set_pwd_from_getcwd();
         });
+        reader_init()
     }
 
     pub use serial_test::serial;