From a2e486966acbb15b1c666ab22e81f42919b67353 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sun, 6 Dec 2020 13:40:45 -0800 Subject: [PATCH] Always become pgroup leader in interactive mode Prior to this change, if fish were launched connected to a tty but not as pgroup leader, it would attempt to become pgroup leader only if --interactive is explicitly passed. But bash will unconditionally attempt to become pgroup leader if launched interactively. This can result in scenarios where fish is running interactively but in another pgroup. The most obvious impact is that control-C will result in the pgroup leader (not fish) exiting and make fish orphaned. Switch to matching the bash behavior here - we will always try to become pgroup leader if interactive. Fixes #7060. --- CHANGELOG.rst | 1 + src/builtin_commandline.cpp | 2 +- src/builtin_status.cpp | 2 +- src/env_dispatch.cpp | 7 +++---- src/fish.cpp | 6 +++--- src/fish_key_reader.cpp | 2 +- src/proc.cpp | 9 ++++----- src/proc.h | 7 +++---- src/reader.cpp | 18 ++++++++---------- 9 files changed, 25 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b6c125216..529267cae 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -40,6 +40,7 @@ Notable improvements and fixes - ``type`` is now a builtin and therefore much faster (:issue:`7342`). - ``string match --regex`` now imports named PCRE2 capture groups as fish variables (:issue:`7459`). Note: Because of this, it can no longer be wrapped in a function and the name has been added as a reserved word. - Globs and other expansions are limited to 512k results (:issue:`7226`). +- fish will now always attempt to become process group leader in interactive mode (:issue:`7060`). Syntax changes and new commands ------------------------------- diff --git a/src/builtin_commandline.cpp b/src/builtin_commandline.cpp index d927a3513..d53c54a63 100644 --- a/src/builtin_commandline.cpp +++ b/src/builtin_commandline.cpp @@ -159,7 +159,7 @@ maybe_t builtin_commandline(parser_t &parser, io_streams_t &streams, wchar_ } if (!current_buffer) { - if (session_interactivity() != session_interactivity_t::not_interactive) { + if (is_interactive_session()) { // Prompt change requested while we don't have a prompt, most probably while reading the // init files. Just ignore it. return STATUS_CMD_ERROR; diff --git a/src/builtin_status.cpp b/src/builtin_status.cpp index 58fbe9230..ccb8200a5 100644 --- a/src/builtin_status.cpp +++ b/src/builtin_status.cpp @@ -397,7 +397,7 @@ maybe_t builtin_status(parser_t &parser, io_streams_t &streams, wchar_t **a } case STATUS_IS_INTERACTIVE: { CHECK_FOR_UNEXPECTED_STATUS_ARGS(opts.status_cmd) - retval = session_interactivity() == session_interactivity_t::not_interactive ? 1 : 0; + retval = is_interactive_session() ? 0 : 1; break; } case STATUS_IS_COMMAND_SUB: { diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp index 4e993c6d0..2758348b5 100644 --- a/src/env_dispatch.cpp +++ b/src/env_dispatch.cpp @@ -437,12 +437,11 @@ static bool initialize_curses_using_fallback(const char *term) { auto term_env = wcs2string(term_var->as_string()); if (term_env == DEFAULT_TERM1 || term_env == DEFAULT_TERM2) return false; - if (session_interactivity() != session_interactivity_t::not_interactive) - FLOGF(warning, _(L"Using fallback terminal type '%s'."), term); + if (is_interactive_session()) FLOGF(warning, _(L"Using fallback terminal type '%s'."), term); int err_ret; if (setupterm(const_cast(term), STDOUT_FILENO, &err_ret) == OK) return true; - if (session_interactivity() != session_interactivity_t::not_interactive) { + if (is_interactive_session()) { FLOGF(warning, _(L"Could not set up terminal using the fallback terminal type '%s'."), term); } @@ -503,7 +502,7 @@ static void init_curses(const environment_t &vars) { int err_ret; if (setupterm(nullptr, STDOUT_FILENO, &err_ret) == ERR) { auto term = vars.get(L"TERM"); - if (session_interactivity() != session_interactivity_t::not_interactive) { + if (is_interactive_session()) { FLOGF(warning, _(L"Could not set up terminal.")); if (term.missing_or_empty()) { FLOGF(warning, _(L"TERM environment variable not set.")); diff --git a/src/fish.cpp b/src/fish.cpp index 9e494788d..c04167661 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -384,7 +384,7 @@ static int fish_parse_opt(int argc, char **argv, fish_cmd_opts_t *opts) { // command or file to execute and stdin is a tty. Note that the -i or // --interactive options also force interactive mode. if (opts->batch_cmds.empty() && optind == argc && isatty(STDIN_FILENO)) { - set_interactive_session(session_interactivity_t::implied); + set_interactive_session(true); } return optind; @@ -447,12 +447,12 @@ int main(int argc, char **argv) { // Apply our options. if (opts.is_login) mark_login(); if (opts.no_exec) mark_no_exec(); - if (opts.is_interactive_session) set_interactive_session(session_interactivity_t::explicit_); + if (opts.is_interactive_session) set_interactive_session(true); if (opts.enable_private_mode) start_private_mode(env_stack_t::globals()); // Only save (and therefore restore) the fg process group if we are interactive. See issues // #197 and #1002. - if (session_interactivity() != session_interactivity_t::not_interactive) { + if (is_interactive_session()) { save_term_foreground_process_group(); } diff --git a/src/fish_key_reader.cpp b/src/fish_key_reader.cpp index 4521621e4..63cb4fc85 100644 --- a/src/fish_key_reader.cpp +++ b/src/fish_key_reader.cpp @@ -243,7 +243,7 @@ static void process_input(bool continuous_mode) { /// Setup our environment (e.g., tty modes), process key strokes, then reset the environment. [[noreturn]] static void setup_and_process_keys(bool continuous_mode) { - set_interactive_session(session_interactivity_t::implied); + set_interactive_session(true); set_main_thread(); setup_fork_guards(); env_init(); diff --git a/src/proc.cpp b/src/proc.cpp index d1e8b898a..7bac717b5 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -57,10 +57,9 @@ /// The signals that signify crashes to us. static const int crashsignals[] = {SIGABRT, SIGBUS, SIGFPE, SIGILL, SIGSEGV, SIGSYS}; -static relaxed_atomic_t s_is_interactive_session{ - session_interactivity_t::not_interactive}; -session_interactivity_t session_interactivity() { return s_is_interactive_session; } -void set_interactive_session(session_interactivity_t flag) { s_is_interactive_session = flag; } +static relaxed_atomic_bool_t s_is_interactive_session{false}; +bool is_interactive_session() { return s_is_interactive_session; } +void set_interactive_session(bool flag) { s_is_interactive_session = flag; } static relaxed_atomic_bool_t s_is_login{false}; bool get_login() { return s_is_login; } @@ -261,7 +260,7 @@ static void handle_child_status(const shared_ptr &job, process_t *proc, if (status.signal_exited()) { int sig = status.signal_code(); if (sig == SIGINT || sig == SIGQUIT) { - if (session_interactivity() != session_interactivity_t::not_interactive) { + if (is_interactive_session()) { // Mark the job group as cancelled. job->group->cancel_with_signal(sig); } else { diff --git a/src/proc.h b/src/proc.h index 73973cd04..f7f31fa4d 100644 --- a/src/proc.h +++ b/src/proc.h @@ -480,10 +480,9 @@ class job_t { maybe_t get_statuses() const; }; -/// Whether this shell is attached to the keyboard at all. -enum class session_interactivity_t { not_interactive, implied, explicit_ }; -session_interactivity_t session_interactivity(); -void set_interactive_session(session_interactivity_t flag); +/// Whether this shell is attached to a tty. +bool is_interactive_session(); +void set_interactive_session(bool flag); /// Whether we are a login shell. bool get_login(); diff --git a/src/reader.cpp b/src/reader.cpp index 8e4b6a44e..7fc8d30c5 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -2076,7 +2076,7 @@ static void acquire_tty_or_exit(pid_t shell_pgid) { owner = tcgetpgrp(STDIN_FILENO); } if (owner == -1 && errno == ENOTTY) { - if (session_interactivity() == session_interactivity_t::not_interactive) { + if (!is_interactive_session()) { // It's OK if we're not able to take control of the terminal. We handle // the fallout from this in a few other places. break; @@ -2114,6 +2114,7 @@ static void reader_interactive_init(parser_t &parser) { ASSERT_IS_MAIN_THREAD(); pid_t shell_pgid = getpgrp(); + pid_t shell_pid = getpid(); // Set up key bindings. init_input(); @@ -2124,12 +2125,10 @@ static void reader_interactive_init(parser_t &parser) { // Wait until we own the terminal. acquire_tty_or_exit(shell_pgid); - // It shouldn't be necessary to place fish in its own process group and force control - // of the terminal, but that works around fish being started with an invalid pgroup, - // such as when launched via firejail (#5295) - // Also become the process group leader if flag -i/--interactive was given (#5909). - if (shell_pgid == 0 || session_interactivity() == session_interactivity_t::explicit_) { - shell_pgid = getpid(); + // If fish has no valid pgroup (possible with firejail, see #5295) or is interactive, + // ensure it owns the terminal. Also see #5909, #7060. + if (shell_pgid == 0 || (is_interactive_session() && shell_pgid != shell_pid)) { + shell_pgid = shell_pid; if (setpgid(shell_pgid, shell_pgid) < 0) { // If we're session leader setpgid returns EPERM. The other cases where we'd get EPERM // don't apply as we passed our own pid. @@ -3705,7 +3704,7 @@ maybe_t reader_data_t::readline(int nchars_or_0) { // This check is required to work around certain issues with fish's approach to // terminal control when launching interactive processes while in non-interactive // mode. See #4178 for one such example. - if (err != ENOTTY || session_interactivity() != session_interactivity_t::not_interactive) { + if (err != ENOTTY || is_interactive_session()) { wperror(L"tcsetattr"); } } @@ -3850,8 +3849,7 @@ maybe_t reader_data_t::readline(int nchars_or_0) { if (s_exit_state != exit_state_t::finished_handlers) { // The order of the two conditions below is important. Try to restore the mode // in all cases, but only complain if interactive. - if (tcsetattr(conf.in, TCSANOW, &old_modes) == -1 && - session_interactivity() != session_interactivity_t::not_interactive) { + if (tcsetattr(conf.in, TCSANOW, &old_modes) == -1 && is_interactive_session()) { if (errno == EIO) redirect_tty_output(); wperror(L"tcsetattr"); // return to previous mode }