Bravely remove reclaim... param from continue_job, and rework tcsetpgrp calls

This changes how fish attempts to protect itself from calling tcsetpgrp() too
aggressively. Recall that tcsetpgrp() will "force" itself, if SIGTTOU is
ignored (which it is in fish when job control is enabled).

Prior to this fix, we avoided SIGTTINs by only transferring the tty ownership
if fish was already the owner. This dated from a time before we had really
nailed down how pgroups should be assigned. Now we more deliberately assign a
job's pgroup so we don't need this conservative check.

However we still need logic to avoid transferring the tty if fish is not the
owner. The bad case is when job control is enabled while fish is running in the
background - here fish would transfer the tty and "steal" from the foreground
process.

So retain the checks of the current tty owner but migrate them to the point of
calling tcsetpgrp() itself.

src/builtin_bg.cpp

@@ -32,7 +32,7 @@ static int send_to_bg(parser_t &parser, io_streams_t &streams, job_t *j) {
                               j->command_wcstr());
     parser.job_promote(j);
     j->group->set_is_foreground(false);
-    j->continue_job(parser, true);
+    j->continue_job(parser);
     return STATUS_CMD_OK;
 }
 

src/builtin_fg.cpp

@@ -107,6 +107,6 @@ int builtin_fg(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     parser.job_promote(job);
     job->group->set_is_foreground(true);
 
-    job->continue_job(parser, true);
+    job->continue_job(parser);
     return STATUS_CMD_OK;
 }

src/exec.cpp

