Change pty.spawn to not block indefinitely

When both file descriptors gracefully report an error. This presents custom implementations of *master_read* and *stdin_read* that accidentally or intentionally return an empty byte string from blocking the terminal. Co-Authored-By: Reilly Tucker Siemens <reilly@tuckersiemens.com>
ambv · Aug 11, 2021 · Feb 26, 2019 · Aug 11, 2021 · Aug 11, 2021 · 9613c9880c38475306e0f0bae7f404832428d2ed
commit 9613c9880c38475306e0f0bae7f404832428d2ed
diff --git a/Doc/library/pty.rst b/Doc/library/pty.rst
@@ -50,7 +50,7 @@ The :mod:`pty` module defines the following functions:
    The functions *master_read* and *stdin_read* are passed a file descriptor
    which they should read from, and they should always return a byte string. In
    order to force spawn to return before the child process exits an
-   :exc:`OSError` should be thrown.
+   empty byte array should be returned to signal end of file.
 
    The default implementation for both functions will read and return up to 1024
    bytes each time the function is called. The *master_read* callback is passed
@@ -65,10 +65,6 @@ The :mod:`pty` module defines the following functions:
    process will quit without any input, *spawn* will then loop forever. If
    *master_read* signals EOF the same behavior results (on linux at least).
 
-   If both callbacks signal EOF then *spawn* will probably never return, unless
-   *select* throws an error on your platform when passed three empty lists. This
-   is a bug, documented in `issue 26228 <https://bugs.python.org/issue26228>`_.
-
    Return the exit status value from :func:`os.waitpid` on the child process.
 
    :func:`waitstatus_to_exitcode` can be used to convert the exit status into

diff --git a/Lib/pty.py b/Lib/pty.py
@@ -7,11 +7,13 @@
 # Author: Steen Lumholt -- with additions by Guido.
 
 from select import select
+from os import close, waitpid
 import os
 import sys
+from tty import setraw, tcgetattr, tcsetattr
 import tty
 
-__all__ = ["openpty","fork","spawn"]
+__all__ = ["openpty", "fork", "spawn"]
 
 STDIN_FILENO = 0
 STDOUT_FILENO = 1
@@ -105,8 +107,8 @@ def fork():
         os.dup2(slave_fd, STDIN_FILENO)
         os.dup2(slave_fd, STDOUT_FILENO)
         os.dup2(slave_fd, STDERR_FILENO)
-        if (slave_fd > STDERR_FILENO):
-            os.close (slave_fd)
+        if slave_fd > STDERR_FILENO:
+            os.close(slave_fd)
 
         # Explicitly open the tty to make it become a controlling tty.
         tmp_fd = os.open(os.ttyname(STDOUT_FILENO), os.O_RDWR)
