Skip to content

Commit 3a4c44b

Browse files
cptpcrdgpshead
cptpcrd
and
gpshead
authored
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] <[email protected]>
1 parent 20e994c commit 3a4c44b

File tree

2 files changed

+164
-130
lines changed

2 files changed

+164
-130
lines changed

‎Lib/subprocess.py‎

Lines changed: 163 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -872,37 +872,6 @@ def __init__(self, args, bufsize=-1, executable=None,
872872
'and universal_newlines are supplied but '
873873
'different. Pass one or the other.')
874874

875-
# Input and output objects. The general principle is like
876-
# this:
877-
#
878-
# Parent Child
879-
# ------ -----
880-
# p2cwrite ---stdin---> p2cread
881-
# c2pread <--stdout--- c2pwrite
882-
# errread <--stderr--- errwrite
883-
#
884-
# On POSIX, the child objects are file descriptors. On
885-
# Windows, these are Windows file handles. The parent objects
886-
# are file descriptors on both platforms. The parent objects
887-
# are -1 when not using PIPEs. The child objects are -1
888-
# when not redirecting.
889-
890-
(p2cread, p2cwrite,
891-
c2pread, c2pwrite,
892-
errread, errwrite) = self._get_handles(stdin, stdout, stderr)
893-
894-
# We wrap OS handles *before* launching the child, otherwise a
895-
# quickly terminating child could make our fds unwrappable
896-
# (see #8458).
897-
898-
if _mswindows:
899-
if p2cwrite != -1:
900-
p2cwrite = msvcrt.open_osfhandle(p2cwrite.Detach(), 0)
901-
if c2pread != -1:
902-
c2pread = msvcrt.open_osfhandle(c2pread.Detach(), 0)
903-
if errread != -1:
904-
errread = msvcrt.open_osfhandle(errread.Detach(), 0)
905-
906875
self.text_mode = encoding or errors or text or universal_newlines
907876
if self.text_mode and encoding is None:
908877
self.encoding = encoding = _text_encoding()
@@ -1003,6 +972,39 @@ def __init__(self, args, bufsize=-1, executable=None,
1003972
if uid < 0:
1004973
raise ValueError(f"User ID cannot be negative, got {uid}")
1005974

975+
# Input and output objects. The general principle is like
976+
# this:
977+
#
978+
# Parent Child
979+
# ------ -----
980+
# p2cwrite ---stdin---> p2cread
981+
# c2pread <--stdout--- c2pwrite
982+
# errread <--stderr--- errwrite
983+
#
984+
# On POSIX, the child objects are file descriptors. On
985+
# Windows, these are Windows file handles. The parent objects
986+
# are file descriptors on both platforms. The parent objects
987+
# are -1 when not using PIPEs. The child objects are -1
988+
# when not redirecting.
989+
990+
(p2cread, p2cwrite,
991+
c2pread, c2pwrite,
992+
errread, errwrite) = self._get_handles(stdin, stdout, stderr)
993+
994+
# From here on, raising exceptions may cause file descriptor leakage
995+
996+
# We wrap OS handles *before* launching the child, otherwise a
997+
# quickly terminating child could make our fds unwrappable
998+
# (see #8458).
999+
1000+
if _mswindows:
1001+
if p2cwrite != -1:
1002+
p2cwrite = msvcrt.open_osfhandle(p2cwrite.Detach(), 0)
1003+
if c2pread != -1:
1004+
c2pread = msvcrt.open_osfhandle(c2pread.Detach(), 0)
1005+
if errread != -1:
1006+
errread = msvcrt.open_osfhandle(errread.Detach(), 0)
1007+
10061008
try:
10071009
if p2cwrite != -1:
10081010
self.stdin = io.open(p2cwrite, 'wb', bufsize)
@@ -1306,6 +1308,26 @@ def _close_pipe_fds(self,
13061308
# Prevent a double close of these handles/fds from __init__ on error.
13071309
self._closed_child_pipe_fds = True
13081310

1311+
@contextlib.contextmanager
1312+
def _on_error_fd_closer(self):
1313+
"""Helper to ensure file descriptors opened in _get_handles are closed"""
1314+
to_close = []
1315+
try:
1316+
yield to_close
1317+
except:
1318+
if hasattr(self, '_devnull'):
1319+
to_close.append(self._devnull)
1320+
del self._devnull
1321+
for fd in to_close:
1322+
try:
1323+
if _mswindows and isinstance(fd, Handle):
1324+
fd.Close()
1325+
else:
1326+
os.close(fd)
1327+
except OSError:
1328+
pass
1329+
raise
1330+
13091331
if _mswindows:
13101332
#
13111333
# Windows methods
@@ -1321,61 +1343,68 @@ def _get_handles(self, stdin, stdout, stderr):
13211343
c2pread, c2pwrite = -1, -1
13221344
errread, errwrite = -1, -1
13231345

1324-
if stdin is None:
1325-
p2cread = _winapi.GetStdHandle(_winapi.STD_INPUT_HANDLE)
1326-
if p2cread is None:
1327-
p2cread, _ = _winapi.CreatePipe(None, 0)
1328-
p2cread = Handle(p2cread)
1329-
_winapi.CloseHandle(_)
1330-
elif stdin == PIPE:
1331-
p2cread, p2cwrite = _winapi.CreatePipe(None, 0)
1332-
p2cread, p2cwrite = Handle(p2cread), Handle(p2cwrite)
1333-
elif stdin == DEVNULL:
1334-
p2cread = msvcrt.get_osfhandle(self._get_devnull())
1335-
elif isinstance(stdin, int):
1336-
p2cread = msvcrt.get_osfhandle(stdin)
1337-
else:
1338-
# Assuming file-like object
1339-
p2cread = msvcrt.get_osfhandle(stdin.fileno())
1340-
p2cread = self._make_inheritable(p2cread)
1341-
1342-
if stdout is None:
1343-
c2pwrite = _winapi.GetStdHandle(_winapi.STD_OUTPUT_HANDLE)
1344-
if c2pwrite is None:
1345-
_, c2pwrite = _winapi.CreatePipe(None, 0)
1346-
c2pwrite = Handle(c2pwrite)
1347-
_winapi.CloseHandle(_)
1348-
elif stdout == PIPE:
1349-
c2pread, c2pwrite = _winapi.CreatePipe(None, 0)
1350-
c2pread, c2pwrite = Handle(c2pread), Handle(c2pwrite)
1351-
elif stdout == DEVNULL:
1352-
c2pwrite = msvcrt.get_osfhandle(self._get_devnull())
1353-
elif isinstance(stdout, int):
1354-
c2pwrite = msvcrt.get_osfhandle(stdout)
1355-
else:
1356-
# Assuming file-like object
1357-
c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
1358-
c2pwrite = self._make_inheritable(c2pwrite)
1359-
1360-
if stderr is None:
1361-
errwrite = _winapi.GetStdHandle(_winapi.STD_ERROR_HANDLE)
1362-
if errwrite is None:
1363-
_, errwrite = _winapi.CreatePipe(None, 0)
1364-
errwrite = Handle(errwrite)
1365-
_winapi.CloseHandle(_)
1366-
elif stderr == PIPE:
1367-
errread, errwrite = _winapi.CreatePipe(None, 0)
1368-
errread, errwrite = Handle(errread), Handle(errwrite)
1369-
elif stderr == STDOUT:
1370-
errwrite = c2pwrite
1371-
elif stderr == DEVNULL:
1372-
errwrite = msvcrt.get_osfhandle(self._get_devnull())
1373-
elif isinstance(stderr, int):
1374-
errwrite = msvcrt.get_osfhandle(stderr)
1375-
else:
1376-
# Assuming file-like object
1377-
errwrite = msvcrt.get_osfhandle(stderr.fileno())
1378-
errwrite = self._make_inheritable(errwrite)
1346+
with self._on_error_fd_closer() as err_close_fds:
1347+
if stdin is None:
1348+
p2cread = _winapi.GetStdHandle(_winapi.STD_INPUT_HANDLE)
1349+
if p2cread is None:
1350+
p2cread, _ = _winapi.CreatePipe(None, 0)
1351+
p2cread = Handle(p2cread)
1352+
err_close_fds.append(p2cread)
1353+
_winapi.CloseHandle(_)
1354+
elif stdin == PIPE:
1355+
p2cread, p2cwrite = _winapi.CreatePipe(None, 0)
1356+
p2cread, p2cwrite = Handle(p2cread), Handle(p2cwrite)
1357+
err_close_fds.extend((p2cread, p2cwrite))
1358+
elif stdin == DEVNULL:
1359+
p2cread = msvcrt.get_osfhandle(self._get_devnull())
1360+
elif isinstance(stdin, int):
1361+
p2cread = msvcrt.get_osfhandle(stdin)
1362+
else:
1363+
# Assuming file-like object
1364+
p2cread = msvcrt.get_osfhandle(stdin.fileno())
1365+
p2cread = self._make_inheritable(p2cread)
1366+
1367+
if stdout is None:
1368+
c2pwrite = _winapi.GetStdHandle(_winapi.STD_OUTPUT_HANDLE)
1369+
if c2pwrite is None:
1370+
_, c2pwrite = _winapi.CreatePipe(None, 0)
1371+
c2pwrite = Handle(c2pwrite)
1372+
err_close_fds.append(c2pwrite)
1373+
_winapi.CloseHandle(_)
1374+
elif stdout == PIPE:
1375+
c2pread, c2pwrite = _winapi.CreatePipe(None, 0)
1376+
c2pread, c2pwrite = Handle(c2pread), Handle(c2pwrite)
1377+
err_close_fds.extend((c2pread, c2pwrite))
1378+
elif stdout == DEVNULL:
1379+
c2pwrite = msvcrt.get_osfhandle(self._get_devnull())
1380+
elif isinstance(stdout, int):
1381+
c2pwrite = msvcrt.get_osfhandle(stdout)
1382+
else:
1383+
# Assuming file-like object
1384+
c2pwrite = msvcrt.get_osfhandle(stdout.fileno())
1385+
c2pwrite = self._make_inheritable(c2pwrite)
1386+
1387+
if stderr is None:
1388+
errwrite = _winapi.GetStdHandle(_winapi.STD_ERROR_HANDLE)
1389+
if errwrite is None:
1390+
_, errwrite = _winapi.CreatePipe(None, 0)
1391+
errwrite = Handle(errwrite)
1392+
err_close_fds.append(errwrite)
1393+
_winapi.CloseHandle(_)
1394+
elif stderr == PIPE:
1395+
errread, errwrite = _winapi.CreatePipe(None, 0)
1396+
errread, errwrite = Handle(errread), Handle(errwrite)
1397+
err_close_fds.extend((errread, errwrite))
1398+
elif stderr == STDOUT:
1399+
errwrite = c2pwrite
1400+
elif stderr == DEVNULL:
1401+
errwrite = msvcrt.get_osfhandle(self._get_devnull())
1402+
elif isinstance(stderr, int):
1403+
errwrite = msvcrt.get_osfhandle(stderr)
1404+
else:
1405+
# Assuming file-like object
1406+
errwrite = msvcrt.get_osfhandle(stderr.fileno())
1407+
errwrite = self._make_inheritable(errwrite)
13791408

13801409
return (p2cread, p2cwrite,
13811410
c2pread, c2pwrite,
@@ -1662,52 +1691,56 @@ def _get_handles(self, stdin, stdout, stderr):
16621691
c2pread, c2pwrite = -1, -1
16631692
errread, errwrite = -1, -1
16641693

1665-
if stdin is None:
1666-
pass
1667-
elif stdin == PIPE:
1668-
p2cread, p2cwrite = os.pipe()
1669-
if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
1670-
fcntl.fcntl(p2cwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
1671-
elif stdin == DEVNULL:
1672-
p2cread = self._get_devnull()
1673-
elif isinstance(stdin, int):
1674-
p2cread = stdin
1675-
else:
1676-
# Assuming file-like object
1677-
p2cread = stdin.fileno()
1694+
with self._on_error_fd_closer() as err_close_fds:
1695+
if stdin is None:
1696+
pass
1697+
elif stdin == PIPE:
1698+
p2cread, p2cwrite = os.pipe()
1699+
err_close_fds.extend((p2cread, p2cwrite))
1700+
if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
1701+
fcntl.fcntl(p2cwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
1702+
elif stdin == DEVNULL:
1703+
p2cread = self._get_devnull()
1704+
elif isinstance(stdin, int):
1705+
p2cread = stdin
1706+
else:
1707+
# Assuming file-like object
1708+
p2cread = stdin.fileno()
16781709

1679-
if stdout is None:
1680-
pass
1681-
elif stdout == PIPE:
1682-
c2pread, c2pwrite = os.pipe()
1683-
if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
1684-
fcntl.fcntl(c2pwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
1685-
elif stdout == DEVNULL:
1686-
c2pwrite = self._get_devnull()
1687-
elif isinstance(stdout, int):
1688-
c2pwrite = stdout
1689-
else:
1690-
# Assuming file-like object
1691-
c2pwrite = stdout.fileno()
1710+
if stdout is None:
1711+
pass
1712+
elif stdout == PIPE:
1713+
c2pread, c2pwrite = os.pipe()
1714+
err_close_fds.extend((c2pread, c2pwrite))
1715+
if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
1716+
fcntl.fcntl(c2pwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
1717+
elif stdout == DEVNULL:
1718+
c2pwrite = self._get_devnull()
1719+
elif isinstance(stdout, int):
1720+
c2pwrite = stdout
1721+
else:
1722+
# Assuming file-like object
1723+
c2pwrite = stdout.fileno()
16921724

1693-
if stderr is None:
1694-
pass
1695-
elif stderr == PIPE:
1696-
errread, errwrite = os.pipe()
1697-
if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
1698-
fcntl.fcntl(errwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
1699-
elif stderr == STDOUT:
1700-
if c2pwrite != -1:
1701-
errwrite = c2pwrite
1702-
else: # child's stdout is not set, use parent's stdout
1703-
errwrite = sys.__stdout__.fileno()
1704-
elif stderr == DEVNULL:
1705-
errwrite = self._get_devnull()
1706-
elif isinstance(stderr, int):
1707-
errwrite = stderr
1708-
else:
1709-
# Assuming file-like object
1710-
errwrite = stderr.fileno()
1725+
if stderr is None:
1726+
pass
1727+
elif stderr == PIPE:
1728+
errread, errwrite = os.pipe()
1729+
err_close_fds.extend((errread, errwrite))
1730+
if self.pipesize > 0 and hasattr(fcntl, "F_SETPIPE_SZ"):
1731+
fcntl.fcntl(errwrite, fcntl.F_SETPIPE_SZ, self.pipesize)
1732+
elif stderr == STDOUT:
1733+
if c2pwrite != -1:
1734+
errwrite = c2pwrite
1735+
else: # child's stdout is not set, use parent's stdout
1736+
errwrite = sys.__stdout__.fileno()
1737+
elif stderr == DEVNULL:
1738+
errwrite = self._get_devnull()
1739+
elif isinstance(stderr, int):
1740+
errwrite = stderr
1741+
else:
1742+
# Assuming file-like object
1743+
errwrite = stderr.fileno()
17111744

17121745
return (p2cread, p2cwrite,
17131746
c2pread, c2pwrite,

‎Misc/NEWS.d/next/Library/2022-08-27-21-41-41.gh-issue-87474.9X-kxt.rst‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix potential file descriptor leaks in :class:`subprocess.Popen`.

0 commit comments

Comments
 (0)