@@ -156,7 +156,7 @@ static void internal_exec(env_stack_t &vars, job_t *j, const io_chain_t &block_i
 
     // child_setup_process makes sure signals are properly set up.
     dup2_list_t redirs = dup2_list_t::resolve_chain(all_ios);
-    if (child_setup_process(INVALID_PID, *j, false, redirs) == 0) {
+    if (child_setup_process(INVALID_PID, INVALID_PID, *j, false, redirs) == 0) {
         // Decrement SHLVL as we're removing ourselves from the shell "stack".
         auto shlvl_var = vars.get(L"SHLVL", ENV_GLOBAL | ENV_EXPORT);
         wcstring shlvl_str = L"0";
@@ -311,6 +311,11 @@ static bool fork_child_for_process(const std::shared_ptr<job_t> &job, process_t
                                    const dup2_list_t &dup2s, const char *fork_type,
                                    const std::function<void()> &child_action) {
     assert(!job->group->is_internal() && "Internal groups should never need to fork");
+    // Decide if we want to job to control the tty.
+    // If so we need to get our pgroup; if not we don't need the pgroup.
+    bool claim_tty = job->group->should_claim_terminal();
+    pid_t fish_pgrp = claim_tty ? getpgrp() : INVALID_PID;
+
     pid_t pid = execute_fork();
     if (pid == 0) {
         // This is the child process. Setup redirections, print correct output to
@@ -322,8 +327,7 @@ static bool fork_child_for_process(const std::shared_ptr<job_t> &job, process_t
         if (int err = execute_setpgid(p->pid, pgid, false /* not parent */)) {
             report_setpgid_error(err, pgid, job.get(), p);
         }
-        child_setup_process(job->group->should_claim_terminal() ? pgid : INVALID_PID, *job, true,
-                            dup2s);
+        child_setup_process(claim_tty ? pgid : INVALID_PID, fish_pgrp, *job, true, dup2s);
         child_action();
         DIE("Child process returned control to fork_child lambda!");
     }
@@ -902,9 +906,6 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const io_chain_t &bl
         return true;
     }
 
-    pid_t pgrp = getpgrp();
-    // Check to see if we should reclaim the foreground pgrp after the job finishes or stops.
-    const bool reclaim_foreground_pgrp = (tcgetpgrp(STDIN_FILENO) == pgrp);
     const size_t stdout_read_limit = parser.libdata().read_limit;
 
     // Get the list of all FDs so we can ensure our pipes do not conflict.
@@ -1003,7 +1004,7 @@ bool exec_job(parser_t &parser, const shared_ptr<job_t> &j, const io_chain_t &bl
         return false;
     }
 
-    j->continue_job(parser, reclaim_foreground_pgrp);
+    j->continue_job(parser);
     return true;
 }
 

src/fish_test_helper.cpp

@@ -20,7 +20,7 @@ static void become_foreground_then_print_stderr() {
     fprintf(stderr, "become_foreground_then_print_stderr done\n");
 }
 
-static void report_foreground() {
+static void report_foreground_loop() {
     int was_fg = -1;
     const auto grp = getpgrp();
     for (;;) {
@@ -35,6 +35,11 @@ static void report_foreground() {
     }
 }
 
+static void report_foreground() {
+    bool is_fg = (tcgetpgrp(STDIN_FILENO) == getpgrp());
+    fputs(is_fg ? "foreground\n" : "background\n", stderr);
+}
+
 static void sigint_parent() {
     // SIGINT the parent after a time, then exit
     int parent = getppid();
@@ -110,7 +115,8 @@ struct fth_command_t {
 static fth_command_t s_commands[] = {
     {"become_foreground_then_print_stderr", become_foreground_then_print_stderr,
      "Claim the terminal (tcsetpgrp) and then print to stderr"},
-    {"report_foreground", report_foreground,
+    {"report_foreground", report_foreground, "Report to stderr whether we own the terminal"},
+    {"report_foreground_loop", report_foreground_loop,
      "Continually report to stderr whether we own the terminal"},
     {"sigint_parent", sigint_parent, "Wait .25 seconds, then SIGINT the parent process"},
     {"print_stdout_stderr", print_stdout_stderr, "Print 'stdout' to stdout and 'stderr' to stderr"},

src/postfork.cpp

@@ -98,7 +98,7 @@ int execute_setpgid(pid_t pid, pid_t pgroup, bool is_parent) {
     }
 }
 
-int child_setup_process(pid_t new_termowner, const job_t &job, bool is_forked,
+int child_setup_process(pid_t new_termowner, pid_t fish_pgrp, const job_t &job, bool is_forked,
                         const dup2_list_t &dup2s) {
     // Note we are called in a forked child.
     for (const auto &act : dup2s.get_actions()) {
@@ -122,12 +122,16 @@ int child_setup_process(pid_t new_termowner, const job_t &job, bool is_forked,
             return err;
         }
     }
-    if (new_termowner != INVALID_PID) {
+    if (new_termowner != INVALID_PID && new_termowner != fish_pgrp) {
         // Assign the terminal within the child to avoid the well-known race between tcsetgrp() in
         // the parent and the child executing. We are not interested in error handling here, except
         // we try to avoid this for non-terminals; in particular pipelines often make non-terminal
         // stdin.
-        if (isatty(STDIN_FILENO)) {
+        // Only do this if the tty currently belongs to fish's pgrp. Don't try to steal it away from
+        // another process which may happen if we are run in the background with job control
+        // enabled. Note if stdin is not a tty, then tcgetpgrp() will return -1 and we will not
+        // enter this.
+        if (tcgetpgrp(STDIN_FILENO) == fish_pgrp) {
             // Ensure this doesn't send us to the background (see #5963)
             signal(SIGTTIN, SIG_IGN);
             signal(SIGTTOU, SIG_IGN);

src/postfork.h

@@ -42,7 +42,7 @@ void report_setpgid_error(int err, pid_t desired_pgid, const job_t *j, const pro
 ///
 /// \return 0 on success, -1 on failure. When this function returns, signals are always unblocked.
 /// On failure, signal handlers, io redirections and process group of the process is undefined.
-int child_setup_process(pid_t new_termowner, const job_t &job, bool is_forked,
+int child_setup_process(pid_t new_termowner, pid_t fish_pgrp, const job_t &job, bool is_forked,
                         const dup2_list_t &dup2s);
 
 /// Call fork(), retrying on failure a few times.

src/proc.cpp

@@ -748,10 +748,48 @@ int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from
     }
 
     // If we are continuing, ensure that stdin is marked as blocking first (issue #176).
+    // Also restore tty modes.
     if (continuing_from_stopped) {
         make_fd_blocking(STDIN_FILENO);
+        if (jg->tmodes.has_value()) {
+            int res = tcsetattr(STDIN_FILENO, TCSADRAIN, &jg->tmodes.value());
+            if (res < 0) wperror(L"tcsetattr");
+        }
     }
 
+    // Ok, we want to transfer to the child.
+    // Note it is important to be very careful about calling tcsetpgrp()!
+    // fish ignores SIGTTOU which means that it has the power to reassign the tty even if it doesn't
+    // own it. This means that other processes may get SIGTTOU and become zombies.
+    // Check who own the tty now. Thre's five cases of interest:
+    //   1. The process's pgrp is the same as fish. In that case there is nothing to do.
+    //   2. There is no tty at all (tcgetpgrp() returns -1). For example running from a pure script.
+    //      Of course do not transfer it in that case.
+    //   3. The tty is owned by the process. This comes about often, as the process will call
+    //      tcsetpgrp() on itself between fork ane exec. This is the essential race inherent in
+    //      tcsetpgrp(). In this case we want to reclaim the tty, but do not need to transfer it
+    //      ourselves since the child won the race.
+    //   4. The tty is owned by a different process. This may come about if fish is running in the
+    //      background with job control enabled. Do not transfer it.
+    //   5. The tty is owned by fish. In that case we want to transfer the pgid.
+    pid_t fish_pgrp = getpgrp();
+    if (fish_pgrp == pgid) {
+        // Case 1.
+        return notneeded;
+    }
+    pid_t current_owner = tcgetpgrp(STDIN_FILENO);
+    if (current_owner < 0) {
+        // Case 2.
+        return notneeded;
+    } else if (current_owner == pgid) {
+        // Case 3.
+        return success;
+    } else if (current_owner != pgid && current_owner != fish_pgrp) {
+        // Case 4.
+        return notneeded;
+    }
+    // Case 5 - we do want to transfer it.
+
     // The tcsetpgrp(2) man page says that EPERM is thrown if "pgrp has a supported value, but
     // is not the process group ID of a process in the same session as the calling process."
     // Since we _guarantee_ that this isn't the case (the child calls setpgid before it calls
@@ -835,13 +873,6 @@ int terminal_maybe_give_to_job_group(const job_group_t *jg, bool continuing_from
         break;
     }
 
-    if (continuing_from_stopped && jg->tmodes.has_value()) {
-        int result = tcsetattr(STDIN_FILENO, TCSADRAIN, &jg->tmodes.value());
-        if (result == -1) {
-            wperror(L"tcsetattr");
-        }
-    }
-
     return success;
 }
 
@@ -896,7 +927,7 @@ maybe_t<pid_t> job_t::get_pgid() const { return group->get_pgid(); }
 
 job_id_t job_t::job_id() const { return group->get_id(); }
 
-void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp) {
+void job_t::continue_job(parser_t &parser) {
     // Put job first in the job list.
     parser.job_promote(this);
     mut_flags().notified = false;
@@ -915,11 +946,15 @@ void job_t::continue_job(parser_t &parser, bool reclaim_foreground_pgrp) {
     // Make sure we retake control of the terminal before leaving this function.
     bool term_transferred = false;
     cleanup_t take_term_back([&] {
-        if (term_transferred && reclaim_foreground_pgrp) {
-            // Only restore terminal attrs if we're continuing a job. See:
+        if (term_transferred) {
+            // Should we restore the terminal attributes?
+            // Historically we have done this conditionally only if we sent SIGCONT.
+            // TODO: rationalize what the right behavior here is.
+            bool restore_attrs = send_sigcont;
+            // Issues of interest:
             // https://github.com/fish-shell/fish-shell/issues/121
             // https://github.com/fish-shell/fish-shell/issues/2114
-            terminal_return_from_job_group(this->group.get(), send_sigcont);
+            terminal_return_from_job_group(this->group.get(), restore_attrs);
         }
     });
 

src/proc.h

@@ -456,10 +456,7 @@ class job_t {
 
     /// Continues running a job, which may be stopped, or may just have started.
     /// This will send SIGCONT if the job is stopped.
-    ///
-    /// \param reclaim_foreground_pgrp whether, when the job finishes or stops, to reclaim the
-    /// foreground pgrp (via tcsetpgrp).
-    void continue_job(parser_t &parser, bool reclaim_foreground_pgrp);
+    void continue_job(parser_t &parser);
 
     /// Send the specified signal to all processes in this job.
     /// \return true on success, false on failure.

tests/checks/job-control-noninteractive.fish

@@ -12,3 +12,10 @@ test $first -ne $second
 and echo "pgroups differed, meaning job control worked"
 or echo "pgroups were the same, job control did not work"
 #CHECK: pgroups differed, meaning job control worked
+
+# fish ignores SIGTTIN and so may transfer the tty even if it
+# doesn't own the tty. Ensure that doesn't happen.
+set -l fish (status fish-path)
+$fish -c 'status job-control full ; $fth report_foreground' &
+wait
+#CHECKERR: background

tests/pexpects/tty_ownership.py

@@ -0,0 +1,15 @@
+#!/usr/bin/env python3
+from pexpect_helper import SpawnedProc
+
+sp = SpawnedProc()
+sendline, expect_prompt = sp.sendline, sp.expect_prompt
+
+expect_prompt()
+sendline("status job-control full")
+expect_prompt()
+
+sendline("$fish -c 'status job-control full ; $fish_test_helper report_foreground' &; wait")
+expect_prompt()
+
+sendline("echo it worked")
+expect_prompt("it worked")