Issue #18851: Avoid a double close of subprocess pipes when the child process fails starting.

3 files changed, 85 insertions, 29 deletions

diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 814cfbe..3108537 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -698,12 +698,12 @@ class Popen(object):
 
         (p2cread, p2cwrite,
          c2pread, c2pwrite,
-         errread, errwrite) = self._get_handles(stdin, stdout, stderr)
+         errread, errwrite), to_close = self._get_handles(stdin, stdout, stderr)
 
         try:
             self._execute_child(args, executable, preexec_fn, close_fds,
                                 cwd, env, universal_newlines,
-                                startupinfo, creationflags, shell,
+                                startupinfo, creationflags, shell, to_close,
                                 p2cread, p2cwrite,
                                 c2pread, c2pwrite,
                                 errread, errwrite)
@@ -711,18 +711,12 @@ class Popen(object):
             # Preserve original exception in case os.close raises.
             exc_type, exc_value, exc_trace = sys.exc_info()
 
-            to_close = []
-            # Only close the pipes we created.
-            if stdin == PIPE:
-                to_close.extend((p2cread, p2cwrite))
-            if stdout == PIPE:
-                to_close.extend((c2pread, c2pwrite))
-            if stderr == PIPE:
-                to_close.extend((errread, errwrite))
-
             for fd in to_close:
                 try:
-                    os.close(fd)
+                    if mswindows:
+                        fd.Close()
+                    else:
+                        os.close(fd)
                 except EnvironmentError:
                     pass
 
@@ -816,8 +810,9 @@ class Popen(object):
             """Construct and return tuple with IO objects:
             p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite
             """
+            to_close = set()
             if stdin is None and stdout is None and stderr is None:
-                return (None, None, None, None, None, None)
+                return (None, None, None, None, None, None), to_close
 
             p2cread, p2cwrite = None, None
             c2pread, c2pwrite = None, None
@@ -835,6 +830,10 @@ class Popen(object):
                 # Assuming file-like object
                 p2cread = msvcrt.get_osfhandle(stdin.fileno())
             p2cread = self._make_inheritable(p2cread)
+            # We just duplicated the handle, it has to be closed at the end
+            to_close.add(p2cread)
+            if stdin == PIPE:
+                to_close.add(p2cwrite)
 
             if stdout is None:
                 c2pwrite = _subprocess.GetStdHandle(_subprocess.STD_OUTPUT_HANDLE)
@@ -848,6 +847,10 @@ class Popen(object):
                 # Assuming file-like object
                 c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
             c2pwrite = self._make_inheritable(c2pwrite)
+            # We just duplicated the handle, it has to be closed at the end
+            to_close.add(c2pwrite)
+            if stdout == PIPE:
+                to_close.add(c2pread)
 
             if stderr is None:
                 errwrite = _subprocess.GetStdHandle(_subprocess.STD_ERROR_HANDLE)
@@ -863,10 +866,14 @@ class Popen(object):
                 # Assuming file-like object
                 errwrite = msvcrt.get_osfhandle(stderr.fileno())
             errwrite = self._make_inheritable(errwrite)
+            # We just duplicated the handle, it has to be closed at the end
+            to_close.add(errwrite)
+            if stderr == PIPE:
+                to_close.add(errread)
 
             return (p2cread, p2cwrite,
                     c2pread, c2pwrite,
-                    errread, errwrite)
+                    errread, errwrite), to_close
 
 
         def _make_inheritable(self, handle):
@@ -895,7 +902,7 @@ class Popen(object):
 
         def _execute_child(self, args, executable, preexec_fn, close_fds,
                            cwd, env, universal_newlines,
-                           startupinfo, creationflags, shell,
+                           startupinfo, creationflags, shell, to_close,
                            p2cread, p2cwrite,
                            c2pread, c2pwrite,
                            errread, errwrite):
@@ -934,6 +941,10 @@ class Popen(object):
                     # kill children.
                     creationflags |= _subprocess.CREATE_NEW_CONSOLE
 
+            def _close_in_parent(fd):
+                fd.Close()
+                to_close.remove(fd)
+
             # Start the process
             try:
                 hp, ht, pid, tid = _subprocess.CreateProcess(executable, args,
@@ -958,11 +969,11 @@ class Popen(object):
                 # pipe will not close when the child process exits and the
                 # ReadFile will hang.
                 if p2cread is not None:
-                    p2cread.Close()
+                    _close_in_parent(p2cread)
                 if c2pwrite is not None:
-                    c2pwrite.Close()
+                    _close_in_parent(c2pwrite)
                 if errwrite is not None:
-                    errwrite.Close()
+                    _close_in_parent(errwrite)
 
             # Retain the process handle, but close the thread handle
             self._child_created = True
@@ -1088,6 +1099,7 @@ class Popen(object):
             """Construct and return tuple with IO objects:
             p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite
             """
+            to_close = set()
             p2cread, p2cwrite = None, None
             c2pread, c2pwrite = None, None
             errread, errwrite = None, None
@@ -1096,6 +1108,7 @@ class Popen(object):
                 pass
             elif stdin == PIPE:
                 p2cread, p2cwrite = self.pipe_cloexec()
+                to_close.update((p2cread, p2cwrite))
             elif isinstance(stdin, int):
                 p2cread = stdin
             else:
@@ -1106,6 +1119,7 @@ class Popen(object):
                 pass
             elif stdout == PIPE:
                 c2pread, c2pwrite = self.pipe_cloexec()
+                to_close.update((c2pread, c2pwrite))
             elif isinstance(stdout, int):
                 c2pwrite = stdout
             else:
@@ -1116,6 +1130,7 @@ class Popen(object):
                 pass
             elif stderr == PIPE:
                 errread, errwrite = self.pipe_cloexec()
+                to_close.update((errread, errwrite))
             elif stderr == STDOUT:
                 errwrite = c2pwrite
             elif isinstance(stderr, int):
@@ -1126,7 +1141,7 @@ class Popen(object):
 
             return (p2cread, p2cwrite,
                     c2pread, c2pwrite,
-                    errread, errwrite)
+                    errread, errwrite), to_close
 
 
         def _set_cloexec_flag(self, fd, cloexec=True):
