[Python-checkins] bpo-26228: Fix pty EOF handling (GH-12049)
ambv
webhook-mailer at python.org
Wed Aug 11 18:22:00 EDT 2021
https://github.com/python/cpython/commit/81ab8db235580317edcb0e559cd4c983f70883f5
commit: 81ab8db235580317edcb0e559cd4c983f70883f5
branch: main
author: Zephyr Shannon <geoffpshannon at gmail.com>
committer: ambv <lukasz at langa.pl>
date: 2021-08-12T00:21:46+02:00
summary:
bpo-26228: Fix pty EOF handling (GH-12049)
On non-Linux POSIX platforms, like FreeBSD or macOS,
the FD used to read a forked PTY may signal its exit not
by raising an error but by sending empty data to the read
syscall. This case wasn't handled, leading to hanging
`pty.spawn` calls.
Co-authored-by: Reilly Tucker Siemens <reilly at tuckersiemens.com>
Co-authored-by: Łukasz Langa <lukasz at langa.pl>
files:
A Misc/NEWS.d/next/Library/2019-02-26-09-31-59.bpo-26228.wyrHKc.rst
M Doc/library/pty.rst
M Lib/pty.py
M Lib/test/test_pty.py
M Misc/ACKS
diff --git a/Doc/library/pty.rst b/Doc/library/pty.rst
index 73d4f102fd4d8e..4b49d244038eb7 100644
--- 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
index a32432041fa1db..43e974fff1c908 100644
--- a/Lib/pty.py
+++ b/Lib/pty.py
@@ -11,7 +11,11 @@
import sys
import tty
-__all__ = ["openpty","fork","spawn"]
+# names imported directly for test mocking purposes
+from os import close, waitpid
+from tty import setraw, tcgetattr, tcsetattr
+
+__all__ = ["openpty", "fork", "spawn"]
STDIN_FILENO = 0
STDOUT_FILENO = 1
@@ -105,8 +109,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 +137,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 +165,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
index 7585c42bf01338..e2e9475b5d337d 100644
--- 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
@@ -123,12 +124,6 @@ def handle_sig(self, sig, frame):
@staticmethod
def handle_sighup(signum, frame):
- # bpo-38547: if the process is the session leader, os.close(master_fd)
- # of "master_fd, slave_name = pty.master_open()" raises SIGHUP
- # signal: just ignore the signal.
- #
- # NOTE: the above comment is from an older version of the test;
- # master_open() is not being used anymore.
pass
@expectedFailureIfStdinIsTTY
@@ -190,13 +185,6 @@ def test_openpty(self):
self.assertEqual(_get_term_winsz(slave_fd), new_stdin_winsz,
"openpty() failed to set slave window size")
- # Solaris requires reading the fd before anything is returned.
- # My guess is that since we open and close the slave fd
- # in master_open(), we need to read the EOF.
- #
- # NOTE: the above comment is from an older version of the test;
- # master_open() is not being used anymore.
-
# Ensure the fd is non-blocking in case there's nothing to read.
blocking = os.get_blocking(master_fd)
try:
@@ -324,22 +312,40 @@ def test_master_read(self):
self.assertEqual(data, b"")
+ def test_spawn_doesnt_hang(self):
+ pty.spawn([sys.executable, '-c', 'print("hi there")'])
+
class SmallPtyTests(unittest.TestCase):
"""These tests don't spawn children or hang."""
def setUp(self):
self.orig_stdin_fileno = pty.STDIN_FILENO
self.orig_stdout_fileno = pty.STDOUT_FILENO
+ self.orig_pty_close = pty.close
+ self.orig_pty__copy = pty._copy
+ 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.close = self.orig_pty_close
+ pty._copy = self.orig_pty__copy
+ 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 +373,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,7 +421,6 @@ 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]])
@@ -415,12 +428,34 @@ 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])
+ # 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
index f845c1c5a48982..7829feebc49b0c 100644
--- 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
new file mode 100644
index 00000000000000..43ff580bcfaf63
--- /dev/null
+++ 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.
\ No newline at end of file
More information about the Python-checkins
mailing list