diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index 2f66e0c..fe9d1f2 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -195,6 +195,15 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): 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 + + # [issue #119] 1. Before forking, open a pipe in the parent process. + exec_err_pipe_read, exec_err_pipe_write = os.pipe() + if use_native_pty_fork: pid, fd = pty.fork() else: @@ -221,9 +230,16 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): if err.args[0] not in (errno.EINVAL, errno.ENOTTY): raise - # Do not allow child to inherit open file descriptors from parent. + # [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 max_fd = resource.getrlimit(resource.RLIMIT_NOFILE)[0] - os.closerange(3, max_fd) + os.closerange(3, exec_err_pipe_write) + os.closerange(exec_err_pipe_write+1, max_fd) if cwd is not None: os.chdir(cwd) @@ -231,10 +247,18 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): for func in before_exec: func() - if env is None: - os.execv(command, argv) - else: - os.execvpe(command, argv, env) + try: + 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. + errbytes = str(err).encode('utf-8') if PY3 else str(err) + os.write(exec_err_pipe_write, struct.pack('i', err.errno) + errbytes) + os.close(exec_err_pipe_write) + os._exit(os.EX_OSERR) # Parent inst = cls(pid, fd) @@ -245,7 +269,30 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): inst.env = env 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: + + exec_err_fmt = 'i' + exec_err_fmt_size = struct.calcsize(exec_err_fmt) + exec_err_errno = struct.unpack(exec_err_fmt, exec_err_data[:exec_err_fmt_size])[0] + exec_err_msg = exec_err_data[exec_err_fmt_size:] + + exception = OSError(exec_err_msg.decode('utf-8', 'replace')) + exception.errno = exec_err_errno + raise exception + try: inst.setwinsize(24, 80) except IOError as err: diff --git a/tests/test_invalid_binary.py b/tests/test_invalid_binary.py new file mode 100755 index 0000000..cf28098 --- /dev/null +++ b/tests/test_invalid_binary.py @@ -0,0 +1,74 @@ +#!/usr/bin/env python +''' +PEXPECT LICENSE + + This license is approved by the OSI and FSF as GPL-compatible. + http://opensource.org/licenses/isc-license.txt + + Copyright (c) 2012, Noah Spurrier + PERMISSION TO USE, COPY, MODIFY, AND/OR DISTRIBUTE THIS SOFTWARE FOR ANY + PURPOSE WITH OR WITHOUT FEE IS HEREBY GRANTED, PROVIDED THAT THE ABOVE + COPYRIGHT NOTICE AND THIS PERMISSION NOTICE APPEAR IN ALL COPIES. + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + +''' +import time +import unittest +from ptyprocess import PtyProcess, PtyProcessUnicode +import errno +import os +import stat +import tempfile + +class InvalidBinaryChars(unittest.TestCase): + + def test_invalid_binary(self): + '''This tests that we correctly handle the case where we attempt to + spawn a child process but the exec call fails''' + + # Create a file that should fail the exec call + dirpath = tempfile.mkdtemp() + fullpath = os.path.join(dirpath, "test") + + with open(fullpath, 'wb') as f: + # Add some constant so it will never be executable + # - Not 0x54AD (Windows PE) + # - Not 0x7FEF (ELF) + # - Not 0410 or 0413 (a.out) + # - Not 0x2321 (script) + file_start = b'\x00\x00' + file_data = file_start + os.urandom(1022) + f.write(file_data) + + # Make it executable + st = os.stat(fullpath) + os.chmod(fullpath, st.st_mode | stat.S_IEXEC) + + # TODO Verify this does what is intended on Windows + try: + child = PtyProcess.spawn([fullpath]) + # If we get here then an OSError was not raised + child.close() + raise AssertionError("OSError was not raised") + except OSError as err: + if errno.ENOEXEC == err.errno: + # This is what should happen + pass + else: + # Re-raise the original error to fail the test + raise + finally: + os.unlink(fullpath) + os.rmdir(dirpath) + +if __name__ == '__main__': + unittest.main() + +suite = unittest.makeSuite(InvalidBinaryChars,'test') +