Fixes issue #16327: The subprocess module no longer leaks file descriptors

used for stdin/stdout/stderr pipes to the child when fork() fails.

2 files changed, 50 insertions, 3 deletions

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 296613a..02c722b 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -817,13 +817,27 @@ class Popen(object):
                                 errread, errwrite,
                                 restore_signals, start_new_session)
         except:
-            # Cleanup if the child failed starting
-            for f in filter(None, [self.stdin, self.stdout, self.stderr]):
+            # Cleanup if the child failed starting.
+            for f in filter(None, (self.stdin, self.stdout, self.stderr)):
                 try:
                     f.close()
                 except EnvironmentError:
-                    # Ignore EBADF or other errors
+                    pass  # Ignore EBADF or other errors.
+
+            # Make sure the child pipes are closed as well.
+            to_close = []
+            if stdin == PIPE:
+                to_close.append(p2cread)
+            if stdout == PIPE:
+                to_close.append(c2pwrite)
+            if stderr == PIPE:
+                to_close.append(errwrite)
+            for fd in to_close:
+                try:
+                    os.close(fd)
+                except EnvironmentError:
                     pass
+
             raise
 
 
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index b260c81..2d77554 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -68,6 +68,18 @@ class BaseTestCase(unittest.TestCase):
         self.assertEqual(actual, expected, msg)
 
 
+class PopenTestException(Exception):
+    pass
+
+
+class PopenExecuteChildRaises(subprocess.Popen):
+    """Popen subclass for testing cleanup of subprocess.PIPE filehandles when
+    _execute_child fails.
+    """
+    def _execute_child(self, *args, **kwargs):
+        raise PopenTestException("Forced Exception for Test")
+
+
 class ProcessTestCase(BaseTestCase):
 
     def test_call_seq(self):
@@ -995,6 +1007,27 @@ class ProcessTestCase(BaseTestCase):
                 process.communicate()
 
 
+    # This test is Linux-ish specific for simplicity to at least have
+    # some coverage.  It is not a platform specific bug.
+    @unittest.skipUnless(os.path.isdir('/proc/%d/fd' % os.getpid()),
+                         "Linux specific")
+    def test_failed_child_execute_fd_leak(self):
+        """Test for the fork() failure fd leak reported in issue16327."""
+        fd_directory = '/proc/%d/fd' % os.getpid()
+        fds_before_popen = os.listdir(fd_directory)
+        with self.assertRaises(PopenTestException):
+            PopenExecuteChildRaises(
+                    [sys.executable, '-c', 'pass'], stdin=subprocess.PIPE,
+                    stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+
+        # NOTE: This test doesn't verify that the real _execute_child
+        # does not close the file descriptors itself on the way out
+        # during an exception.  Code inspection has confirmed that.
+
+        fds_after_exception = os.listdir(fd_directory)
+        self.assertEqual(fds_before_popen, fds_after_exception)
+
+
 # context manager
 class _SuppressCoreFiles(object):
     """Try to prevent core files from being created."""