From 8f8b86a7d8d139a66525b6d29e3dc63acc2d6915 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Thu, 10 Aug 2017 15:00:41 -0400 Subject: [PATCH 1/9] bpo-31178: Avoid concatenating bytes with str in subprocess error --- Lib/subprocess.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 99bca477cecdab..7a9fc081f1981e 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1307,15 +1307,15 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, try: exception_name, hex_errno, err_msg = ( errpipe_data.split(b':', 2)) + err_msg = err_msg.decode(errors="surrogatepass") except ValueError: exception_name = b'SubprocessError' hex_errno = b'0' - err_msg = (b'Bad exception data from child: ' + - repr(errpipe_data)) + err_msg = errpipe_data.decode(errors="surrogatepass") + err_msg = "Bad exception data from child: '%s'" % err_msg 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") From c277e292235de4ec798ef335f692cc2aad1f839a Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Wed, 16 Aug 2017 09:36:24 -0400 Subject: [PATCH 2/9] Do not attempt to decode in exception path --- Lib/subprocess.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 7a9fc081f1981e..e1c63210f354cc 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1311,8 +1311,8 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, except ValueError: exception_name = b'SubprocessError' hex_errno = b'0' - err_msg = errpipe_data.decode(errors="surrogatepass") - err_msg = "Bad exception data from child: '%s'" % err_msg + err_msg = "Bad exception data from child: {!s}".format( + bytes(errpipe_data)) child_exception_type = getattr( builtins, exception_name.decode('ascii'), SubprocessError) From df3e1d392b71509e205be5535a8be1fa5d582eef Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Thu, 17 Aug 2017 01:29:37 -0400 Subject: [PATCH 3/9] Add tests and remove surrogatepass --- Lib/subprocess.py | 5 ++++- Lib/test/test_subprocess.py | 44 +++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index e1c63210f354cc..12b1e816809cd8 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1307,7 +1307,10 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, try: exception_name, hex_errno, err_msg = ( errpipe_data.split(b':', 2)) - err_msg = err_msg.decode(errors="surrogatepass") + # 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' diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 1d2d15c7a24c3b..1dd344d5133343 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1485,6 +1485,50 @@ 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 + @mock.patch.object(subprocess.Popen, "__del__") + @mock.patch("subprocess._posixsubprocess.fork_exec") + def test_exception_errpipe_normal(self, fork_exec, destructor): + """Test error passing done through errpipe_write in the good case""" + def proper_error(*args): + errpipe_write = args[13] + # 15 is the unix error code for EISDIR: 'is a directory' + os.write(errpipe_write, b"OSError:15:") + return 0 + + fork_exec.side_effect = proper_error + + with self.assertRaises(IsADirectoryError): + subprocess.Popen(["cmd"]) + + @mock.patch.object(subprocess.Popen, "__del__") + @mock.patch("subprocess._posixsubprocess.fork_exec") + def test_exception_errpipe_bad_data(self, fork_exec, destructor): + """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: + subprocess.Popen(["cmd"]) + + 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. From 7410ae22c5100afd9cc2f5757f3822e4fe5e3beb Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Thu, 17 Aug 2017 07:19:06 -0400 Subject: [PATCH 4/9] Use repr instead of str in bad path --- Lib/subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 12b1e816809cd8..0644576f8681d2 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1314,7 +1314,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, except ValueError: exception_name = b'SubprocessError' hex_errno = b'0' - err_msg = "Bad exception data from child: {!s}".format( + err_msg = "Bad exception data from child: {!r}".format( bytes(errpipe_data)) child_exception_type = getattr( builtins, exception_name.decode('ascii'), From f007582b2c8de3f71c258c8f57f7661860af7b55 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Thu, 17 Aug 2017 07:36:30 -0400 Subject: [PATCH 5/9] Add UnicodeError to except in case the junk contains 2 colons --- Lib/subprocess.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 0644576f8681d2..4d851a4e4b49b3 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1311,7 +1311,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, # written in by the subprocess implementations # like _posixsubprocess err_msg = err_msg.decode() - except ValueError: + except (ValueError, UnicodeError): exception_name = b'SubprocessError' hex_errno = b'0' err_msg = "Bad exception data from child: {!r}".format( From f061ff1129cb02bc65fa35918c2bd06ab9163da5 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Thu, 17 Aug 2017 10:11:25 -0400 Subject: [PATCH 6/9] Changes for Serhiy's review --- Lib/subprocess.py | 4 ++-- Lib/test/test_subprocess.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 4d851a4e4b49b3..27f5177db17a0c 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1311,10 +1311,10 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, # written in by the subprocess implementations # like _posixsubprocess err_msg = err_msg.decode() - except (ValueError, UnicodeError): + except ValueError: exception_name = b'SubprocessError' hex_errno = b'0' - err_msg = "Bad exception data from child: {!r}".format( + err_msg = 'Bad exception data from child: {!r}'.format( bytes(errpipe_data)) child_exception_type = getattr( builtins, exception_name.decode('ascii'), diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 1dd344d5133343..e1e868e54ca20d 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1505,7 +1505,7 @@ def proper_error(*args): fork_exec.side_effect = proper_error with self.assertRaises(IsADirectoryError): - subprocess.Popen(["cmd"]) + subprocess.Popen(["non_existent_command"]) @mock.patch.object(subprocess.Popen, "__del__") @mock.patch("subprocess._posixsubprocess.fork_exec") @@ -1524,7 +1524,7 @@ def bad_error(*args): fork_exec.side_effect = bad_error with self.assertRaises(subprocess.SubprocessError) as e: - subprocess.Popen(["cmd"]) + subprocess.Popen(["non_existent_command"]) self.assertIn(repr(error_data), str(e.exception)) From 7a36648700aef6f36d4ca0a55b9fd8afa8b77568 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 20 Aug 2017 08:53:52 -0400 Subject: [PATCH 7/9] Avoid harcoding EISDIR errno --- Lib/test/test_subprocess.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index e1e868e54ca20d..bbc60efcb989b7 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1498,8 +1498,9 @@ def test_exception_errpipe_normal(self, fork_exec, destructor): """Test error passing done through errpipe_write in the good case""" def proper_error(*args): errpipe_write = args[13] - # 15 is the unix error code for EISDIR: 'is a directory' - os.write(errpipe_write, b"OSError:15:") + # 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 From 270761590347bf19baecd09bd1d3ad6ea6d236d8 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Sun, 20 Aug 2017 09:01:26 -0400 Subject: [PATCH 8/9] Mock __del__ with a subclass instead of mocking the method --- Lib/test/test_subprocess.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index bbc60efcb989b7..8c6dc80a154e65 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1492,9 +1492,12 @@ def test_exception_bad_args_0(self): # 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 - @mock.patch.object(subprocess.Popen, "__del__") + class PopenNoDestructor(subprocess.Popen): + def __del__(self): + pass + @mock.patch("subprocess._posixsubprocess.fork_exec") - def test_exception_errpipe_normal(self, fork_exec, destructor): + 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] @@ -1506,11 +1509,10 @@ def proper_error(*args): fork_exec.side_effect = proper_error with self.assertRaises(IsADirectoryError): - subprocess.Popen(["non_existent_command"]) + self.PopenNoDestructor(["non_existent_command"]) - @mock.patch.object(subprocess.Popen, "__del__") @mock.patch("subprocess._posixsubprocess.fork_exec") - def test_exception_errpipe_bad_data(self, fork_exec, destructor): + 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" @@ -1525,7 +1527,7 @@ def bad_error(*args): fork_exec.side_effect = bad_error with self.assertRaises(subprocess.SubprocessError) as e: - subprocess.Popen(["non_existent_command"]) + self.PopenNoDestructor(["non_existent_command"]) self.assertIn(repr(error_data), str(e.exception)) From 8634c2b86da7c8569957e3d0986be5c89a90d232 Mon Sep 17 00:00:00 2001 From: Ammar Askar Date: Tue, 5 Sep 2017 14:55:43 -0400 Subject: [PATCH 9/9] Add news entry --- .../NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2017-09-05-14-55-28.bpo-31178.JrSFo7.rst 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 00000000000000..df018e0c37e7c0 --- /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