diff --git a/src/builtin_bg.cpp b/src/builtin_bg.cpp index fdde21c29..ca05442fd 100644 --- a/src/builtin_bg.cpp +++ b/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; } diff --git a/src/builtin_fg.cpp b/src/builtin_fg.cpp index 314becb10..7bc05f467 100644 --- a/src/builtin_fg.cpp +++ b/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; } diff --git a/src/exec.cpp b/src/exec.cpp index 3b65b4fe4..c995594ce 100644 --- a/src/exec.cpp +++ b/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, process_t const dup2_list_t &dup2s, const char *fork_type, const std::function &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, 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 &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 &j, const io_chain_t &bl return false; } - j->continue_job(parser, reclaim_foreground_pgrp); + j->continue_job(parser); return true; } diff --git a/src/fish_test_helper.cpp b/src/fish_test_helper.cpp index 16f6032b4..988b85d5d 100644 --- a/src/fish_test_helper.cpp +++ b/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"}, diff --git a/src/postfork.cpp b/src/postfork.cpp index fc9c0f879..6bb5b20aa 100644 --- a/src/postfork.cpp +++ b/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); diff --git a/src/postfork.h b/src/postfork.h index 0b41976ab..97ddbf572 100644 --- a/src/postfork.h +++ b/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. diff --git a/src/proc.cpp b/src/proc.cpp index 06dc8ba53..3e213b44f 100644 --- a/src/proc.cpp +++ b/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 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); } }); diff --git a/src/proc.h b/src/proc.h index 89a65ea34..2541d85eb 100644 --- a/src/proc.h +++ b/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. diff --git a/tests/checks/job-control-noninteractive.fish b/tests/checks/job-control-noninteractive.fish index a19dd5738..dc166335d 100644 --- a/tests/checks/job-control-noninteractive.fish +++ b/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 diff --git a/tests/pexpects/tty_ownership.py b/tests/pexpects/tty_ownership.py new file mode 100644 index 000000000..237ff5136 --- /dev/null +++ b/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")