traverseda
/
cpython
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

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

This commit is contained in:
Antoine Pitrou 2013-08-30 23:38:13 +02:00
parent 30a5445b30
commit 33fc744ea3
3 changed files with 85 additions and 29 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
Lib/subprocess.py
View File

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

38
Lib/test/test_subprocess.py
View File

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

3
Misc/NEWS
View File

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