Description

assignee = None
closed_at = None
created_at = <Date 2021-02-23.18:26:22.367>
labels = ['3.8', 'library', '3.9', '3.10', 'performance']
title = 'subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments'
updated_at = <Date 2021-02-24.12:03:47.372>
user = 'https://github.com/cptpcrd'

activity = <Date 2021-02-24.12:03:47.372>
actor = 'izbyshev'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2021-02-23.18:26:22.367>
creator = 'cptpcrd'
dependencies = []
files = ['49830']
hgrepos = []
issue_num = 43308
keywords = ['patch']
message_count = 1.0
messages = ['387589']
nosy_count = 2.0
nosy_names = ['gregory.p.smith', 'cptpcrd']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'resource usage'
url = 'https://bugs.python.org/issue43308'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Activity

1. cptpcrd commented on Feb 23, 2021

   

   cptpcrdmannequin

   on Feb 23, 2021

   MannequinAuthor

   More actions

   TL;DR: subprocess.Popen's handling of file descriptors opened for DEVNULL or PIPE inputs/outputs has serious problems, and it can be coerced into leaking file descriptors in several ways. This can cause issues related to resource exhaustion.

   # The basic problem

   As part of its setup, Popen.__init__() calls Popen._get_handles(), which looks at the given stdin/stdout/stderr arguments and returns a tuple of 6 file descriptors (on Windows, file handles) indicating how stdin/stdout/stderr should be redirected. However, these file descriptors aren't properly closed if exceptions occur in certain cases.

   # Variant 1: Bad argument errors (introduced in 3.9)

   The first variant of this bug is shockingly easy to reproduce (note that this only works on platforms with /proc/self/fd, like Linux):

   import os, subprocess
   
   def show_fds():
       for entry in os.scandir("/proc/self/fd"):
           print(entry.name, "->", os.readlink(entry.path))
   
   print("Before:")
   show_fds()
   
   try:
       subprocess.Popen(["ls"], stdin=subprocess.PIPE, user=1.0)
   except TypeError as e:  # "User must be a string or an integer"
       print(e)
   
   print("After:")
   show_fds()

   This produces something like:

   Before:
   0 -> /dev/pts/1
   1 -> /dev/pts/1
   2 -> /dev/pts/1
   3 -> /proc/12345/fd
   User must be a string or an integer
   After:
   0 -> /dev/pts/1
   1 -> /dev/pts/1
   2 -> /dev/pts/1
   3 -> pipe:[1234567]
   3 -> pipe:[1234567]
   5 -> /proc/12345/fd
   

   The process never got launched (because of the invalid user argument), but the (unused) pipe created for piping to stdin is left open! Substituting DEVNULL for PIPE instead leaves a single file descriptor open to /dev/null.

   This happens because the code that validates the user, group, and extra_groups arguments 1 was added to Popen.init() after the call to Popen._get_handles() 2, and there isn't a try/except that closes the file descriptors if an exception gets raised during validation (which can easily happen).

   Variant 2: Error opening file descriptors (seems to have been around in subprocess forever)

   Within Popen._get_handles() (on Windows 3 or POSIX 4), previously opened file descriptors are not closed if an error occurs while opening later file descriptors.

   For example, take the case where only one more file descriptor can be opened without hitting the limit on the number of file descriptors, and subprocess.Popen(["ls"], stdin=subprocess.DEVNULL, stdout=supbrocess.PIPE) is called. subprocess will be able to open /dev/null for stdin, but trying to creating a pipe() for stdout will fail with EMFILE or ENFILE. Since Popen._get_handles() doesn't handle exceptions from pipe() (or when opening /dev/null), the /dev/null file descriptor opened for stdin will be be left open.

   This variant is most easily triggered by file descriptor exhaustion, and it makes that problem worse by leaking even *more* file descriptors.

   Here's an example that reproduces this by monkey-patching os to force an error:

   import os, subprocess
   
   def show_fds():
       for entry in os.scandir("/proc/self/fd"):
           print(entry.name, "->", os.readlink(entry.path))
   
   print("Before:")
   show_fds()
   
   # Trigger an error when trying to open /dev/null
   os.devnull = "/NOEXIST"
   
   try:
       subprocess.Popen(["ls"], stdin=subprocess.PIPE, stdout=subprocess.DEVNULL)
   except FileNotFoundError as e:  # "User must be a string or an integer"
       print(e)
   
   print("After:")
   show_fds()

   Output:

   Before:
   0 -> /dev/pts/1
   1 -> /dev/pts/1
   2 -> /dev/pts/1
   3 -> /proc/12345/fd
   [Errno 2] No such file or directory: '/dev/null'
   After:
   0 -> /dev/pts/1
   1 -> /dev/pts/1
   2 -> /dev/pts/1
   3 -> pipe:[1234567]
   4 -> pipe:[1234567]
   5 -> /proc/12345/fd
   

   Again, the pipe is left open.

   # Paths to fix.

   Variant 1 can be fixed by simply reordering code in Popen.__init__() (and leaving comments warning about the importance of maintaining the order!). I've attached a basic patch that does this.

   Variant 2 might take some more work -- especially given the shared Popen._devnull file descriptor that needs to be accounted for separately -- and may require significant changes to both Popen.__init__() and Popen._get_handles() to fix.
2. cptpcrd

   added

   3.8 (EOL)end of lifeend of life

   3.9 (EOL)end of lifeend of life

   3.10 (EOL)end of lifeend of life

   stdlibStandard Library Python modules in the Lib/ directoryStandard Library Python modules in the Lib/ directory

   performancePerformance or resource usagePerformance or resource usage

   on Feb 23, 2021
3. ezio-melotti

   transferred this issue fromon Apr 10, 2022
4. iritkatriel

   added

   type-bugAn unexpected behavior, bug, or errorAn unexpected behavior, bug, or error

   and removed

   performancePerformance or resource usagePerformance or resource usage

   on Sep 10, 2022
5. kumaraditya303

   assigned

   gpshead

   on Sep 30, 2022
6. gpshead

   added

   3.11only security fixesonly security fixes

   and removed

   3.9 (EOL)end of lifeend of life

   3.8 (EOL)end of lifeend of life

   on Oct 1, 2022
7. gpshead

   closed this as completedin #96351on May 16, 2023
8. gpshead

   added a commit that references this issue on May 16, 2023

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

   3a4c44b
9. miss-islington

   added a commit that references this issue on May 16, 2023

   pythongh-87474: Fix file descriptor leaks in subprocess.Popen (python…

   96ca688
10. gpshead

    added a commit that references this issue on May 17, 2023

    [3.11] gh-87474: Fix file descriptor leaks in subprocess.Popen (GH-96351

    3ce7d57
11. gpshead commented on May 17, 2023

    

    gpshead

    on May 17, 2023

    Member

    More actions

    Thanks for all your work diagnosing and fixing these. It is a good cleanup!
12. carljm

    added a commit that references this issue on May 17, 2023

    Merge branch 'main' into 709funconly

    b1bb3a7

Issue actions