@@ -1170,7 +1185,7 @@ class Popen(object):
 
         def _execute_child(self, args, executable, preexec_fn, close_fds,
                            cwd, env, universal_newlines,
-                           startupinfo, creationflags, shell,
+                           startupinfo, creationflags, shell, to_close,
                            p2cread, p2cwrite,
                            c2pread, c2pwrite,
                            errread, errwrite):
@@ -1189,6 +1204,10 @@ class Popen(object):
             if executable is None:
                 executable = args[0]
 
+            def _close_in_parent(fd):
+                os.close(fd)
+                to_close.remove(fd)
+
             # For transferring possible exec failure from child to parent
             # The first char specifies the exception type: 0 means
             # OSError, 1 means some other error.
@@ -1283,17 +1302,17 @@ class Popen(object):
                     # be sure the FD is closed no matter what
                     os.close(errpipe_write)
 
-                if p2cread is not None and p2cwrite is not None:
-                    os.close(p2cread)
-                if c2pwrite is not None and c2pread is not None:
-                    os.close(c2pwrite)
-                if errwrite is not None and errread is not None:
-                    os.close(errwrite)
-
                 # Wait for exec to fail or succeed; possibly raising exception
                 # Exception limited to 1M
                 data = _eintr_retry_call(os.read, errpipe_read, 1048576)
             finally:
+                if p2cread is not None and p2cwrite is not None:
+                    _close_in_parent(p2cread)
+                if c2pwrite is not None and c2pread is not None:
+                    _close_in_parent(c2pwrite)
+                if errwrite is not None and errread is not None:
+                    _close_in_parent(errwrite)
+
                 # be sure the FD is closed no matter what
                 os.close(errpipe_read)
 
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index f43b51c..0efcdbf 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -14,6 +14,10 @@ try:
     import resource
 except ImportError:
     resource = None
+try:
+    import threading
+except ImportError:
+    threading = None
 
 mswindows = (sys.platform == "win32")
 
@@ -629,6 +633,36 @@ class ProcessTestCase(BaseTestCase):
             if c.exception.errno not in (errno.ENOENT, errno.EACCES):
                 raise c.exception
 
+    @unittest.skipIf(threading is None, "threading required")
+    def test_double_close_on_error(self):
+        # Issue #18851
+        fds = []
+        def open_fds():
+            for i in range(20):
+                fds.extend(os.pipe())
+                time.sleep(0.001)
+        t = threading.Thread(target=open_fds)
+        t.start()
+        try:
+            with self.assertRaises(EnvironmentError):
+                subprocess.Popen(['nonexisting_i_hope'],
+                                 stdin=subprocess.PIPE,
+                                 stdout=subprocess.PIPE,
+                                 stderr=subprocess.PIPE)
+        finally:
+            t.join()
+            exc = None
+            for fd in fds:
+                # If a double close occurred, some of those fds will
+                # already have been closed by mistake, and os.close()
+                # here will raise.
+                try:
+                    os.close(fd)
+                except OSError as e:
+                    exc = e
+            if exc is not None:
+                raise exc
+
     def test_handles_closed_on_exception(self):
         # If CreateProcess exits with an error, ensure the
         # duplicate output handles are released
@@ -783,7 +817,7 @@ class POSIXProcessTestCase(BaseTestCase):
 
         def _execute_child(
                 self, args, executable, preexec_fn, close_fds, cwd, env,
-                universal_newlines, startupinfo, creationflags, shell,
+                universal_newlines, startupinfo, creationflags, shell, to_close,
                 p2cread, p2cwrite,
                 c2pread, c2pwrite,
                 errread, errwrite):
@@ -791,7 +825,7 @@ class POSIXProcessTestCase(BaseTestCase):
                 subprocess.Popen._execute_child(
                         self, args, executable, preexec_fn, close_fds,
                         cwd, env, universal_newlines,
-                        startupinfo, creationflags, shell,
+                        startupinfo, creationflags, shell, to_close,
                         p2cread, p2cwrite,
                         c2pread, c2pwrite,
                         errread, errwrite)
diff --git a/Misc/NEWS b/Misc/NEWS
index 9544b3b..0db727f 100644
--- a/Misc/NEWS
+++ b/Misc/NEWS
@@ -32,6 +32,9 @@ Core and Builtins
 Library
 -------
 
+- Issue #18851: Avoid a double close of subprocess pipes when the child
+  process fails starting.
+
 - Issue #18418: After fork(), reinit all threads states, not only active ones.
   Patch by A. Jesse Jiryu Davis.