bpo-26228: pty.spawn hangs on FreeBSD, OS X, and Solaris

Doc/library/pty.rst

@@ -2,8 +2,8 @@
 ========================================
 
 .. module:: pty
-   :platform: Linux
-   :synopsis: Pseudo-Terminal Handling for Linux.
+   :platform: Unix
+   :synopsis: Pseudo-Terminal Handling for Unix.
 
 .. moduleauthor:: Steen Lumholt
 .. sectionauthor:: Moshe Zadka <[email protected]>
@@ -16,9 +16,9 @@ The :mod:`pty` module defines operations for handling the pseudo-terminal
 concept: starting another process and being able to write to and read from its
 controlling terminal programmatically.
 
-Because pseudo-terminal handling is highly platform dependent, there is code to
-do it only for Linux. (The Linux code is supposed to work on other platforms,
-but hasn't been tested yet.)
+Pseudo-terminal handling is highly platform dependent. This code is mainly
+tested on Linux, FreeBSD, and OS X (it is supposed to work on other POSIX
+platforms).
 
 The :mod:`pty` module defines the following functions:
 
@@ -41,9 +41,13 @@ The :mod:`pty` module defines the following functions:
 
 .. function:: spawn(argv[, master_read[, stdin_read]])
 
-   Spawn a process, and connect its controlling terminal with the current
-   process's standard io. This is often used to baffle programs which insist on
-   reading from the controlling terminal.
+   Spawn a child process, and connect its controlling terminal with the
+   current process's standard io. This is often used to baffle programs which
+   insist on reading from the controlling terminal.
+
+   A loop copies STDIN of the current process to the child and data received
+   from the child to STDOUT of the current process. It is not signaled to the
+   child if STDIN of the current process closes down.
 
    The functions *master_read* and *stdin_read* should be functions which read from
    a file descriptor. The defaults try to read 1024 bytes each time they are

Lib/pty.py

@@ -1,13 +1,14 @@
 """Pseudo terminal utilities."""
 
 # Bugs: No signal handling.  Doesn't set slave termios and window size.
-#       Only tested on Linux.
+#       Only tested on Linux, FreeBSD, and OS X.
 # See:  W. Richard Stevens. 1992.  Advanced Programming in the
 #       UNIX Environment.  Chapter 19.
 # Author: Steen Lumholt -- with additions by Guido.
 
 from select import select
 import os
+import sys
 import tty
 
 __all__ = ["openpty","fork","spawn"]
@@ -133,11 +134,16 @@ def _copy(master_fd, master_read=_read, stdin_read=_read):
             standard input -> pty master    (stdin_read)"""
     fds = [master_fd, STDIN_FILENO]
     while True:
+        # The expected path to leave this infinite loop is that the
+        # child exits and its slave_fd is destroyed. In this case,
+        # master_fd will become ready in select() and reading from
+        # master_fd either raises an OSError (Input/output error) on
+        # Linux or returns EOF on BSD.
         rfds, wfds, xfds = select(fds, [], [])
         if master_fd in rfds:
             data = master_read(master_fd)
             if not data:  # Reached EOF.
-                fds.remove(master_fd)
+                return
             else:
                 os.write(STDOUT_FILENO, data)
         if STDIN_FILENO in rfds:
@@ -153,7 +159,15 @@ def spawn(argv, master_read=_read, stdin_read=_read):
         argv = (argv,)
     pid, master_fd = fork()
     if pid == CHILD:
-        os.execlp(argv[0], *argv)
+        try:
+            os.execlp(argv[0], *argv)
+        except:
+            # If we wanted to be really clever, we would use
+            # the same method as subprocess() to pass the error
+            # back to the parent.  For now just dump stack trace.
+            sys.excepthook(*sys.exc_info())
+        finally:
+            os._exit(1)
     try:
         mode = tty.tcgetattr(STDIN_FILENO)
         tty.setraw(STDIN_FILENO)
@@ -163,6 +177,10 @@ def spawn(argv, master_read=_read, stdin_read=_read):
     try:
         _copy(master_fd, master_read, stdin_read)
     except OSError:
+        # Some OSes never return an EOF on pty, just raise
+        # an error instead.
+        pass
+    finally:
         if restore:
             tty.tcsetattr(STDIN_FILENO, tty.TCSAFLUSH, mode)
 

Lib/test/test_pty.py

@@ -211,8 +211,15 @@ def test_fork(self):
         # pty.fork() passed.
 
 
+class _MockSelectEternalWait(Exception):
+    """Used both as exception and placeholder value.  Models that no
+    more select activity is expected and that a test can be
+    terminated."""
+    pass
+
 class SmallPtyTests(unittest.TestCase):
-    """These tests don't spawn children or hang."""
+    """Whitebox mocking tests which don't spawn children or hang.  Test
+    the _copy loop to transfer data between parent and child."""
 
     def setUp(self):
         self.orig_stdin_fileno = pty.STDIN_FILENO
@@ -223,6 +230,10 @@ def setUp(self):
         self.select_rfds_lengths = []
         self.select_rfds_results = []
 
+        # monkey-patch and replace with mock
+        pty.select = self._mock_select
+        self._mock_stdin_stdout()
+
     def tearDown(self):
         pty.STDIN_FILENO = self.orig_stdin_fileno
         pty.STDOUT_FILENO = self.orig_stdout_fileno
@@ -243,67 +254,154 @@ def _pipe(self):
         self.fds.extend(pipe_fds)
         return pipe_fds
 
-    def _socketpair(self):
-        socketpair = socket.socketpair()
-        self.files.extend(socketpair)
-        return socketpair
+    def _mock_stdin_stdout(self):
+        """Mock STDIN and STDOUT with two fresh pipes.  Replaces
+        pty.STDIN_FILENO/pty.STDOUT_FILENO by one end of the pipe.
+        Makes the other end of the pipe available via self."""
+        self.read_from_stdout_fd, pty.STDOUT_FILENO = self._pipe()
+        pty.STDIN_FILENO, self.write_to_stdin_fd = self._pipe()
 
     def _mock_select(self, rfds, wfds, xfds):
+        """Simulates the behavior of select.select.  Only implemented
+        for reader waiting list (first parameter)."""
+        assert wfds == [] and xfds == []
         # This will raise IndexError when no more expected calls exist.
         self.assertEqual(self.select_rfds_lengths.pop(0), len(rfds))
-        return self.select_rfds_results.pop(0), [], []
+        if len(rfds) == 0:
+            # called with three empty lists as file descriptors to wait
+            # on.  Behavior of real select is platform-dependent and
+            # likely infinite blocking on Linux.
+            self.fail("mock select on no waitables")
+        rfds_result = self.select_rfds_results.pop(0)
+
+        if rfds_result is _MockSelectEternalWait:
+            raise _MockSelectEternalWait
+        return rfds_result, [], []
+
+    def test__mock_stdin_stdout(self):
+        """Test that _mock_stdin_stdout was called during setUp."""
+        self.assertGreater(pty.STDIN_FILENO, 2, "stdin replaced by our mock")
+        self.assertGreater(pty.STDOUT_FILENO, 2, "stdout replaced by our mock")
+
+    def test__mock_select(self):
+        # Test the select proxy of this test class. Meta testing.
+        self.select_rfds_lengths.append(0)
+        with self.assertRaises(AssertionError):
+            self._mock_select([], [], [])
+
+        # Prepare two select calls. Second one will block forever.
+        self.select_rfds_lengths.append(3)
+        self.select_rfds_results.append("foo")
+        self.select_rfds_lengths.append(3)
+        self.select_rfds_results.append(_MockSelectEternalWait)
+
+        # Call one
+        self.assertEqual(self._mock_select([1, 2, 3], [], []),
+                         ("foo", [], []))
+
+        # Call two
+        with self.assertRaises(_MockSelectEternalWait):
+            self._mock_select([1, 2, 3], [], [])
+
+        # lists are cleaned
+        self.assertEqual(self.select_rfds_lengths, [])
+        self.assertEqual(self.select_rfds_results, [])
+
+    def _socketpair(self):
+        socketpair = socket.socketpair()
+        self.files.extend(socketpair)
+        return socketpair
 
     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()
-        pty.STDOUT_FILENO = mock_stdout_fd
-        mock_stdin_fd, write_to_stdin_fd = self._pipe()
-        pty.STDIN_FILENO = mock_stdin_fd
-        socketpair = self._socketpair()
-        masters = [s.fileno() for s in socketpair]
+        # Test the normal data case on both master_fd and stdin.
+        masters = [s.fileno() for s in self._socketpair()]
 
         # Feed data.  Smaller than PIPEBUF.  These writes will not block.
         os.write(masters[1], b'from master')
-        os.write(write_to_stdin_fd, b'from stdin')
+        os.write(self.write_to_stdin_fd, b'from stdin')
 
-        # Expect two select calls, the last one will cause IndexError
-        pty.select = self._mock_select
+        # Expect two select calls, the last one will simulate eternal waiting
         self.select_rfds_lengths.append(2)
-        self.select_rfds_results.append([mock_stdin_fd, masters[0]])
+        self.select_rfds_results.append([pty.STDIN_FILENO, masters[0]])
         self.select_rfds_lengths.append(2)
+        self.select_rfds_results.append(_MockSelectEternalWait)
 
-        with self.assertRaises(IndexError):
+        with self.assertRaises(_MockSelectEternalWait):
             pty._copy(masters[0])
 
         # Test that the right data went to the right places.
-        rfds = select.select([read_from_stdout_fd, masters[1]], [], [], 0)[0]
-        self.assertEqual([read_from_stdout_fd, masters[1]], rfds)
-        self.assertEqual(os.read(read_from_stdout_fd, 20), b'from master')
+        rfds = select.select([self.read_from_stdout_fd, masters[1]], [], [], 0)[0]
+        self.assertEqual([self.read_from_stdout_fd, masters[1]], rfds)
+        self.assertEqual(os.read(self.read_from_stdout_fd, 20), b'from master')
         self.assertEqual(os.read(masters[1], 20), b'from stdin')
 
-    def test__copy_eof_on_all(self):
-        """Test the empty read EOF case on both master_fd and stdin."""
-        read_from_stdout_fd, mock_stdout_fd = self._pipe()
-        pty.STDOUT_FILENO = mock_stdout_fd
-        mock_stdin_fd, write_to_stdin_fd = self._pipe()
-        pty.STDIN_FILENO = mock_stdin_fd
+    def _copy_eof_close_slave_helper(self, close_stdin):
+        """Helper to test the empty read EOF case on master_fd and/or
+        stdin."""
         socketpair = self._socketpair()
         masters = [s.fileno() for s in socketpair]
 
+        # This side of the channel would usually be the slave_fd of the
+        # child.  We simulate that the child has exited and its side of
+        # the channel is destroyed.
         socketpair[1].close()
-        os.close(write_to_stdin_fd)
+        self.files.remove(socketpair[1])
 
-        # Expect two select calls, the last one will cause IndexError
-        pty.select = self._mock_select
+        # Optionally close fd or fill with dummy data in order to
+        # prevent blocking on one read call
+        if close_stdin:
+            os.close(self.write_to_stdin_fd)
+            self.fds.remove(self.write_to_stdin_fd)
+        else:
+            os.write(self.write_to_stdin_fd, b'from stdin')
+
+        # Expect exactly one select() call.  This call returns master_fd
+        # and STDIN.  Since the slave side of masters is closed, we
+        # expect the _copy loop to exit immediately.
         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)
