Skip to content
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.

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

subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments #87474

Closed
cptpcrd mannequin opened this issue Feb 23, 2021 · 2 comments · Fixed by #96351
Closed

subprocess.Popen leaks file descriptors opened for DEVNULL or PIPE stdin/stdout/stderr arguments #87474

cptpcrd mannequin opened this issue Feb 23, 2021 · 2 comments · Fixed by #96351
Assignees
Labels
3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@cptpcrd
Copy link
Mannequin

cptpcrd mannequin commented Feb 23, 2021

BPO 43308
Nosy @gpshead, @cptpcrd
Files
  • subprocess-validation-fd-leak.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    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'

    bugs.python.org fields:

    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']

    Linked PRs

    @cptpcrd
    Copy link
    Mannequin Author

    cptpcrd mannequin commented Feb 23, 2021

    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.

    @cptpcrd cptpcrd mannequin added 3.8 (EOL) end of life 3.9 only security fixes 3.10 only security fixes stdlib Python modules in the Lib dir performance Performance or resource usage labels Feb 23, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel iritkatriel added type-bug An unexpected behavior, bug, or error and removed performance Performance or resource usage labels Sep 10, 2022
    @gpshead gpshead added 3.11 only security fixes and removed 3.9 only security fixes 3.8 (EOL) end of life labels Oct 1, 2022
    gpshead added a commit that referenced this issue May 16, 2023
    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]>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 16, 2023
    …GH-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.
    
    ---------
    
    (cherry picked from commit 3a4c44b)
    
    Co-authored-by: cptpcrd <[email protected]>
    Co-authored-by: Gregory P. Smith [Google] <[email protected]>
    gpshead added a commit that referenced this issue May 17, 2023
    ) (#104563)
    
    gh-87474: Fix file descriptor leaks in subprocess.Popen (GH-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.
    
    ---------
    
    (cherry picked from commit 3a4c44b)
    
    Co-authored-by: cptpcrd <[email protected]>
    Co-authored-by: Gregory P. Smith [Google] <[email protected]>
    @gpshead
    Copy link
    Member

    gpshead commented May 17, 2023

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

    carljm added a commit to carljm/cpython that referenced this issue May 17, 2023
    * main: (26 commits)
      pythonGH-101520: Move tracemalloc functionality into core, leaving interface in Modules. (python#104508)
      typing: Add more tests for TypeVar (python#104571)
      pythongh-104572: Improve error messages for invalid constructs in PEP 695 contexts (python#104573)
      typing: Use PEP 695 syntax in typing.py (python#104553)
      pythongh-102153: Start stripping C0 control and space chars in `urlsplit` (python#102508)
      pythongh-104469: Update README.txt for _testcapi (pythongh-104529)
      pythonGH-103092: isolate `_elementtree` (python#104561)
      pythongh-104050: Add typing to Argument Clinic converters (python#104547)
      pythonGH-103906: Remove immortal refcounting in the interpreter (pythonGH-103909)
      pythongh-87474: Fix file descriptor leaks in subprocess.Popen (python#96351)
      pythonGH-103092: isolate `pyexpat`  (python#104506)
      pythongh-75367: Fix data descriptor detection in inspect.getattr_static (python#104517)
      pythongh-104050: Add more annotations to `Tools/clinic.py` (python#104544)
      pythongh-104555: Fix isinstance() and issubclass() for runtime-checkable protocols that use PEP 695 (python#104556)
      pythongh-103865: add monitoring support to LOAD_SUPER_ATTR (python#103866)
      CODEOWNERS: Assign new PEP 695 files to myself (python#104551)
      pythonGH-104510: Fix refleaks in `_io` base types (python#104516)
      pythongh-104539: Fix indentation error in logging.config.rst (python#104545)
      pythongh-104050: Don't star-import 'types' in Argument Clinic (python#104543)
      pythongh-104050: Add basic typing to CConverter in clinic.py (python#104538)
      ...
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    Successfully merging a pull request may close this issue.

    2 participants