bpo-26228: Fix pty EOF handling (GH-12049) (GH-27732)
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@tuckersiemens.com>
Co-authored-by: Łukasz Langa <lukasz@langa.pl>
(cherry picked from commit 81ab8db235580317edcb0e559cd4c983f70883f5)
Co-authored-by: Zephyr Shannon <geoffpshannon@gmail.com>
diff --git a/Lib/pty.py b/Lib/pty.py
index a324320..43e974f 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 7585c42..e2e9475 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()