Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.4k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
gpshead commentedOct 1, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 A method something like this perhaps? @contextlib.contextmanagerdef_on_error_fd_closer(self):to_close= []try:yieldo_closeexcept:ifhasattr(self, '_devnull):fds_to_close.append(self._devnull)delself._devnullforfdinset(to_close):ifwindows_handle_close:=getattr(fd,'Close'):windows_handle_close()else:os.close(fd)# wrap this in its own try: ... except: pass for good measure?raise withself._on_error_fd_closer()asfds: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. |
cptpcrd commentedOct 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 |
Uh oh!
There was an error while loading.Please reload this page.
cptpcrd commentedOct 14, 2022
@gpshead any other changes you wanted to see/make to this PR before merging? |
cptpcrd commentedNov 13, 2022
@gpshead pinging again, I don't want this to fall through the cracks. |
Simplifies the logic and should help avoid mistakes in the future.Co-authored-by: Gregory P. Smith <greg@krypto.org>
cptpcrd commentedMar 12, 2023
@gpshead it's been a while and I think I've addressed all your concerns; any way to get this merged soon? |
miss-islington commentedMay 16, 2023
bedevere-bot commentedMay 16, 2023
GH-104563 is a backport of this pull request to the3.11 branch. |
…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 commit3a4c44b)Co-authored-by: cptpcrd <31829097+cptpcrd@users.noreply.github.com>Co-authored-by: Gregory P. Smith [Google] <greg@krypto.org>
) (#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 commit3a4c44b)Co-authored-by: cptpcrd <31829097+cptpcrd@users.noreply.github.com>Co-authored-by: Gregory P. Smith [Google] <greg@krypto.org>
* 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) ...
Uh oh!
There was an error while loading.Please reload this page.
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.