From a043bf190c2567c78823bb9f378cf8ddb19037c9 Mon Sep 17 00:00:00 2001 From: Segev Finer Date: Fri, 18 Aug 2017 16:18:13 +0300 Subject: [PATCH 1/2] bpo-30121: Fix debug assert in subprocess on Windows (#1224) * bpo-30121: Fix debug assert in subprocess on Windows This is caused by closing HANDLEs using os.close which is for CRT file descriptors and not for HANDLEs. * bpo-30121: Suppress debug assertion in test_subprocess when ran directly (cherry picked from commit 4d3851727fb82195e4995c6064b0b2d6cbc031c4) --- Lib/subprocess.py | 8 +++++++- Lib/test/test_subprocess.py | 9 +++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index b790cbd65ba208..e0a93c55dc0f18 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -725,7 +725,10 @@ def __init__(self, args, bufsize=-1, executable=None, to_close.append(self._devnull) for fd in to_close: try: - os.close(fd) + if _mswindows and isinstance(fd, Handle): + fd.Close() + else: + os.close(fd) except OSError: pass @@ -1005,6 +1008,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, errwrite.Close() if hasattr(self, '_devnull'): os.close(self._devnull) + # Prevent a double close of these handles/fds from __init__ + # on error. + self._closed_child_pipe_fds = True # Retain the process handle, but close the thread handle self._child_created = True diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 004f1f01ab5046..8c521d21515ba8 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1120,10 +1120,11 @@ def _test_bufsize_equal_one(self, line, expected, universal_newlines): p.stdin.write(line) # expect that it flushes the line in text mode os.close(p.stdin.fileno()) # close it without flushing the buffer read_line = p.stdout.readline() - try: - p.stdin.close() - except OSError: - pass + with support.SuppressCrashReport(): + try: + p.stdin.close() + except OSError: + pass p.stdin = None self.assertEqual(p.returncode, 0) self.assertEqual(read_line, expected) From d4e183b1d33576b03274239ef7d00d89193987fb Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 21 Aug 2017 23:51:31 +0200 Subject: [PATCH 2/2] Add test_subprocess.test_nonexisting_with_pipes() (#3133) bpo-30121: Test the Popen failure when Popen was created with pipes. Create also NONEXISTING_CMD variable in test_subprocess.py. (cherry picked from commit 9a83f651f31b47b3f6c8b210f7807b26e8c373a5) --- Lib/test/test_subprocess.py | 51 ++++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 8c521d21515ba8..ccdd3232700470 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -49,6 +49,8 @@ else: SETBINARY = '' +NONEXISTING_CMD = ('nonexisting_i_hope',) + class BaseTestCase(unittest.TestCase): def setUp(self): @@ -1149,13 +1151,54 @@ def test_leaking_fds_on_error(self): # 1024 times (each call leaked two fds). for i in range(1024): with self.assertRaises(OSError) as c: - subprocess.Popen(['nonexisting_i_hope'], + subprocess.Popen(NONEXISTING_CMD, stdout=subprocess.PIPE, stderr=subprocess.PIPE) # ignore errors that indicate the command was not found if c.exception.errno not in (errno.ENOENT, errno.EACCES): raise c.exception + def test_nonexisting_with_pipes(self): + # bpo-30121: Popen with pipes must close properly pipes on error. + # Previously, os.close() was called with a Windows handle which is not + # a valid file descriptor. + # + # Run the test in a subprocess to control how the CRT reports errors + # and to get stderr content. + try: + import msvcrt + msvcrt.CrtSetReportMode + except (AttributeError, ImportError): + self.skipTest("need msvcrt.CrtSetReportMode") + + code = textwrap.dedent(f""" + import msvcrt + import subprocess + + cmd = {NONEXISTING_CMD!r} + + for report_type in [msvcrt.CRT_WARN, + msvcrt.CRT_ERROR, + msvcrt.CRT_ASSERT]: + msvcrt.CrtSetReportMode(report_type, msvcrt.CRTDBG_MODE_FILE) + msvcrt.CrtSetReportFile(report_type, msvcrt.CRTDBG_FILE_STDERR) + + try: + subprocess.Popen([cmd], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + except OSError: + pass + """) + cmd = [sys.executable, "-c", code] + proc = subprocess.Popen(cmd, + stderr=subprocess.PIPE, + universal_newlines=True) + with proc: + stderr = proc.communicate()[1] + self.assertEqual(stderr, "") + self.assertEqual(proc.returncode, 0) + @unittest.skipIf(threading is None, "threading required") def test_double_close_on_error(self): # Issue #18851 @@ -1168,7 +1211,7 @@ def open_fds(): t.start() try: with self.assertRaises(EnvironmentError): - subprocess.Popen(['nonexisting_i_hope'], + subprocess.Popen(NONEXISTING_CMD, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE) @@ -2434,7 +2477,7 @@ def test_leak_fast_process_del_killed(self): # should trigger the wait() of p time.sleep(0.2) with self.assertRaises(OSError) as c: - with subprocess.Popen(['nonexisting_i_hope'], + with subprocess.Popen(NONEXISTING_CMD, stdout=subprocess.PIPE, stderr=subprocess.PIPE) as proc: pass @@ -2864,7 +2907,7 @@ def test_communicate_stdin(self): def test_invalid_args(self): with self.assertRaises((FileNotFoundError, PermissionError)) as c: - with subprocess.Popen(['nonexisting_i_hope'], + with subprocess.Popen(NONEXISTING_CMD, stdout=subprocess.PIPE, stderr=subprocess.PIPE) as proc: pass