From 15c1b3ed4b45e076b0573815292fb302a4220c9c Mon Sep 17 00:00:00 2001 From: Mathieu Duponchelle Date: Thu, 12 Dec 2019 13:50:33 +0100 Subject: [PATCH] Place fish in its own process group when launched with -i Fixes #5909 --- src/builtin.cpp | 3 ++- src/builtin_commandline.cpp | 2 +- src/builtin_status.cpp | 2 +- src/env_dispatch.cpp | 7 ++++--- src/fish.cpp | 6 +++--- src/fish_key_reader.cpp | 3 ++- src/fish_tests.cpp | 6 +++--- src/proc.cpp | 9 +++++---- src/proc.h | 5 +++-- src/reader.cpp | 10 ++++++---- 10 files changed, 30 insertions(+), 23 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 469feb416..d27f4f8af 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -449,7 +449,8 @@ proc_status_t builtin_run(parser_t &parser, int job_pgid, wchar_t **argv, io_str if (const builtin_data_t *data = builtin_lookup(argv[0])) { // If we are interactive, save the foreground pgroup and restore it after in case the // builtin needs to read from the terminal. See #4540. - bool grab_tty = is_interactive_session() && isatty(streams.stdin_fd); + bool grab_tty = session_interactivity() != session_interactivity_t::not_interactive && + isatty(streams.stdin_fd); pid_t pgroup_to_restore = grab_tty ? terminal_acquire_before_builtin(job_pgid) : -1; int ret = data->func(parser, streams, argv); if (pgroup_to_restore >= 0) { diff --git a/src/builtin_commandline.cpp b/src/builtin_commandline.cpp index f91db88b0..e8cf67f7f 100644 --- a/src/builtin_commandline.cpp +++ b/src/builtin_commandline.cpp @@ -160,7 +160,7 @@ int builtin_commandline(parser_t &parser, io_streams_t &streams, wchar_t **argv) } if (!current_buffer) { - if (is_interactive_session()) { + if (session_interactivity() != session_interactivity_t::not_interactive) { // 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 2fcbce962..34765bb21 100644 --- a/src/builtin_status.cpp +++ b/src/builtin_status.cpp @@ -384,7 +384,7 @@ int builtin_status(parser_t &parser, io_streams_t &streams, wchar_t **argv) { } case STATUS_IS_INTERACTIVE: { CHECK_FOR_UNEXPECTED_STATUS_ARGS(opts.status_cmd) - retval = !is_interactive_session(); + retval = session_interactivity() == session_interactivity_t::not_interactive ? 1 : 0; break; } case STATUS_IS_COMMAND_SUB: { diff --git a/src/env_dispatch.cpp b/src/env_dispatch.cpp index 1a747c677..41ce0cf62 100644 --- a/src/env_dispatch.cpp +++ b/src/env_dispatch.cpp @@ -394,11 +394,12 @@ 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 (is_interactive_session()) debug(1, _(L"Using fallback terminal type '%s'."), term); + if (session_interactivity() != session_interactivity_t::not_interactive) + debug(1, _(L"Using fallback terminal type '%s'."), term); int err_ret; if (setupterm(const_cast(term), STDOUT_FILENO, &err_ret) == OK) return true; - if (is_interactive_session()) { + if (session_interactivity() != session_interactivity_t::not_interactive) { debug(1, _(L"Could not set up terminal using the fallback terminal type '%s'."), term); } return false; @@ -457,7 +458,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 (is_interactive_session()) { + if (session_interactivity() != session_interactivity_t::not_interactive) { debug(1, _(L"Could not set up terminal.")); if (term.missing_or_empty()) { debug(1, _(L"TERM environment variable not set.")); diff --git a/src/fish.cpp b/src/fish.cpp index 82ff5387e..8b013c52e 100644 --- a/src/fish.cpp +++ b/src/fish.cpp @@ -396,7 +396,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(true); + set_interactive_session(session_interactivity_t::implied); } return optind; @@ -446,11 +446,11 @@ 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(true); + if (opts.is_interactive_session) set_interactive_session(session_interactivity_t::explicit_); // Only save (and therefore restore) the fg process group if we are interactive. See issues // #197 and #1002. - if (is_interactive_session()) { + if (session_interactivity() != session_interactivity_t::not_interactive) { save_term_foreground_process_group(); } diff --git a/src/fish_key_reader.cpp b/src/fish_key_reader.cpp index ef556df71..8f6a04969 100644 --- a/src/fish_key_reader.cpp +++ b/src/fish_key_reader.cpp @@ -287,7 +287,8 @@ static void install_our_signal_handlers() { /// Setup our environment (e.g., tty modes), process key strokes, then reset the environment. static void setup_and_process_keys(bool continuous_mode) { - set_interactive_session(true); // by definition this program is interactive + set_interactive_session( + session_interactivity_t::implied); // by definition this program is interactive set_main_thread(); setup_fork_guards(); env_init(); diff --git a/src/fish_tests.cpp b/src/fish_tests.cpp index b1597bb61..2ab4f7bc1 100644 --- a/src/fish_tests.cpp +++ b/src/fish_tests.cpp @@ -1069,10 +1069,10 @@ static void test_cancellation() { // Ensure that if child processes SIGINT, we exit our loops // Test for #3780 - // Ugly hack - temporarily set is_interactive_session + // Ugly hack - temporarily fake an interactive session // else we will SIGINT ourselves in response to our child death - bool iis = is_interactive_session(); - set_interactive_session(true); + session_interactivity_t iis = session_interactivity(); + set_interactive_session(session_interactivity_t::implied); const wchar_t *child_self_destructor = L"while true ; sh -c 'sleep .25; kill -s INT $$' ; end"; parser_t::principal_parser().eval(child_self_destructor, io_chain_t()); set_interactive_session(iis); diff --git a/src/proc.cpp b/src/proc.cpp index 71b17d3e2..4ac9c1ec8 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -54,9 +54,10 @@ /// The signals that signify crashes to us. static const int crashsignals[] = {SIGABRT, SIGBUS, SIGFPE, SIGILL, SIGSEGV, SIGSYS}; -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_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_login{false}; bool get_login() { return s_is_login; } @@ -245,7 +246,7 @@ static void handle_child_status(process_t *proc, proc_status_t status) { if (status.signal_exited()) { int sig = status.signal_code(); if (sig == SIGINT || sig == SIGQUIT) { - if (is_interactive_session()) { + if (session_interactivity() != session_interactivity_t::not_interactive) { // In an interactive session, tell the principal parser to skip all blocks we're // executing so control-C returns control to the user. parser_t::skip_all_blocks(); diff --git a/src/proc.h b/src/proc.h index c209007b3..5042f35c7 100644 --- a/src/proc.h +++ b/src/proc.h @@ -475,8 +475,9 @@ class job_t { }; /// Whether this shell is attached to the keyboard at all. -bool is_interactive_session(); -void set_interactive_session(bool flag); +enum class session_interactivity_t { not_interactive, implied, explicit_ }; +session_interactivity_t session_interactivity(); +void set_interactive_session(session_interactivity_t flag); /// Whether we are a login shell. bool get_login(); diff --git a/src/reader.cpp b/src/reader.cpp index a9be240d2..38d33a0a3 100644 --- a/src/reader.cpp +++ b/src/reader.cpp @@ -1734,7 +1734,7 @@ static void reader_interactive_init(parser_t &parser) { owner = tcgetpgrp(STDIN_FILENO); } if (owner == -1 && errno == ENOTTY) { - if (!is_interactive_session()) { + if (session_interactivity() == session_interactivity_t::not_interactive) { // 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; @@ -1772,7 +1772,8 @@ static void reader_interactive_init(parser_t &parser) { // 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) - if (shell_pgid == 0) { + // 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 (setpgid(shell_pgid, shell_pgid) < 0) { FLOG(error, _(L"Failed to assign shell to its own process group")); @@ -3239,7 +3240,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 || is_interactive_session()) { + if (err != ENOTTY || session_interactivity() != session_interactivity_t::not_interactive) { wperror(L"tcsetattr"); } } @@ -3342,7 +3343,8 @@ maybe_t reader_data_t::readline(int nchars_or_0) { if (!reader_exit_forced()) { // The order of the two conditions below is important. Try to restore the mode // in all cases, but only complain if interactive. - if (tcsetattr(0, TCSANOW, &old_modes) == -1 && is_interactive_session()) { + if (tcsetattr(0, TCSANOW, &old_modes) == -1 && + session_interactivity() != session_interactivity_t::not_interactive) { if (errno == EIO) redirect_tty_output(); wperror(L"tcsetattr"); // return to previous mode }