[Python-checkins] [3.6] bpo-31178: Avoid concatenating bytes with str in subprocess error (GH-3066) (#3388)

[Gregory P. Smith]

https://github.com/python/cpython/commit/3bad1650a03fdc8cfdd4cce154e1b2c07e3e4fa0
commit: 3bad1650a03fdc8cfdd4cce154e1b2c07e3e4fa0
branch: 3.6
author: Gregory P. Smith <greg at krypto.org>
committer: GitHub <noreply at github.com>
date: 2017-09-06T13:34:17-07:00
summary:

[3.6] bpo-31178: Avoid concatenating bytes with str in subprocess error (GH-3066) (#3388)

Avoid concatenating bytes with str in the typically rare subprocess error path (exec failed). Includes a mock based unittest to exercise the codepath.
(cherry picked from commit 3fc499bca18454b9f432b9b0106cab662bfeb549)

files:
A Misc/NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst
M Lib/subprocess.py
M Lib/test/test_subprocess.py

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index d0132342462..575413d9870 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -1314,15 +1314,18 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
                 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 83abe9d67f7..df3f7506a93 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -1545,6 +1545,53 @@ def test_exception_bad_args_0(self):
         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 00000000000..df018e0c37e
--- /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