@@ -133,14 +135,22 @@ def _copy(master_fd, master_read=_read, stdin_read=_read):
             pty master -> standard output   (master_read)
             standard input -> pty master    (stdin_read)"""
     fds = [master_fd, STDIN_FILENO]
-    while True:
-        rfds, wfds, xfds = select(fds, [], [])
+    while fds:
+        rfds, _wfds, _xfds = select(fds, [], [])
+
         if master_fd in rfds:
-            data = master_read(master_fd)
+            # Some OSes signal EOF by returning an empty byte string,
+            # some throw OSErrors.
+            try:
+                data = master_read(master_fd)
+            except OSError:
+                data = b""
             if not data:  # Reached EOF.
-                fds.remove(master_fd)
+                return    # Assume the child process has exited and is
+                          # unreachable, so we clean up.
             else:
                 os.write(STDOUT_FILENO, data)
+
         if STDIN_FILENO in rfds:
             data = stdin_read(STDIN_FILENO)
             if not data:
@@ -153,20 +163,23 @@ def spawn(argv, master_read=_read, stdin_read=_read):
     if type(argv) == type(''):
         argv = (argv,)
     sys.audit('pty.spawn', argv)
+
     pid, master_fd = fork()
     if pid == CHILD:
         os.execlp(argv[0], *argv)
+
     try:
-        mode = tty.tcgetattr(STDIN_FILENO)
-        tty.setraw(STDIN_FILENO)
-        restore = 1
+        mode = tcgetattr(STDIN_FILENO)
+        setraw(STDIN_FILENO)
+        restore = True
     except tty.error:    # This is the same as termios.error
-        restore = 0
+        restore = False
+
     try:
         _copy(master_fd, master_read, stdin_read)
-    except OSError:
+    finally:
         if restore:
-            tty.tcsetattr(STDIN_FILENO, tty.TCSAFLUSH, mode)
+            tcsetattr(STDIN_FILENO, tty.TCSAFLUSH, mode)
 
-    os.close(master_fd)
-    return os.waitpid(pid, 0)[1]
+    close(master_fd)
+    return waitpid(pid, 0)[1]
diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py
@@ -5,8 +5,9 @@
 import_module('termios')
 
 import errno
-import pty
 import os
+import pty
+import tty
 import sys
 import select
 import signal
@@ -330,16 +331,27 @@ class SmallPtyTests(unittest.TestCase):
     def setUp(self):
         self.orig_stdin_fileno = pty.STDIN_FILENO
         self.orig_stdout_fileno = pty.STDOUT_FILENO
+        self.orig_pty_fork = pty.fork
         self.orig_pty_select = pty.select
+        self.orig_pty_setraw = pty.setraw
+        self.orig_pty_tcgetattr = pty.tcgetattr
+        self.orig_pty_tcsetattr = pty.tcsetattr
+        self.orig_pty_waitpid = pty.waitpid
         self.fds = []  # A list of file descriptors to close.
         self.files = []
         self.select_rfds_lengths = []
         self.select_rfds_results = []
+        self.tcsetattr_mode_setting = None
 
     def tearDown(self):
         pty.STDIN_FILENO = self.orig_stdin_fileno
         pty.STDOUT_FILENO = self.orig_stdout_fileno
+        pty.fork = self.orig_pty_fork
         pty.select = self.orig_pty_select
+        pty.setraw = self.orig_pty_setraw
+        pty.tcgetattr = self.orig_pty_tcgetattr
+        pty.tcsetattr = self.orig_pty_tcsetattr
+        pty.waitpid = self.orig_pty_waitpid
         for file in self.files:
             try:
                 file.close()
@@ -367,6 +379,14 @@ def _mock_select(self, rfds, wfds, xfds, timeout=0):
         self.assertEqual(self.select_rfds_lengths.pop(0), len(rfds))
         return self.select_rfds_results.pop(0), [], []
 
+    def _make_mock_fork(self, pid):
+        def mock_fork():
+            return (pid, 12)
+        return mock_fork
+
+    def _mock_tcsetattr(self, fileno, opt, mode):
+        self.tcsetattr_mode_setting = mode
+
     def test__copy_to_each(self):
         """Test the normal data case on both master_fd and stdin."""
         read_from_stdout_fd, mock_stdout_fd = self._pipe()
@@ -407,20 +427,41 @@ def test__copy_eof_on_all(self):
         socketpair[1].close()
         os.close(write_to_stdin_fd)
 
-        # Expect two select calls, the last one will cause IndexError
         pty.select = self._mock_select
         self.select_rfds_lengths.append(2)
         self.select_rfds_results.append([mock_stdin_fd, masters[0]])
         # We expect that both fds were removed from the fds list as they
         # both encountered an EOF before the second select call.
         self.select_rfds_lengths.append(0)
 
-        with self.assertRaises(IndexError):
-            pty._copy(masters[0])
+        # We expect the function to return without error.
+        self.assertEqual(pty._copy(masters[0]), None)
+
+    def test__restore_tty_mode_normal_return(self):
+        """Test that spawn resets the tty mode no when _copy returns normally."""
+
+        # PID 1 is returned from mocked fork to run the parent branch
+        # of code
+        pty.fork = self._make_mock_fork(1)
+
+        status_sentinel = object()
+        pty.waitpid = lambda _1, _2: [None, status_sentinel]
+        pty.close = lambda _: None
+
+        pty._copy = lambda _1, _2, _3: None
+
+        mode_sentinel = object()
+        pty.tcgetattr = lambda fd: mode_sentinel
+        pty.tcsetattr = self._mock_tcsetattr
+        pty.setraw = lambda _: None
+
+        self.assertEqual(pty.spawn([]), status_sentinel, "pty.waitpid process status not returned by pty.spawn")
+        self.assertEqual(self.tcsetattr_mode_setting, mode_sentinel, "pty.tcsetattr not called with original mode value")
 
 
 def tearDownModule():
     reap_children()
 
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/Misc/ACKS b/Misc/ACKS
@@ -1622,6 +1622,7 @@ Yue Shuaijie
 Jaysinh Shukla
 Terrel Shumway
 Eric Siegerman
+Reilly Tucker Siemens
 Paul Sijben
 SilentGhost
 Tim Silk

diff --git a/Misc/NEWS.d/next/Library/2019-02-26-09-31-59.bpo-26228.wyrHKc.rst b/Misc/NEWS.d/next/Library/2019-02-26-09-31-59.bpo-26228.wyrHKc.rst
@@ -0,0 +1 @@
+pty.spawn no longer hangs on FreeBSD, OS X, and Solaris.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		pty.spawn no longer hangs on FreeBSD, OS X, and Solaris.