diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 814cfbe60f1..3108537c504 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 f43b51caf83..0efcdbf2534 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 9544b3b9135..0db727f9320 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.