GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-87474: Fix file descriptor leaks in subprocess.Popen by cptpcrd · Pull Request #96351 · python/cpython

gh-87474: Fix file descriptor leaks in subprocess.Popen #96351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gpshead merged 5 commits into python:main from cptpcrd:subprocess-fd-leak
May 16, 2023

Uh oh!

There was an error while loading. Please reload this page.
Merged

gh-87474: Fix file descriptor leaks in subprocess.Popen #96351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6270ccd
Fix file descriptor leaks in subprocess.Popen
cptpcrd Aug 28, 2022
783e6f8
Refactor to use context manager
cptpcrd Oct 2, 2022
0d4efba
Move err_close_fds adds next to pipe creation.
gpshead Oct 7, 2022
0265ce7
add ReST formatting to NEWS.
gpshead May 16, 2023
1b7b0f4
Merge branch 'main' into subprocess-fd-leak
gpshead May 16, 2023
File filter
Filter by extension
Clear filters
Conversations
Failed to load comments.
Loading

Uh oh!

There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading

Uh oh!

There was an error while loading. Please reload this page.
Diff view
Diff view

There are no files selected for viewing

293 changes: 163 additions & 130 deletions 293 Lib/subprocess.py
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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()
Comment thread
gpshead marked this conversation as resolved.
Resolved

Uh oh!

There was an error while loading. Please reload this page.
elif isinstance(stderr, int):
errwrite = stderr
else:
# Assuming file-like object
errwrite = stderr.fileno()

return (p2cread, p2cwrite,
c2pread, c2pwrite,
Expand Down
1 change: 1 addition & 0 deletions 1 Misc/NEWS.d/next/Library/2022-08-27-21-41-41.gh-issue-87474.9X-kxt.rst
Show comments
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix potential file descriptor leaks in :class:`subprocess.Popen`.
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.