diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 0fff36fa5d6..c0151046ec5 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -1419,9 +1419,6 @@ class Popen(object): child_exception_type = getattr( builtins, exception_name.decode('ascii'), SubprocessError) - for fd in (p2cwrite, c2pread, errread): - if fd != -1: - os.close(fd) err_msg = err_msg.decode(errors="surrogatepass") if issubclass(child_exception_type, OSError) and hex_errno: errno_num = int(hex_errno, 16) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 8978b31fb5f..e502618219f 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1189,6 +1189,45 @@ class POSIXProcessTestCase(BaseTestCase): self.fail("Exception raised by preexec_fn did not make it " "to the parent process.") + @unittest.skipIf(not os.path.exists("/dev/zero"), "/dev/zero required.") + def test_preexec_errpipe_does_not_double_close_pipes(self): + """Issue16140: Don't double close pipes on preexec error.""" + class SafeConstructorPopen(subprocess.Popen): + def __init__(self): + pass # Do nothing so we can modify the instance for testing. + def RealPopen(self, *args, **kwargs): + subprocess.Popen.__init__(self, *args, **kwargs) + def raise_it(): + raise subprocess.SubprocessError( + "force the _execute_child() errpipe_data path.") + + p = SafeConstructorPopen() + + def _test_fds_execute_child_wrapper(*args, **kwargs): + try: + subprocess.Popen._execute_child(p, *args, **kwargs) + finally: + # Open a bunch of file descriptors and verify that + # none of them are the same as the ones the Popen + # instance is using for stdin/stdout/stderr. + devzero_fds = [os.open("/dev/zero", os.O_RDONLY) + for _ in range(8)] + try: + for fd in devzero_fds: + self.assertNotIn(fd, ( + p.stdin.fileno(), p.stdout.fileno(), + p.stderr.fileno()), + msg="At least one fd was closed early.") + finally: + map(os.close, devzero_fds) + + p._execute_child = _test_fds_execute_child_wrapper + + with self.assertRaises(subprocess.SubprocessError): + p.RealPopen([sys.executable, "-c", "pass"], + stdin=subprocess.PIPE, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, preexec_fn=raise_it) + def test_preexec_gc_module_failure(self): # This tests the code that disables garbage collection if the child # process will execute any Python. diff --git a/Misc/NEWS b/Misc/NEWS index ba474d8d2bb..9729ba944ed 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -116,6 +116,9 @@ Core and Builtins Library ------- +- Issue #16140: The subprocess module no longer double closes its child + subprocess.PIPE parent file descriptors on child error prior to exec(). + - Remove a bare print to stdout from the subprocess module that could have happened if the child process wrote garbage to its pre-exec error pipe.