From ee3f8838d614322aa6ac550163185a4e8016ac07 Mon Sep 17 00:00:00 2001 From: diekmann Date: Sun, 29 Oct 2017 17:51:08 +0100 Subject: [PATCH 1/7] original patch by Chris Torek https://bugs.python.org/file41738/pty.patch --- Lib/pty.py | 16 ++++++++++++++-- Lib/test/test_pty.py | 5 ++--- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/Lib/pty.py b/Lib/pty.py index e841f12f3edb9b8..72fefebb278f60a 100644 --- a/Lib/pty.py +++ b/Lib/pty.py @@ -137,7 +137,7 @@ def _copy(master_fd, master_read=_read, stdin_read=_read): 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 +153,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. + traceback.print_exc() + finally: + os._exit(1) try: mode = tty.tcgetattr(STDIN_FILENO) tty.setraw(STDIN_FILENO) @@ -163,6 +171,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) diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py index 3b448569a2ffcb9..60aa73a1b0675fe 100644 --- a/Lib/test/test_pty.py +++ b/Lib/test/test_pty.py @@ -293,7 +293,7 @@ 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 + # Expect two select calls, then a normal return on master EOF pty.select = self._mock_select self.select_rfds_lengths.append(2) self.select_rfds_results.append([mock_stdin_fd, masters[0]]) @@ -301,8 +301,7 @@ def test__copy_eof_on_all(self): # both encountered an EOF before the second select call. self.select_rfds_lengths.append(0) - with self.assertRaises(IndexError): - pty._copy(masters[0]) + pty._copy(masters[0]) def tearDownModule(): From 0dcfb63e5378aeb367f0a65cdb77ddd3b5f2518a Mon Sep 17 00:00:00 2001 From: diekmann Date: Sun, 29 Oct 2017 18:01:29 +0100 Subject: [PATCH 2/7] Updating documentation for pty module * Linux -> Unix * More details about the behavior of spawn --- Doc/library/pty.rst | 20 ++++++++++++-------- Lib/pty.py | 7 ++++++- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/Doc/library/pty.rst b/Doc/library/pty.rst index 0ab766065d6e817..2e6653227718818 100644 --- a/Doc/library/pty.rst +++ b/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 @@ -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 diff --git a/Lib/pty.py b/Lib/pty.py index 72fefebb278f60a..d23f17046b856cb 100644 --- a/Lib/pty.py +++ b/Lib/pty.py @@ -1,7 +1,7 @@ """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. @@ -133,6 +133,11 @@ 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) From e773525bae95900b4706c26c3465ee39e4387db2 Mon Sep 17 00:00:00 2001 From: diekmann Date: Sun, 29 Oct 2017 18:27:10 +0100 Subject: [PATCH 3/7] Updated pty test suite --- Lib/test/test_pty.py | 179 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 144 insertions(+), 35 deletions(-) diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py index 60aa73a1b0675fe..caac4f4940396d3 100644 --- a/Lib/test/test_pty.py +++ b/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,65 +254,163 @@ 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. + raise 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): + self.assertGreater(pty.STDIN_FILENO, 2, "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, then a normal return on master EOF - 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, []) + + # Test that STDIN was not touched. This test simulated the + # scenario where the child process immediately closed its end of + # the pipe. This means, nothing should be copied. + rfds = select.select([self.read_from_stdout_fd, pty.STDIN_FILENO], [], [], 0)[0] + # data or EOF is still sitting unconsumed in STDIN + self.assertEqual(rfds, [pty.STDIN_FILENO]) + unconsumed = os.read(pty.STDIN_FILENO, 20) + if close_stdin: + self.assertFalse(unconsumed) #EOF + else: + self.assertEqual(unconsumed, b'from stdin') + + 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') + + 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]) - pty._copy(masters[0]) + # We expect that everything is consumed + self.assertEqual(self.select_rfds_results, []) + self.assertEqual(self.select_rfds_lengths, []) def tearDownModule(): From db5194c0fe3c6d03f1206348e2493f9958d01fc2 Mon Sep 17 00:00:00 2001 From: diekmann Date: Sun, 29 Oct 2017 18:31:03 +0100 Subject: [PATCH 4/7] Acks --- Misc/ACKS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Misc/ACKS b/Misc/ACKS index 54d8d62b633f706..8fc9eafdf861e76 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -1787,3 +1787,5 @@ Jelle Zijlstra Gennadiy Zlobin Doug Zongker Peter Åstrand +Chris Torek +Cornelius Diekmann From 8bbf30b8bb3495649c2cf6876a0ad3c2fb567bee Mon Sep 17 00:00:00 2001 From: diekmann Date: Sun, 29 Oct 2017 23:18:25 +0100 Subject: [PATCH 5/7] Fix missing import in pty.py patch (error path) Thanks to vadmium for catching this error twice. https://bugs.python.org/review/29070/diff/19699/Lib/pty.py#newcode170 --- Lib/pty.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Lib/pty.py b/Lib/pty.py index d23f17046b856cb..ae122b81ea0aa74 100644 --- a/Lib/pty.py +++ b/Lib/pty.py @@ -8,6 +8,7 @@ from select import select import os +import sys import tty __all__ = ["openpty","fork","spawn"] @@ -164,7 +165,7 @@ def spawn(argv, master_read=_read, stdin_read=_read): # 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. - traceback.print_exc() + sys.excepthook(*sys.exc_info()) finally: os._exit(1) try: From 538676ac4c50d1bcf5098ef7a351f282d2f228b8 Mon Sep 17 00:00:00 2001 From: diekmann Date: Sun, 29 Oct 2017 23:48:17 +0100 Subject: [PATCH 6/7] Adding Misc/NEWS.d entry --- Misc/NEWS.d/next/Library/2017-10-29.bpo-26228.piIl5E.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2017-10-29.bpo-26228.piIl5E.rst diff --git a/Misc/NEWS.d/next/Library/2017-10-29.bpo-26228.piIl5E.rst b/Misc/NEWS.d/next/Library/2017-10-29.bpo-26228.piIl5E.rst new file mode 100644 index 000000000000000..b998bdf7c72ded8 --- /dev/null +++ b/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. From 615fecdaa2bda49c9b35f60205717502ebdb29c4 Mon Sep 17 00:00:00 2001 From: diekmann Date: Sat, 2 Dec 2017 18:33:18 +0100 Subject: [PATCH 7/7] Incorporate vadmium's suggestions. --- Lib/test/test_pty.py | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/Lib/test/test_pty.py b/Lib/test/test_pty.py index caac4f4940396d3..64f593c521ec3c4 100644 --- a/Lib/test/test_pty.py +++ b/Lib/test/test_pty.py @@ -271,7 +271,7 @@ def _mock_select(self, rfds, wfds, xfds): # called with three empty lists as file descriptors to wait # on. Behavior of real select is platform-dependent and # likely infinite blocking on Linux. - raise self.fail("mock select on no waitables") + self.fail("mock select on no waitables") rfds_result = self.select_rfds_results.pop(0) if rfds_result is _MockSelectEternalWait: @@ -279,7 +279,9 @@ def _mock_select(self, rfds, wfds, xfds): return rfds_result, [], [] def test__mock_stdin_stdout(self): - self.assertGreater(pty.STDIN_FILENO, 2, "replaced by our mock") + """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. @@ -366,18 +368,6 @@ def _copy_eof_close_slave_helper(self, close_stdin): self.assertEqual(self.select_rfds_results, []) self.assertEqual(self.select_rfds_lengths, []) - # Test that STDIN was not touched. This test simulated the - # scenario where the child process immediately closed its end of - # the pipe. This means, nothing should be copied. - rfds = select.select([self.read_from_stdout_fd, pty.STDIN_FILENO], [], [], 0)[0] - # data or EOF is still sitting unconsumed in STDIN - self.assertEqual(rfds, [pty.STDIN_FILENO]) - unconsumed = os.read(pty.STDIN_FILENO, 20) - if close_stdin: - self.assertFalse(unconsumed) #EOF - else: - self.assertEqual(unconsumed, b'from stdin') - 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)