From 5cf077820727656778e96abbc1f88955db41dbcc Mon Sep 17 00:00:00 2001
From: ridiculousfish <rf@fishshell.com>
Date: Thu, 8 Sep 2022 16:47:43 -0700
Subject: [PATCH] Claim the tty unconditionally in reader_data_t::readline

When fish runs with job control enabled, it transfers ownership of the
tty to a child process, and then reclaims the tty after the process
exits. If job control is disabled then fish does not transfer or reclaim
the tty.

It may happen that the child process creates a pgroup and then transfers
the tty to it. In that case fish will not attempt to reclaim the tty, as
fish did not transfer it. Then when fish reads from stdin it will
receive SIGTTIN instead of data.

Fix this by unconditionally claiming the tty in readline().

Fixes #9181
---
 CHANGELOG.rst                   |  1 +
 src/fish_test_helper.cpp        | 23 +++++++++++++++++++++++
 src/reader.cpp                  |  5 +++++
 tests/pexpects/tty_ownership.py | 12 ++++++++++++
 4 files changed, 41 insertions(+)

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 0bd714f12..b8de09673 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -69,6 +69,7 @@ Interactive improvements
 - A crash when completing a token that contained both a potential glob and a quoted variable expansion was fixed (:issue:`9137`).
 - If ``$fish_color_valid_path`` contains an actual color instead of just modifiers, those will be used for valid paths even if the underlying color isn't "normal" (:issue:`9159`).
 - ``prompt_pwd`` no longer accidentally overwrites a global or universal ``$fish_prompt_pwd_full_dirs`` when called with the ``-d`` or ``--full-length-dirs`` option (:issue:`9123`).
+- A bug which caused fish to freeze or exit after running a command which does not preserve the foreground process group was fixed (:issue:`9181`).
 
 New or improved bindings
 ^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/src/fish_test_helper.cpp b/src/fish_test_helper.cpp
index 009442579..cac2838d8 100644
--- a/src/fish_test_helper.cpp
+++ b/src/fish_test_helper.cpp
@@ -2,6 +2,7 @@
 // programs, allowing fish to test its behavior.
 
 #include <fcntl.h>
+#include <sys/wait.h>
 #include <unistd.h>
 
 #include <algorithm>
@@ -11,6 +12,27 @@
 #include <cstring>
 #include <iterator>  // for std::begin/end
 
+static void abandon_tty() {
+    pid_t pid = fork();
+    if (pid < 0) {
+        perror("fork");
+        exit(EXIT_FAILURE);
+    }
+    // Both parent and child do the same thing.
+    pid_t child = pid ? pid : getpid();
+    if (setpgid(child, child)) {
+        perror("setpgid");
+        exit(EXIT_FAILURE);
+    }
+    // tcsetpgrp may fail in the parent if the child has already exited.
+    // This is the benign race.
+    (void)tcsetpgrp(STDIN_FILENO, child);
+    // Parent waits for child to exit.
+    if (pid > 0) {
+        waitpid(child, nullptr, 0);
+    }
+}
+
 static void become_foreground_then_print_stderr() {
     if (tcsetpgrp(STDOUT_FILENO, getpgrp()) < 0) {
         perror("tcsetgrp");
@@ -192,6 +214,7 @@ struct fth_command_t {
 };
 
 static fth_command_t s_commands[] = {
+    {"abandon_tty", abandon_tty, "Create a new pgroup and transfer tty ownership to it"},
     {"become_foreground_then_print_stderr", become_foreground_then_print_stderr,
      "Claim the terminal (tcsetpgrp) and then print to stderr"},
     {"nohup_wait", nohup_wait, "Ignore SIGHUP and just wait"},
diff --git a/src/reader.cpp b/src/reader.cpp
index c67234bd3..06d349e26 100644
--- a/src/reader.cpp
+++ b/src/reader.cpp
@@ -4278,6 +4278,11 @@ maybe_t<wcstring> reader_data_t::readline(int nchars_or_0) {
 
     history_search.reset();
 
+    // It may happen that a command we ran when job control was disabled nevertheless stole the tty
+    // from us. In that case when we read from our fd, it will trigger SIGTTIN. So just
+    // unconditionally reclaim the tty. See #9181.
+    (void)tcsetpgrp(conf.in, getpgrp());
+
     // Get the current terminal modes. These will be restored when the function returns.
     struct termios old_modes {};
     if (tcgetattr(conf.in, &old_modes) == -1 && errno == EIO) redirect_tty_output();
diff --git a/tests/pexpects/tty_ownership.py b/tests/pexpects/tty_ownership.py
index 46794556d..39c6c38be 100644
--- a/tests/pexpects/tty_ownership.py
+++ b/tests/pexpects/tty_ownership.py
@@ -15,3 +15,15 @@ expect_prompt()
 
 sendline("echo it worked")
 expect_prompt("it worked")
+
+# Regression test for #9181
+sendline("status job-control interactive")
+expect_prompt()
+sendline("$fish_test_helper abandon_tty")
+expect_prompt()
+sendline("echo cool")
+expect_prompt("cool")
+sendline("true ($fish_test_helper abandon_tty)")
+expect_prompt()
+sendline("echo even cooler")
+expect_prompt("even cooler")