+        self.select_rfds_results.append([pty.STDIN_FILENO, masters[0]])
+
+        # Run the _copy test, which returns nothing and cleanly exits
+        self.assertIsNone(pty._copy(masters[0]))
+
+        # We expect that everything is consumed
+        self.assertEqual(self.select_rfds_results, [])
+        self.assertEqual(self.select_rfds_lengths, [])
+
+    def test__copy_eof_on_all(self):
+        # Test the empty read EOF case on both master_fd and stdin.
+        self._copy_eof_close_slave_helper(close_stdin=True)
+
+    def test__copy_eof_on_master(self):
+        # Test the empty read EOF case on only master_fd.
+        self._copy_eof_close_slave_helper(close_stdin=False)
+
+    def test__copy_eof_on_stdin(self):
+        # Test the empty read EOF case on stdin.
+        masters = [s.fileno() for s in self._socketpair()]
+
+        # Fill with dummy data
+        os.write(masters[1], b'from master')
 
-        with self.assertRaises(IndexError):
+        os.close(self.write_to_stdin_fd)
+        self.fds.remove(self.write_to_stdin_fd)
+
+        # Expect two select() calls.  The first call returns master_fd
+        # and STDIN.
+        self.select_rfds_lengths.append(2)
+        self.select_rfds_results.append([pty.STDIN_FILENO, masters[0]])
+        # The second call causes _MockSelectEternalWait.  We expect that
+        # STDIN is removed from the waiters as it reached EOF.
+        self.select_rfds_lengths.append(1)
+        self.select_rfds_results.append(_MockSelectEternalWait)
+
+        with self.assertRaises(_MockSelectEternalWait):
             pty._copy(masters[0])
 
+        # We expect that everything is consumed
+        self.assertEqual(self.select_rfds_results, [])
+        self.assertEqual(self.select_rfds_lengths, [])
+
 
 def tearDownModule():
     reap_children()

Misc/ACKS

@@ -1787,3 +1787,5 @@ Jelle Zijlstra
 Gennadiy Zlobin
 Doug Zongker
 Peter Åstrand
+Chris Torek
+Cornelius Diekmann

Misc/NEWS.d/next/Library/2017-10-29.bpo-26228.piIl5E.rst

@@ -0,0 +1 @@
+pty.spawn no longer hangs on FreeBSD, OS X, and Solaris.