bpo-31178: Avoid concatenating bytes with str in subprocess error (#3066)
Avoid concatenating bytes with str in the typically rare subprocess error path (exec failed). Includes a mock based unittest to exercise the codepath.
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 @@
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 @@
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