[Python-checkins] gh-87474: Fix file descriptor leaks in subprocess.Popen (#96351)
gpshead
webhook-mailer at python.org
Tue May 16 16:24:02 EDT 2023
https://github.com/python/cpython/commit/3a4c44bb1e92802db64deec59cf8a68ad3973219
commit: 3a4c44bb1e92802db64deec59cf8a68ad3973219
branch: main
author: cptpcrd <31829097+cptpcrd at users.noreply.github.com>
committer: gpshead <greg at krypto.org>
date: 2023-05-16T20:23:53Z
summary:
gh-87474: Fix file descriptor leaks in subprocess.Popen (#96351)
This fixes several ways file descriptors could be leaked from `subprocess.Popen` constructor during error conditions by opening them later and using a context manager "fds to close" registration scheme to ensure they get closed before returning.
---------
Co-authored-by: Gregory P. Smith [Google] <greg at krypto.org>
files:
A Misc/NEWS.d/next/Library/2022-08-27-21-41-41.gh-issue-87474.9X-kxt.rst
M Lib/subprocess.py
diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 1f203bd00d35..fbc76b8d0f14 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -872,37 +872,6 @@ def __init__(self, args, bufsize=-1, executable=None,
'and universal_newlines are supplied but '
'different. Pass one or the other.')
- # Input and output objects. The general principle is like
- # this:
- #
- # Parent Child
- # ------ -----
- # p2cwrite ---stdin---> p2cread
- # c2pread <--stdout--- c2pwrite
- # errread <--stderr--- errwrite
- #
- # On POSIX, the child objects are file descriptors. On
- # Windows, these are Windows file handles. The parent objects
- # are file descriptors on both platforms. The parent objects
- # are -1 when not using PIPEs. The child objects are -1
- # when not redirecting.
-
- (p2cread, p2cwrite,
- c2pread, c2pwrite,
- errread, errwrite) = self._get_handles(stdin, stdout, stderr)
-
- # We wrap OS handles *before* launching the child, otherwise a
- # quickly terminating child could make our fds unwrappable
- # (see #8458).
-
- if _mswindows:
- if p2cwrite != -1:
- p2cwrite = msvcrt.open_osfhandle(p2cwrite.Detach(), 0)
- if c2pread != -1:
- c2pread = msvcrt.open_osfhandle(c2pread.Detach(), 0)
- if errread != -1:
- errread = msvcrt.open_osfhandle(errread.Detach(), 0)
-
self.text_mode = encoding or errors or text or universal_newlines
if self.text_mode and encoding is None:
self.encoding = encoding = _text_encoding()
@@ -1003,6 +972,39 @@ def __init__(self, args, bufsize=-1, executable=None,
if uid < 0:
raise ValueError(f"User ID cannot be negative, got {uid}")
+ # Input and output objects. The general principle is like
+ # this:
+ #
+ # Parent Child
+ # ------ -----
+ # p2cwrite ---stdin---> p2cread
+ # c2pread <--stdout--- c2pwrite
+ # errread <--stderr--- errwrite
+ #
+ # On POSIX, the child objects are file descriptors. On
+ # Windows, these are Windows file handles. The parent objects
+ # are file descriptors on both platforms. The parent objects
+ # are -1 when not using PIPEs. The child objects are -1
+ # when not redirecting.
+
+ (p2cread, p2cwrite,
+ c2pread, c2pwrite,
+ errread, errwrite) = self._get_handles(stdin, stdout, stderr)
+
+ # From here on, raising exceptions may cause file descriptor leakage
+
+ # We wrap OS handles *before* launching the child, otherwise a
+ # quickly terminating child could make our fds unwrappable
+ # (see #8458).
+
+ if _mswindows:
+ if p2cwrite != -1:
+ p2cwrite = msvcrt.open_osfhandle(p2cwrite.Detach(), 0)
+ if c2pread != -1:
+ c2pread = msvcrt.open_osfhandle(c2pread.Detach(), 0)
+ if errread != -1:
+ errread = msvcrt.open_osfhandle(errread.Detach(), 0)
+
try:
if p2cwrite != -1:
self.stdin = io.open(p2cwrite, 'wb', bufsize)
@@ -1306,6 +1308,26 @@ def _close_pipe_fds(self,
# Prevent a double close of these handles/fds from __init__ on error.
self._closed_child_pipe_fds = True
+ @contextlib.contextmanager
+ def _on_error_fd_closer(self):
+ """Helper to ensure file descriptors opened in _get_handles are closed"""
+ to_close = []
+ try:
+ yield to_close
+ except:
+ if hasattr(self, '_devnull'):
+ to_close.append(self._devnull)
+ del self._devnull
+ for fd in to_close:
+ try:
+ if _mswindows and isinstance(fd, Handle):
+ fd.Close()
+ else:
+ os.close(fd)
+ except OSError:
+ pass
+ raise
+
if _mswindows:
#
# Windows methods
@@ -1321,61 +1343,68 @@ def _get_handles(self, stdin, stdout, stderr):
c2pread, c2pwrite = -1, -1
errread, errwrite = -1, -1
- if stdin is None:
- p2cread = _winapi.GetStdHandle(_winapi.STD_INPUT_HANDLE)
- if p2cread is None:
- p2cread, _ = _winapi.CreatePipe(None, 0)
- p2cread = Handle(p2cread)
- _winapi.CloseHandle(_)
- elif stdin == PIPE:
- p2cread, p2cwrite = _winapi.CreatePipe(None, 0)
- p2cread, p2cwrite = Handle(p2cread), Handle(p2cwrite)
- elif stdin == DEVNULL:
- p2cread = msvcrt.get_osfhandle(self._get_devnull())
- elif isinstance(stdin, int):
- p2cread = msvcrt.get_osfhandle(stdin)
- else:
- # Assuming file-like object
- p2cread = msvcrt.get_osfhandle(stdin.fileno())
- p2cread = self._make_inheritable(p2cread)
-
- if stdout is None:
- c2pwrite = _winapi.GetStdHandle(_winapi.STD_OUTPUT_HANDLE)
- if c2pwrite is None:
- _, c2pwrite = _winapi.CreatePipe(None, 0)
- c2pwrite = Handle(c2pwrite)
- _winapi.CloseHandle(_)
- elif stdout == PIPE:
- c2pread, c2pwrite = _winapi.CreatePipe(None, 0)
- c2pread, c2pwrite = Handle(c2pread), Handle(c2pwrite)
- elif stdout == DEVNULL:
- c2pwrite = msvcrt.get_osfhandle(self._get_devnull())
- elif isinstance(stdout, int):
- c2pwrite = msvcrt.get_osfhandle(stdout)
- else:
- # Assuming file-like object
- c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
- c2pwrite = self._make_inheritable(c2pwrite)
-
- if stderr is None:
- errwrite = _winapi.GetStdHandle(_winapi.STD_ERROR_HANDLE)
- if errwrite is None:
- _, errwrite = _winapi.CreatePipe(None, 0)
- errwrite = Handle(errwrite)
- _winapi.CloseHandle(_)
- elif stderr == PIPE:
- errread, errwrite = _winapi.CreatePipe(None, 0)
- errread, errwrite = Handle(errread), Handle(errwrite)
- elif stderr == STDOUT:
- errwrite = c2pwrite
- elif stderr == DEVNULL:
- errwrite = msvcrt.get_osfhandle(self._get_devnull())
- elif isinstance(stderr, int):
- errwrite = msvcrt.get_osfhandle(stderr)
- else:
- # Assuming file-like object
- errwrite = msvcrt.get_osfhandle(stderr.fileno())
- errwrite = self._make_inheritable(errwrite)
+ with self._on_error_fd_closer() as err_close_fds:
+ if stdin is None:
+ p2cread = _winapi.GetStdHandle(_winapi.STD_INPUT_HANDLE)
+ if p2cread is None:
+ p2cread, _ = _winapi.CreatePipe(None, 0)
+ p2cread = Handle(p2cread)
+ err_close_fds.append(p2cread)
+ _winapi.CloseHandle(_)
+ elif stdin == PIPE:
+ p2cread, p2cwrite = _winapi.CreatePipe(None, 0)
+ p2cread, p2cwrite = Handle(p2cread), Handle(p2cwrite)
+ err_close_fds.extend((p2cread, p2cwrite))
+ elif stdin == DEVNULL:
+ p2cread = msvcrt.get_osfhandle(self._get_devnull())
+ elif isinstance(stdin, int):
+ p2cread = msvcrt.get_osfhandle(stdin)
+ else:
+ # Assuming file-like object
+ p2cread = msvcrt.get_osfhandle(stdin.fileno())
+ p2cread = self._make_inheritable(p2cread)
+
+ if stdout is None:
+ c2pwrite = _winapi.GetStdHandle(_winapi.STD_OUTPUT_HANDLE)
+ if c2pwrite is None:
+ _, c2pwrite = _winapi.CreatePipe(None, 0)
+ c2pwrite = Handle(c2pwrite)
+ err_close_fds.append(c2pwrite)
+ _winapi.CloseHandle(_)
+ elif stdout == PIPE:
+ c2pread, c2pwrite = _winapi.CreatePipe(None, 0)
+ c2pread, c2pwrite = Handle(c2pread), Handle(c2pwrite)
+ err_close_fds.extend((c2pread, c2pwrite))
+ elif stdout == DEVNULL:
+ c2pwrite = msvcrt.get_osfhandle(self._get_devnull())
+ elif isinstance(stdout, int):
+ c2pwrite = msvcrt.get_osfhandle(stdout)
+ else:
+ # Assuming file-like object
+ c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
+ c2pwrite = self._make_inheritable(c2pwrite)
+
+ if stderr is None:
+ errwrite = _winapi.GetStdHandle(_winapi.STD_ERROR_HANDLE)
+ if errwrite is None:
+ _, errwrite = _winapi.CreatePipe(None, 0)
+ errwrite = Handle(errwrite)
+ err_close_fds.append(errwrite)
+ _winapi.CloseHandle(_)
+ elif stderr == PIPE:
+ errread, errwrite = _winapi.CreatePipe(None, 0)
+ errread, errwrite = Handle(errread), Handle(errwrite)
+ err_close_fds.extend((errread, errwrite))
+ elif stderr == STDOUT:
+ errwrite = c2pwrite
+ elif stderr == DEVNULL:
+ errwrite = msvcrt.get_osfhandle(self._get_devnull())
+ elif isinstance(stderr, int):
+ errwrite = msvcrt.get_osfhandle(stderr)
+ else:
+ # Assuming file-like object
+ errwrite = msvcrt.get_osfhandle(stderr.fileno())
+ errwrite = self._make_inheritable(errwrite)
return (p2cread, p2cwrite,
c2pread, c2pwrite,
@@ -1662,52 +1691,56 @@ def _get_handles(self, stdin, stdout, stderr):
c2pread, c2pwrite = -1, -1
errread, errwrite = -1, -1
- if stdin is None:
- pass
- elif stdin == PIPE:
- p2cread, p2cwrite = os.pipe()
- if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
- fcntl.fcntl(p2cwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
- elif stdin == DEVNULL:
- p2cread = self._get_devnull()
- elif isinstance(stdin, int):
- p2cread = stdin
- else:
- # Assuming file-like object
- p2cread = stdin.fileno()
+ with self._on_error_fd_closer() as err_close_fds:
+ if stdin is None:
+ pass
+ elif stdin == PIPE:
+ p2cread, p2cwrite = os.pipe()
+ err_close_fds.extend((p2cread, p2cwrite))
+ if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
+ fcntl.fcntl(p2cwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
+ elif stdin == DEVNULL:
+ p2cread = self._get_devnull()
+ elif isinstance(stdin, int):
+ p2cread = stdin
+ else:
+ # Assuming file-like object
+ p2cread = stdin.fileno()
- if stdout is None:
- pass
- elif stdout == PIPE:
- c2pread, c2pwrite = os.pipe()
- if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
- fcntl.fcntl(c2pwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
- elif stdout == DEVNULL:
- c2pwrite = self._get_devnull()
- elif isinstance(stdout, int):
- c2pwrite = stdout
- else:
- # Assuming file-like object
- c2pwrite = stdout.fileno()
+ if stdout is None:
+ pass
+ elif stdout == PIPE:
+ c2pread, c2pwrite = os.pipe()
+ err_close_fds.extend((c2pread, c2pwrite))
+ if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
+ fcntl.fcntl(c2pwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
+ elif stdout == DEVNULL:
+ c2pwrite = self._get_devnull()
+ elif isinstance(stdout, int):
+ c2pwrite = stdout
+ else:
+ # Assuming file-like object
+ c2pwrite = stdout.fileno()
- if stderr is None:
- pass
- elif stderr == PIPE:
- errread, errwrite = os.pipe()
- if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
- fcntl.fcntl(errwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
- elif stderr == STDOUT:
- if c2pwrite != -1:
- errwrite = c2pwrite
- else: # child's stdout is not set, use parent's stdout
- errwrite = sys.__stdout__.fileno()
- elif stderr == DEVNULL:
- errwrite = self._get_devnull()
- elif isinstance(stderr, int):
- errwrite = stderr
- else:
- # Assuming file-like object
- errwrite = stderr.fileno()
+ if stderr is None:
+ pass
+ elif stderr == PIPE:
+ errread, errwrite = os.pipe()
+ err_close_fds.extend((errread, errwrite))
+ if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
+ fcntl.fcntl(errwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
+ elif stderr == STDOUT:
+ if c2pwrite != -1:
+ errwrite = c2pwrite
+ else: # child's stdout is not set, use parent's stdout
+ errwrite = sys.__stdout__.fileno()
+ elif stderr == DEVNULL:
+ errwrite = self._get_devnull()
+ elif isinstance(stderr, int):
+ errwrite = stderr
+ else:
+ # Assuming file-like object
+ errwrite = stderr.fileno()
return (p2cread, p2cwrite,
c2pread, c2pwrite,
diff --git a/Misc/NEWS.d/next/Library/2022-08-27-21-41-41.gh-issue-87474.9X-kxt.rst b/Misc/NEWS.d/next/Library/2022-08-27-21-41-41.gh-issue-87474.9X-kxt.rst
new file mode 100644
index 000000000000..eeb5308680e8
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2022-08-27-21-41-41.gh-issue-87474.9X-kxt.rst
@@ -0,0 +1 @@
+Fix potential file descriptor leaks in :class:`subprocess.Popen`.
More information about the Python-checkins
mailing list