From 638dd59a4e2c9f471803e3a3767666c9a3fce691 Mon Sep 17 00:00:00 2001 From: anwilli5 Date: Thu, 9 Oct 2014 19:40:20 -0400 Subject: [PATCH 1/5] - Adding more robust code to handle the case where the exec call within spawn fails. Now, spawn will raise an exception if this happens. - Adding a test to ensure this exception is raised when an invalid binary is run --- ptyprocess/ptyprocess.py | 54 +++++++++++++++++++++++---- tests/test_invalid_binary.py | 71 ++++++++++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 7 deletions(-) create mode 100755 tests/test_invalid_binary.py diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index 2f66e0c..c4f4b6e 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. + read_end, write_end = 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(read_end) + fcntl.fcntl(write_end, fcntl.F_SETFD, fcntl.FD_CLOEXEC) + + # Do not allow child to inherit open file descriptors from parent, + # with the exception of the write_end of the pipe max_fd = resource.getrlimit(resource.RLIMIT_NOFILE)[0] - os.closerange(3, max_fd) + os.closerange(3, write_end) + os.closerange(write_end+1, max_fd) if cwd is not None: os.chdir(cwd) @@ -231,10 +247,17 @@ 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. + os.write(write_end, str(err).encode('utf-8')) + os.close(write_end) + os._exit(os.EX_OSERR) # Parent inst = cls(pid, fd) @@ -245,7 +268,24 @@ 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(write_end) + data = os.read(read_end, 4096) + os.close(read_end) + + # [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(data) != 0: + exception = OSError(data.decode('utf-8')) + exception.errno = errno.ENOEXEC + 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..ba53863 --- /dev/null +++ b/tests/test_invalid_binary.py @@ -0,0 +1,71 @@ +#!/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 os +import stat +import tempfile +import errno + +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") + + test = os.open(fullpath, os.O_RDWR | os.O_CREAT) + os.write(test, os.urandom(1024)) # Contents shouldn't matter + os.close(test) + + # 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() + os.unlink(fullpath) + os.rmdir(dirpath) + raise AssertionError("OSError was not raised") + except OSError as err: + if errno.ENOEXEC == err.errno: + # This is what should happen + pass + else: + os.unlink(fullpath) + os.rmdir(dirpath) + raise AssertionError("Wrong OSError raised") + + os.unlink(fullpath) + os.rmdir(dirpath) + +if __name__ == '__main__': + unittest.main() + +suite = unittest.makeSuite(InvalidBinaryChars,'test') + From 94fd3da1e6dfea428dd38af14bee48ccee96ab8d Mon Sep 17 00:00:00 2001 From: anwilli5 Date: Fri, 10 Oct 2014 00:20:26 -0400 Subject: [PATCH 2/5] - Addressing all of the points raised by @takluyver in https://github.com/pexpect/ptyprocess/pull/1 with the exception of the generic exec OSError handling - waiting on thoughts on how best to implement this. --- ptyprocess/ptyprocess.py | 27 ++++++++++++++------------- tests/test_invalid_binary.py | 19 ++++++++----------- 2 files changed, 22 insertions(+), 24 deletions(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index c4f4b6e..ba34c6f 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -202,7 +202,7 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): # parent process # [issue #119] 1. Before forking, open a pipe in the parent process. - read_end, write_end = os.pipe() + exec_err_pipe_read, exec_err_pipe_write = os.pipe() if use_native_pty_fork: pid, fd = pty.fork() @@ -232,14 +232,14 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): # [issue #119] 3. The child closes the reading end and sets the # close-on-exec flag for the writing end. - os.close(read_end) - fcntl.fcntl(write_end, fcntl.F_SETFD, fcntl.FD_CLOEXEC) + 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 write_end of the pipe + # with the exception of the exec_err_pipe_write of the pipe max_fd = resource.getrlimit(resource.RLIMIT_NOFILE)[0] - os.closerange(3, write_end) - os.closerange(write_end+1, 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) @@ -255,8 +255,9 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): 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. - os.write(write_end, str(err).encode('utf-8')) - os.close(write_end) + errbytes = str(err).encode('utf-8') if PY3 else str(err) + os.write(exec_err_pipe_write, errbytes) + os.close(exec_err_pipe_write) os._exit(os.EX_OSERR) # Parent @@ -271,9 +272,9 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): # [issue #119] 2. After forking, the parent closes the writing end # of the pipe and reads from the reading end. - os.close(write_end) - data = os.read(read_end, 4096) - os.close(read_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 @@ -281,8 +282,8 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): # failed, the parent reads the error code and can proceed # accordingly. Either way, the parent blocks until the child calls # exec. - if len(data) != 0: - exception = OSError(data.decode('utf-8')) + if len(exec_err_data) != 0: + exception = OSError(exec_err_data.decode('utf-8', 'replace')) exception.errno = errno.ENOEXEC raise exception diff --git a/tests/test_invalid_binary.py b/tests/test_invalid_binary.py index ba53863..ba973d5 100755 --- a/tests/test_invalid_binary.py +++ b/tests/test_invalid_binary.py @@ -36,9 +36,9 @@ def test_invalid_binary(self): dirpath = tempfile.mkdtemp() fullpath = os.path.join(dirpath, "test") - test = os.open(fullpath, os.O_RDWR | os.O_CREAT) - os.write(test, os.urandom(1024)) # Contents shouldn't matter - os.close(test) + with open(fullpath, 'wb') as f: + f.write(os.urandom(1024)) # Contents shouldn't matter + f.close # Make it executable st = os.stat(fullpath) @@ -49,20 +49,17 @@ def test_invalid_binary(self): child = PtyProcess.spawn([fullpath]) # If we get here then an OSError was not raised child.close() - os.unlink(fullpath) - os.rmdir(dirpath) raise AssertionError("OSError was not raised") except OSError as err: if errno.ENOEXEC == err.errno: # This is what should happen pass else: - os.unlink(fullpath) - os.rmdir(dirpath) - raise AssertionError("Wrong OSError raised") - - os.unlink(fullpath) - os.rmdir(dirpath) + # Re-raise the original error to fail the test + raise + finally: + os.unlink(fullpath) + os.rmdir(dirpath) if __name__ == '__main__': unittest.main() From df851dde016b7864ed27dca736a5341c9ca96edc Mon Sep 17 00:00:00 2001 From: anwilli5 Date: Sat, 11 Oct 2014 11:51:15 -0400 Subject: [PATCH 3/5] - In the case where exec fails, the errno returned in the exception is now the one returned from the child process exception --- ptyprocess/ptyprocess.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/ptyprocess/ptyprocess.py b/ptyprocess/ptyprocess.py index ba34c6f..fe9d1f2 100644 --- a/ptyprocess/ptyprocess.py +++ b/ptyprocess/ptyprocess.py @@ -256,7 +256,7 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): # [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, errbytes) + os.write(exec_err_pipe_write, struct.pack('i', err.errno) + errbytes) os.close(exec_err_pipe_write) os._exit(os.EX_OSERR) @@ -283,8 +283,14 @@ def spawn(cls, argv, cwd=None, env=None, echo=True, before_exec=()): # accordingly. Either way, the parent blocks until the child calls # exec. if len(exec_err_data) != 0: - exception = OSError(exec_err_data.decode('utf-8', 'replace')) - exception.errno = errno.ENOEXEC + + 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: From fbb5f59852c2985d872cbb2863aa0e1231b3ff04 Mon Sep 17 00:00:00 2001 From: anwilli5 Date: Mon, 13 Oct 2014 13:17:14 -0400 Subject: [PATCH 4/5] - Minor improvements to the exec failure test case --- tests/test_invalid_binary.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/test_invalid_binary.py b/tests/test_invalid_binary.py index ba973d5..0755758 100755 --- a/tests/test_invalid_binary.py +++ b/tests/test_invalid_binary.py @@ -21,10 +21,13 @@ import time import unittest from ptyprocess import PtyProcess, PtyProcessUnicode +import errno import os import stat +import sys import tempfile -import errno + +PY3 = sys.version_info[0] >= 3 class InvalidBinaryChars(unittest.TestCase): @@ -37,8 +40,15 @@ def test_invalid_binary(self): fullpath = os.path.join(dirpath, "test") with open(fullpath, 'wb') as f: - f.write(os.urandom(1024)) # Contents shouldn't matter - f.close + # 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 = str('\x00\x00').encode('utf-8') if PY3 else str('\x00\x00') + file_data = file_start + os.urandom(1022) + f.write(file_data) + f.flush() # Make it executable st = os.stat(fullpath) From b5d54f55174ade3b4c3dbcb630ec0cbd4c73db9c Mon Sep 17 00:00:00 2001 From: anwilli5 Date: Mon, 13 Oct 2014 14:32:44 -0400 Subject: [PATCH 5/5] - Simplifying some of the exec failure test code based on suggestions from @takluyver --- tests/test_invalid_binary.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/test_invalid_binary.py b/tests/test_invalid_binary.py index 0755758..cf28098 100755 --- a/tests/test_invalid_binary.py +++ b/tests/test_invalid_binary.py @@ -24,11 +24,8 @@ import errno import os import stat -import sys import tempfile -PY3 = sys.version_info[0] >= 3 - class InvalidBinaryChars(unittest.TestCase): def test_invalid_binary(self): @@ -45,10 +42,9 @@ def test_invalid_binary(self): # - Not 0x7FEF (ELF) # - Not 0410 or 0413 (a.out) # - Not 0x2321 (script) - file_start = str('\x00\x00').encode('utf-8') if PY3 else str('\x00\x00') + file_start = b'\x00\x00' file_data = file_start + os.urandom(1022) f.write(file_data) - f.flush() # Make it executable st = os.stat(fullpath)