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

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

Merged
merged 5 commits into from May 16, 2023

Conversation

cptpcrd
Copy link
Contributor

@cptpcrd cptpcrd commented Aug 28, 2022

Rationale in #87474. Didn't PR until now because there were no further comments on that issue, and the immediate problem I was seeing was resolved.

Fixes #87474.

@gpshead
Copy link
Member

gpshead commented Oct 1, 2022

Rather than logic that individually keeps track of which things need closing and duplicates decision logic as to which were opened within the exception handler, a cleaner less difficult to mess up tactic could be to use a fd closing context manager that allows registration of fds, and closes them all when the context manager __exit__ is called with an exception rather than a clean exit.

A method something like this perhaps?

@contextlib.contextmanager
def _on_error_fd_closer(self):
    to_close = []
    try:
        yield o_close
    except:
        if hasattr(self, '_devnull):
            fds_to_close.append(self._devnull)
            del self._devnull
        for fd in set(to_close):
            if windows_handle_close := getattr(fd, 'Close'):
                windows_handle_close()
            else:
                os.close(fd)   # wrap this in its own try: ... except: pass for good measure?
        raise
with self._on_error_fd_closer() as fds:
    x, y = os.pipe()
    fds.extend((x, y))
    ...
    # stuff happens
    ...

If an exception occurs, the file descriptors registered by adding them to the context's list of things to cleanup will be closed.

This way it becomes an RAII pattern, the code maintenance mistake that could be made is forgetting to add a fd or handle to the list of things to close. Which should be much more obvious in the code as that would usually happen immediately after its creation instead of duplicate logic in an error handler not within visual range.

@gpshead gpshead self-assigned this Oct 1, 2022
@cptpcrd
Copy link
Contributor Author

cptpcrd commented Oct 2, 2022

@gpshead That's definitely much cleaner. I've incorporated a slightly modified version of that context manager (written to more closely mirror the cleanup code at the end of __init__).

@cptpcrd
Copy link
Contributor Author

cptpcrd commented Oct 14, 2022

@gpshead any other changes you wanted to see/make to this PR before merging?

@cptpcrd
Copy link
Contributor Author

cptpcrd commented Nov 13, 2022

@gpshead pinging again, I don't want this to fall through the cracks.

cptpcrd and others added 3 commits March 12, 2023 15:14
Simplifies the logic and should help avoid mistakes in the future.

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@cptpcrd
Copy link
Contributor Author

cptpcrd commented Mar 12, 2023

@gpshead it's been a while and I think I've addressed all your concerns; any way to get this merged soon?

@hugovk hugovk removed the needs backport to 3.10 only security fixes label Apr 7, 2023
@gpshead gpshead enabled auto-merge (squash) May 16, 2023 19:55
@gpshead gpshead merged commit 3a4c44b into python:main May 16, 2023
23 checks passed
@miss-islington
Copy link
Contributor

Thanks @cptpcrd for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒🤖

@bedevere-bot
Copy link

GH-104563 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 bug and security fixes label May 16, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request 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 <31829097+cptpcrd@users.noreply.github.com>
Co-authored-by: Gregory P. Smith [Google] <greg@krypto.org>
gpshead added a commit that referenced this pull request 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 <31829097+cptpcrd@users.noreply.github.com>
Co-authored-by: Gregory P. Smith [Google] <greg@krypto.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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