diff options
| -rw-r--r-- | Lib/subprocess.py | 9 | ||||
| -rw-r--r-- | Lib/test/test_subprocess.py | 47 | ||||
| -rw-r--r-- | Misc/NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst | 1 |
3 files changed, 54 insertions, 3 deletions
diff --git a/Lib/subprocess.py b/Lib/subprocess.py index bafb501..25e5698 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1313,15 +1313,18 @@ class Popen(object): try: exception_name, hex_errno, err_msg = ( errpipe_data.split(b':', 2)) + # The encoding here should match the encoding + # written in by the subprocess implementations + # like _posixsubprocess + err_msg = err_msg.decode() except ValueError: exception_name = b'SubprocessError' hex_errno = b'0' - err_msg = (b'Bad exception data from child: ' + - repr(errpipe_data)) + err_msg = 'Bad exception data from child: {!r}'.format( + bytes(errpipe_data)) child_exception_type = getattr( builtins, exception_name.decode('ascii'), SubprocessError) - err_msg = err_msg.decode(errors="surrogatepass") if issubclass(child_exception_type, OSError) and hex_errno: errno_num = int(hex_errno, 16) child_exec_never_called = (err_msg == "noexec") diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index a0d3dcd..b7079b1 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1542,6 +1542,53 @@ class POSIXProcessTestCase(BaseTestCase): else: self.fail("Expected OSError: %s" % desired_exception) + # We mock the __del__ method for Popen in the next two tests + # because it does cleanup based on the pid returned by fork_exec + # along with issuing a resource warning if it still exists. Since + # we don't actually spawn a process in these tests we can forego + # the destructor. An alternative would be to set _child_created to + # False before the destructor is called but there is no easy way + # to do that + class PopenNoDestructor(subprocess.Popen): + def __del__(self): + pass + + @mock.patch("subprocess._posixsubprocess.fork_exec") + def test_exception_errpipe_normal(self, fork_exec): + """Test error passing done through errpipe_write in the good case""" + def proper_error(*args): + errpipe_write = args[13] + # Write the hex for the error code EISDIR: 'is a directory' + err_code = '{:x}'.format(errno.EISDIR).encode() + os.write(errpipe_write, b"OSError:" + err_code + b":") + return 0 + + fork_exec.side_effect = proper_error + + with self.assertRaises(IsADirectoryError): + self.PopenNoDestructor(["non_existent_command"]) + + @mock.patch("subprocess._posixsubprocess.fork_exec") + def test_exception_errpipe_bad_data(self, fork_exec): + """Test error passing done through errpipe_write where its not + in the expected format""" + error_data = b"\xFF\x00\xDE\xAD" + def bad_error(*args): + errpipe_write = args[13] + # Anything can be in the pipe, no assumptions should + # be made about its encoding, so we'll write some + # arbitrary hex bytes to test it out + os.write(errpipe_write, error_data) + return 0 + + fork_exec.side_effect = bad_error + + with self.assertRaises(subprocess.SubprocessError) as e: + self.PopenNoDestructor(["non_existent_command"]) + + self.assertIn(repr(error_data), str(e.exception)) + + def test_restore_signals(self): # Code coverage for both values of restore_signals to make sure it # at least does not blow up. diff --git a/Misc/NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst b/Misc/NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst new file mode 100644 index 0000000..df018e0 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst @@ -0,0 +1 @@ +Fix string concatenation bug in rare error path in the subprocess module |