Handle wrapper specific fields never having been set in the C extension - #354
Merged
Merged
Conversation
Calling a FunctionWrapper, BoundFunctionWrapper or PartialCallableObjectProxy for which __init__() was never called, but which had __wrapped__ assigned directly, crashed the interpreter with the C extension. Accessing such a FunctionWrapper as a descriptor did not crash, but returned a wrapper built from the unset fields which crashed when called. The check for an uninitialized wrapper only considers __wrapped__, so it passed and the additional fields were then used while still NULL. This arises where a derived class overrides __init__() and sets __wrapped__ itself without calling __init__() of the base class, or an instance is created using __new__() alone. The fields are now checked once acquired and an AttributeError naming the missing _self_ attribute is raised, as the pure Python implementation already does. The pure Python BoundFunctionWrapper instead failed with a RecursionError, as its __getattr__() looked up _self_parent on itself, and now raises AttributeError too. A crash on calling an uninitialized FunctionWrapper was first noted by Ding Qiuran in #351, which guarded one of the fields involved. This addresses all of the fields and the other affected code paths.
…tes. When __init__() had never been called, the getters in the C extension for the _self_ attributes of a function wrapper returned None, and those of a PartialCallableObjectProxy an empty tuple or dictionary, where the pure Python implementation raised AttributeError as the instance attributes do not exist. None is a valid value for most of these attributes, so the result was indistinguishable from that of an initialized wrapper. The getters now fail with AttributeError. The attribute lookup then falls through to __getattr__() and on to the wrapped object, so the outcome is the same as for the pure Python implementation, including the attribute of the wrapped object being returned where that is itself a wrapper. The captured keyword arguments of a partial are legitimately unset when none were supplied, so _self_kwargs only fails where the captured positional arguments, which __init__() always sets, are unset as well. The tests are moved to test_uninitialized_wrapper.py as they no longer only cover calling a wrapper.
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.
Supersedes the second part of #351. Companion to #353, which handles the
first part.
Problem
Calling a
FunctionWrapper,BoundFunctionWrapperorPartialCallableObjectProxyfor which__init__()was never called, butwhich had
__wrapped__assigned directly, crashed the interpreter whenthe C extension was in use. The realistic way to get there is a derived
class which overrides
__init__()and sets__wrapped__itself withoutcalling
__init__()of the base class:An instance created using
__new__()alone behaves the same. The existingcheck for an uninitialized wrapper only considers whether
__wrapped__isset, so it passed, and the additional fields of the wrapper (
instance,wrapper,enabled,binding,parent,owner, orargsfor apartial) were then used while still NULL.
Four code paths were affected:
FunctionWrapper.__call__BoundFunctionWrapper.__call__PartialCallableObjectProxy.__call__FunctionWrapper.__get__The pure Python implementation raised
AttributeErrorin these cases, asit holds the same state in instance attributes which do not exist. The
exception was
BoundFunctionWrapper, which failed withRecursionError,including on merely assigning
__wrapped__, because its__getattr__()looked up
_self_parenton itself and recursed when it was not set.Separately, reading the same fields through the
_self_attributes didnot crash, but the C extension substituted
None, or an empty tuple ordictionary for a partial, where the pure Python implementation raised
AttributeError.Noneis a valid value for_self_instance,_self_enabled,_self_parentand_self_owner, so the C extensionreported a plausible state for a wrapper which had none.
Changes
wrapt_require_field()helper, applied in each ofthe four functions above to the fields once they have been acquired.
The check is made on the strong references already held, so adds no
further unlocked reads on a free-threaded build. One
Py_DECREFin thecleanup of the partial became
Py_XDECREFso that the error path issafe.
_self_attributes use the samehelper in place of substituting a default. As the lookup of a missing
attribute on a proxy falls through to
__getattr__()and on to thewrapped object, the result is exactly that of the pure Python
implementation, including the attribute of the wrapped object being
returned where that is itself a wrapper.
none were supplied, so they are not required when calling it, and
_self_kwargscontinues to be an empty dictionary in that case. Itfails only where the captured positional arguments, which
__init__()always sets, are NULL as well.
BoundFunctionWrapper.__getattr__()raisesAttributeErrorfor_self_parentitself rather than recursing._self_attribute, forwrappers created both by
__new__()alone and by a derived class notcalling the base
__init__(), across all three types; that the errorfor an attribute is the one the wrapped object gives; that a wrapped
wrapper's attributes are returned; and that initialized wrappers,
including partials with and without keyword arguments, are unaffected.
Choice of exception
AttributeErrornaming the missing_self_attribute, rather thanWrapperNotInitializedError. It is what the pure Python implementationalready raises, so the two agree without adding a try/except to the pure
Python call path, and it follows the existing convention in
raise_uninitialized_wrapper_error(), where__init__()never havingbeen called is an
AttributeError, andWrapperNotInitializedErroriskept for
__init__()having run but__wrapped__since being removed.The message has the same form as the existing one:
Compatibility
The change to the
_self_attributes alters behaviour which did notcrash. Code which reads one of them, or tests for it with
hasattr(),before
__init__()of the base class has been called would have workedwith the C extension and will now raise. Such code already failed with
the pure Python implementation, so was not portable.
Relationship to #351
A crash on calling an uninitialized
FunctionWrapperwas first noted byDing Qiuran in #351, found by static analysis, which added a guard for
the
enabledfield in two of the functions. The other fields acquiredalongside it were still used unchecked, so the crash became a
SystemErrorrather than being resolved, and the partial and descriptorpaths were not covered. This PR addresses all of the fields and all of
the affected paths.
Note on merging
This and #353 both add their changelog entry directly under
**Bugs Fixed**for 2.4.2, so whichever is merged second will have atrivial conflict in
docs/changes.rst.Verification
just testpasses across the full matrix (3.9 to 3.15, three variantseach).