# HG changeset patch

# User Antoine Pitrou <solipsis@pitrou.net>

# Date 1377898693 -7200

# Node ID 43749cb6bdbd0fdab70f76cd171c3c02a3f600dd

# Parent  ba54011aa295004ad87438211fe3bb1568dd69ab

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

diff --git a/Lib/subprocess.py b/Lib/subprocess.py

--- 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

--- 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)