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

@@ -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
 -------------------------------

src/builtin_commandline.cpp

@@ -159,7 +159,7 @@ maybe_t<int> 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;

src/builtin_status.cpp

@@ -397,7 +397,7 @@ maybe_t<int> 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: {

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<char *>(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."));

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();
     }
 

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();

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<session_interactivity_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_t> &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 {

src/proc.h

@@ -480,10 +480,9 @@ class job_t {
     maybe_t<statuses_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();

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<wcstring> 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<wcstring> 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
         }