Popen.__getattribute__(): don't recurse if __init__() failed - #3000
Open
istoolsfox wants to merge 1 commit into
Open
istoolsfox wants to merge 1 commit into
istoolsfox wants to merge 1 commit into
Conversation
If Popen.__init__() raises (e.g. the executable does not exist), the half-initialized instance has no _Popen__subproc attribute. The fallback branch of __getattribute__() accessed it via attribute syntax (self.__subproc), which re-enters __getattribute__() and recurses until the stack is exhausted. Normal usage never notices, because the exception from __init__() propagates and the instance is discarded, but any code introspecting the broken instance (e.g. Sentry-style frame walking, issue giampaolo#1121) or a subclass swallowing the original error gets a RecursionError instead of a bounded exception. Look _Popen__subproc up via object.__getattribute__() instead, which does not re-enter this method. What __init__() raises is unchanged; delegation for successfully initialized instances behaves the same, with one dispatch less in the fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Description
This one has been open since 2017, and Cykooz had the mechanism figured out back then already: when
Popen.__init__()fails (say the executable does not exist), the instance never gets its__subprocattribute. The fallback in__getattribute__()then reads it with the usual attribute syntax, which re-enters__getattribute__()and recurses until the stack blows. His last comment in the thread is essentially this fix, it just never turned into a PR, and the recursion is still there on master today.You can only hit it by touching the half-initialized instance, which is probably why it survived: normal code gets the
FileNotFoundErrorfrom the constructor and moves on. The 2017 report hit it through raven walking frame locals and callingrepr()on the object. The other way in is a subclass that swallows the error and uses the instance later:The fix is the direct lookup Cykooz sketched:
object.__getattribute__(self, '_Popen__subproc'), with the mangled name spelled out so the fallback no longer re-enters the method.Nothing else changes on purpose. The constructor still raises the original
FileNotFoundError. A working instance delegates exactly like before (the fallback actually saves one dispatch now). Touching a broken instance gives you theAttributeErrorthe existing handler already produces, instead of a stack trace. I saw your note from 2017 about the class-attribute variant surfacingAttributeErrorwhere you would expectFileNotFoundError; this patch stays away from that and does not try to decide what these operations should raise on a broken instance, it only stops the stack from blowing. If you would rather see something else raised there, happy to adjust.TestPopen.test__getattribute__failed_initreproduces the scenario from the issue and fails withRecursionErroron master.