From 8b44a5d31f69e0d0b6493a56e713be6d5ccdbcd7 Mon Sep 17 00:00:00 2001 From: Andrew Cagney Date: Thu, 11 Apr 2019 16:22:45 -0400 Subject: [PATCH 1/5] Use and prefer os.posix_spawn() when available Python 3.8 has added os.posix_spawn(), this changes ptyprocess to use it when available. Since os.posix_spawn() completey bypasses os.fork(), pty.fork(), it avoids problems with logging locks and code needing to be thread-safe. --- ptyprocess/ptyprocess.py | 245 ++++++++++++++++++++++++--------------- 1 file changed, 150 insertions(+), 95 deletions(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index 78d19fd..4c02061 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -10,6 +10,7 @@ import sys import termios import time +import threading try: import builtins # Python 3 @@ -17,12 +18,14 @@ import __builtin__ as builtins # Python 2 # Constants -from pty import (STDIN_FILENO, CHILD) +from pty import (STDIN_FILENO, CHILD, STDOUT_FILENO, STDERR_FILENO) from .util import which, PtyProcessError _platform = sys.platform.lower() +_posix_spawn_lock = threading.Lock() + # Solaris uses internal __fork_pty(). All others use pty.fork(). _is_solaris = ( _platform.startswith('solaris') or @@ -217,91 +220,169 @@ def spawn( command = command_with_path argv[0] = command - # [issue #119] To prevent the case where exec fails and the user is - # stuck interacting with a python child process instead of whatever - # was expected, we implement the solution from - # http://stackoverflow.com/a/3703179 to pass the exception to the - # parent process + if hasattr(os, 'posix_spawn'): + # Without the lock parallel posix_spawns() would + # unwittingly inherit each other's PTY(FD)/TTY pair. + # True? + with _posix_spawn_lock: + # Issue 36603: Use os.openpty() (and try to avoid the + # whole pty module) as that guarentees inheritable (if + # it ever fails then just file a bug against + # os.openpty() + fd, tty = os.openpty() + # Try to set window size on TTY per below; but is this + # needed? + try: + _setwinsize(tty, *dimensions) + except IOError as err: + if err.args[0] not in (errno.EINVAL, errno.ENOTTY): + raise + # Try to disable echo if spawn argument echo was unset per + # below; but does this work? + if not echo: + try: + _setecho(tty, False) + except (IOError, termios.error) as err: + if err.args[0] not in (errno.EINVAL, errno.ENOTTY): + raise + # Create the child: convert the tty into STDIO; use + # the default ENV if needed; and try to make the child + # the session head using SETSID. Assume that all + # files have inheritable (close-on-exec) correctly + # set. + file_actions=[ + (os.POSIX_SPAWN_DUP2, tty, STDIN_FILENO), + (os.POSIX_SPAWN_DUP2, tty, STDOUT_FILENO), + (os.POSIX_SPAWN_DUP2, tty, STDERR_FILENO), + (os.POSIX_SPAWN_CLOSE, tty), + (os.POSIX_SPAWN_CLOSE, fd), + ] + spawn_env = env or os.environ + pid = os.posix_spawn(command, argv, spawn_env, + file_actions=file_actions, + setsid=True) + # Child started. Now close tty and stop PTY(FD) being + # inherited + os.close(tty) + os.set_inheritable(fd, False) + else: - # [issue #119] 1. Before forking, open a pipe in the parent process. - exec_err_pipe_read, exec_err_pipe_write = os.pipe() + # [issue #119] To prevent the case where exec fails and + # the user is stuck interacting with a python child + # process instead of whatever was expected, we implement + # the solution from http://stackoverflow.com/a/3703179 to + # pass the exception to the parent process - if use_native_pty_fork: - pid, fd = pty.fork() - else: - # Use internal fork_pty, for Solaris - pid, fd = _fork_pty.fork_pty() + # [issue #119] 1. Before forking, open a pipe in the + # parent process. + exec_err_pipe_read, exec_err_pipe_write = os.pipe() - # Some platforms must call setwinsize() and setecho() from the - # child process, and others from the master process. We do both, - # allowing IOError for either. + # XXX: only use fork_pty()? + if use_native_pty_fork: + pid, fd = pty.fork() + else: + # Use internal fork_pty, for Solaris + pid, fd = _fork_pty.fork_pty() - if pid == CHILD: - # set window size - try: - _setwinsize(STDIN_FILENO, *dimensions) - except IOError as err: - if err.args[0] not in (errno.EINVAL, errno.ENOTTY): - raise + # Some platforms must call setwinsize() and setecho() from + # the child process, and others from the master + # process. We do both, allowing IOError for either. - # disable echo if spawn argument echo was unset - if not echo: + if pid == CHILD: + # set window size try: - _setecho(STDIN_FILENO, False) - except (IOError, termios.error) as err: + _setwinsize(STDIN_FILENO, *dimensions) + except IOError as err: if err.args[0] not in (errno.EINVAL, errno.ENOTTY): raise - # [issue #119] 3. The child closes the reading end and sets the - # close-on-exec flag for the writing end. - os.close(exec_err_pipe_read) - fcntl.fcntl(exec_err_pipe_write, fcntl.F_SETFD, fcntl.FD_CLOEXEC) - - # Do not allow child to inherit open file descriptors from parent, - # with the exception of the exec_err_pipe_write of the pipe - # and pass_fds. - # Impose ceiling on max_fd: AIX bugfix for users with unlimited - # nofiles where resource.RLIMIT_NOFILE is 2^63-1 and os.closerange() - # occasionally raises out of range error - max_fd = min(1048576, resource.getrlimit(resource.RLIMIT_NOFILE)[0]) - spass_fds = sorted(set(pass_fds) | {exec_err_pipe_write}) - for pair in zip([2] + spass_fds, spass_fds + [max_fd]): - os.closerange(pair[0]+1, pair[1]) - - if cwd is not None: - os.chdir(cwd) - - if preexec_fn is not None: + # disable echo if spawn argument echo was unset + if not echo: + try: + _setecho(STDIN_FILENO, False) + except (IOError, termios.error) as err: + if err.args[0] not in (errno.EINVAL, errno.ENOTTY): + raise + + # [issue #119] 3. The child closes the reading end and + # sets the close-on-exec flag for the writing end. + os.close(exec_err_pipe_read) + fcntl.fcntl(exec_err_pipe_write, fcntl.F_SETFD, fcntl.FD_CLOEXEC) + + # Do not allow child to inherit open file descriptors from parent, + # with the exception of the exec_err_pipe_write of the pipe + # and pass_fds. + + # Impose ceiling on max_fd: AIX bugfix for users with + # unlimited nofiles where resource.RLIMIT_NOFILE is + # 2^63-1 and os.closerange() occasionally raises out + # of range error + max_fd = min(1048576, resource.getrlimit(resource.RLIMIT_NOFILE)[0]) + spass_fds = sorted(set(pass_fds) | {exec_err_pipe_write}) + for pair in zip([2] + spass_fds, spass_fds + [max_fd]): + os.closerange(pair[0]+1, pair[1]) + + if cwd is not None: + os.chdir(cwd) + + if preexec_fn is not None: + try: + preexec_fn() + except Exception as e: + ename = type(e).__name__ + tosend = '{}:0:{}'.format(ename, str(e)) + if PY3: + tosend = tosend.encode('utf-8') + + os.write(exec_err_pipe_write, tosend) + os.close(exec_err_pipe_write) + os._exit(1) + try: - preexec_fn() - except Exception as e: - ename = type(e).__name__ - tosend = '{}:0:{}'.format(ename, str(e)) + if env is None: + os.execv(command, argv) + else: + os.execvpe(command, argv, env) + except OSError as err: + # [issue #119] 5. If exec fails, the child writes + # the error code back to the parent using the + # pipe, then exits. + tosend = 'OSError:{}:{}'.format(err.errno, str(err)) if PY3: tosend = tosend.encode('utf-8') + os.write(exec_err_pipe_write, tosend) + os.close(exec_err_pipe_write) + os._exit(os.EX_OSERR) + + # [issue #119] 2. After forking, the parent closes the + # writing end of the pipe and reads from the reading end. + os.close(exec_err_pipe_write) + exec_err_data = os.read(exec_err_pipe_read, 4096) + os.close(exec_err_pipe_read) - os.write(exec_err_pipe_write, tosend) - os.close(exec_err_pipe_write) - os._exit(1) - - try: - if env is None: - os.execv(command, argv) + # [issue #119] 6. The parent reads eof (a zero-length + # read) if the child successfully performed exec, since + # close-on-exec made successful exec close the writing end + # of the pipe. Or, if exec failed, the parent reads the + # error code and can proceed accordingly. Either way, the + # parent blocks until the child calls exec. + if len(exec_err_data) != 0: + try: + errclass, errno_s, errmsg = exec_err_data.split(b':', 2) + exctype = getattr(builtins, errclass.decode('ascii'), Exception) + + exception = exctype(errmsg.decode('utf-8', 'replace')) + if exctype is OSError: + exception.errno = int(errno_s) + except: + raise Exception('Subprocess failed, got bad error data: %r' + % exec_err_data) else: - os.execvpe(command, argv, env) - except OSError as err: - # [issue #119] 5. If exec fails, the child writes the error - # code back to the parent using the pipe, then exits. - tosend = 'OSError:{}:{}'.format(err.errno, str(err)) - if PY3: - tosend = tosend.encode('utf-8') - os.write(exec_err_pipe_write, tosend) - os.close(exec_err_pipe_write) - os._exit(os.EX_OSERR) + raise exception # Parent inst = cls(pid, fd) - + # Set some informational attributes inst.argv = argv if env is not None: @@ -309,32 +390,6 @@ def spawn( if cwd is not None: inst.launch_dir = cwd - # [issue #119] 2. After forking, the parent closes the writing end - # of the pipe and reads from the reading end. - os.close(exec_err_pipe_write) - exec_err_data = os.read(exec_err_pipe_read, 4096) - os.close(exec_err_pipe_read) - - # [issue #119] 6. The parent reads eof (a zero-length read) if the - # child successfully performed exec, since close-on-exec made - # successful exec close the writing end of the pipe. Or, if exec - # failed, the parent reads the error code and can proceed - # accordingly. Either way, the parent blocks until the child calls - # exec. - if len(exec_err_data) != 0: - try: - errclass, errno_s, errmsg = exec_err_data.split(b':', 2) - exctype = getattr(builtins, errclass.decode('ascii'), Exception) - - exception = exctype(errmsg.decode('utf-8', 'replace')) - if exctype is OSError: - exception.errno = int(errno_s) - except: - raise Exception('Subprocess failed, got bad error data: %r' - % exec_err_data) - else: - raise exception - try: inst.setwinsize(*dimensions) except IOError as err: From 892d8efb414c1a06e53ed476786e351168f67308 Mon Sep 17 00:00:00 2001 From: Andrew Cagney Date: Fri, 12 Apr 2019 13:04:40 -0400 Subject: [PATCH 2/5] Reduce scope of lock to just os.openpty() and set O_CLOEXEC (non-inheritable is code for O_CLOEXEC) --- ptyprocess/ptyprocess.py | 86 ++++++++++++++++++++++------------------ 1 file changed, 47 insertions(+), 39 deletions(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index 4c02061..591d605 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -221,50 +221,58 @@ def spawn( argv[0] = command if hasattr(os, 'posix_spawn'): - # Without the lock parallel posix_spawns() would - # unwittingly inherit each other's PTY(FD)/TTY pair. - # True? + print("using os.spawn_lock()") with _posix_spawn_lock: - # Issue 36603: Use os.openpty() (and try to avoid the - # whole pty module) as that guarentees inheritable (if - # it ever fails then just file a bug against - # os.openpty() + # Try to ensure that the tty/pty have O_CLOEXEC set + # (aka non-inheritable) so that a parallel call to + # this code won't end up with an open PTY/TTY. + # Unfortunately os.openpty() (openpty(3)) never sets + # O_CLOEXEC and pty.open() (Issue 36603) only + # sometimes sets O_CLOEXEC. + # + # Is this a bug? Arguably yes. However it isn't easy + # to fix - unless the native openpty(3) atomically + # opens the pty/tty with O_CLOEXEC there's always + # going to be a race. This lock mitigates the case + # where fork()/exec() is using this path (or other + # code paths such as subprocess that brute force a + # close of all FDs). fd, tty = os.openpty() - # Try to set window size on TTY per below; but is this - # needed? + os.set_inheritable(fd, False) + os.set_inheritable(tty, False) + # Try to set window size on TTY per below; but is this + # needed? + try: + _setwinsize(tty, *dimensions) + except IOError as err: + if err.args[0] not in (errno.EINVAL, errno.ENOTTY): + raise + # Try to disable echo if spawn argument echo was unset per + # below; but does this work? + if not echo: try: - _setwinsize(tty, *dimensions) - except IOError as err: + _setecho(tty, False) + except (IOError, termios.error) as err: if err.args[0] not in (errno.EINVAL, errno.ENOTTY): raise - # Try to disable echo if spawn argument echo was unset per - # below; but does this work? - if not echo: - try: - _setecho(tty, False) - except (IOError, termios.error) as err: - if err.args[0] not in (errno.EINVAL, errno.ENOTTY): - raise - # Create the child: convert the tty into STDIO; use - # the default ENV if needed; and try to make the child - # the session head using SETSID. Assume that all - # files have inheritable (close-on-exec) correctly - # set. - file_actions=[ - (os.POSIX_SPAWN_DUP2, tty, STDIN_FILENO), - (os.POSIX_SPAWN_DUP2, tty, STDOUT_FILENO), - (os.POSIX_SPAWN_DUP2, tty, STDERR_FILENO), - (os.POSIX_SPAWN_CLOSE, tty), - (os.POSIX_SPAWN_CLOSE, fd), - ] - spawn_env = env or os.environ - pid = os.posix_spawn(command, argv, spawn_env, - file_actions=file_actions, - setsid=True) - # Child started. Now close tty and stop PTY(FD) being - # inherited - os.close(tty) - os.set_inheritable(fd, False) + # Create the child: convert the tty into STDIO; use the + # default ENV if needed; and try to make the child the + # session head using SETSID. Assume that all files have + # inheritable (close-on-exec) correctly set. + file_actions=[ + (os.POSIX_SPAWN_DUP2, tty, STDIN_FILENO), + (os.POSIX_SPAWN_DUP2, tty, STDOUT_FILENO), + (os.POSIX_SPAWN_DUP2, tty, STDERR_FILENO), + # not needed? + (os.POSIX_SPAWN_CLOSE, tty), + (os.POSIX_SPAWN_CLOSE, fd), + ] + spawn_env = env or os.environ + pid = os.posix_spawn(command, argv, spawn_env, + file_actions=file_actions, + setsid=True) + # Child started; close the child's tty. + os.close(tty) else: # [issue #119] To prevent the case where exec fails and From a6501d60bf5436e8fc3a7f78ea90d20f811b183f Mon Sep 17 00:00:00 2001 From: Andrew Cagney Date: Fri, 12 Apr 2019 13:33:44 -0400 Subject: [PATCH 3/5] Use setpgroup=0 to put the child in its own process group. Drop setsid=True which is less portable but more useful. --- ptyprocess/ptyprocess.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index 591d605..8797ee9 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -221,7 +221,6 @@ def spawn( argv[0] = command if hasattr(os, 'posix_spawn'): - print("using os.spawn_lock()") with _posix_spawn_lock: # Try to ensure that the tty/pty have O_CLOEXEC set # (aka non-inheritable) so that a parallel call to @@ -255,10 +254,19 @@ def spawn( except (IOError, termios.error) as err: if err.args[0] not in (errno.EINVAL, errno.ENOTTY): raise - # Create the child: convert the tty into STDIO; use the - # default ENV if needed; and try to make the child the - # session head using SETSID. Assume that all files have - # inheritable (close-on-exec) correctly set. + # Create the child: + # + # - convert the tty into STDIN / STDOUT / STDERR + # + # - always specify ENV (use the default if needed) + # + # - put the child into its own process group using + # setpgroup=0; while setsid=True seems to be better + # there's no way to determine if it is available + # (bpo-36619). + # + # - assume that all files have inheritable (close-on-exec) + # correctly set. file_actions=[ (os.POSIX_SPAWN_DUP2, tty, STDIN_FILENO), (os.POSIX_SPAWN_DUP2, tty, STDOUT_FILENO), @@ -270,7 +278,7 @@ def spawn( spawn_env = env or os.environ pid = os.posix_spawn(command, argv, spawn_env, file_actions=file_actions, - setsid=True) + setpgroup=0) # Child started; close the child's tty. os.close(tty) else: From 9d0c278b7c8c45d3c22ade539c7ec37acfa87aa0 Mon Sep 17 00:00:00 2001 From: Andrew Cagney Date: Fri, 12 Apr 2019 16:24:24 -0400 Subject: [PATCH 4/5] Require POSIX_SPAWN_SETSID extension. --- ptyprocess/ptyprocess.py | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index 8797ee9..ee1c8fb 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -24,7 +24,20 @@ _platform = sys.platform.lower() -_posix_spawn_lock = threading.Lock() +# Is posix_spawn() robust? Require both os.posix_spawn() and the +# POSIX_SPAWN_SETSID extension (the latter, via setsid(), +# disassociates the child from the parent's session creating a new +# session and group and should be called when the system has things +# like job control) (there doesn't seem to be a runtime test for the +# latter, see bpo-36619). On systems that don't have job control, +# POSIX_SPAWN_SETPGROUP is probably sufficient but detecting that case +# is even harder. +_have_posix_spawn = ( + hasattr(os, 'posix_spawn') and + _platform.startswith("linux")) +if _have_posix_spawn: + print("using posix_spawn") + _posix_spawn_lock = threading.Lock() # Solaris uses internal __fork_pty(). All others use pty.fork(). _is_solaris = ( @@ -220,7 +233,7 @@ def spawn( command = command_with_path argv[0] = command - if hasattr(os, 'posix_spawn'): + if _have_posix_spawn: with _posix_spawn_lock: # Try to ensure that the tty/pty have O_CLOEXEC set # (aka non-inheritable) so that a parallel call to @@ -260,10 +273,9 @@ def spawn( # # - always specify ENV (use the default if needed) # - # - put the child into its own process group using - # setpgroup=0; while setsid=True seems to be better - # there's no way to determine if it is available - # (bpo-36619). + # - use setsid=True to create a new session (and + # disassociate the child from the parent's session / + # terminal) and make the child the process group leader # # - assume that all files have inheritable (close-on-exec) # correctly set. @@ -278,7 +290,7 @@ def spawn( spawn_env = env or os.environ pid = os.posix_spawn(command, argv, spawn_env, file_actions=file_actions, - setpgroup=0) + setsid=0) # Child started; close the child's tty. os.close(tty) else: From fce5af09106fa58b0fb349b7195bb97aaf80bc8c Mon Sep 17 00:00:00 2001 From: Andrew Cagney Date: Fri, 12 Apr 2019 16:29:58 -0400 Subject: [PATCH 5/5] Delete stray print() lines. --- ptyprocess/ptyprocess.py | 1 - 1 file changed, 1 deletion(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index ee1c8fb..0ece06c 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -36,7 +36,6 @@ hasattr(os, 'posix_spawn') and _platform.startswith("linux")) if _have_posix_spawn: - print("using posix_spawn") _posix_spawn_lock = threading.Lock() # Solaris uses internal __fork_pty(). All others use pty.fork().