Forward deletion of __module__ and __doc__ on a proxy to the wrapped object - #353
Merged
Merged
Conversation
…object. Deleting __module__ or __doc__ on a proxy crashed the interpreter with the C extension, as the attribute setter was invoked with a NULL value for the deletion and passed that on to PyDict_SetItemString() when updating the copy held in the proxy's own dict. The pure Python implementation raised AttributeError instead, as the properties for these attributes had no deleter, so the deletion was never forwarded to the wrapped object. Both implementations now forward the deletion so the outcome matches deleting the attribute on the wrapped object directly. The C extension then refreshes its cached copy from the wrapped object, leaving the proxy in the same state as one newly created over it. The crash and its cause were identified by Ding Qiuran in #351, which also proposed a fix. This supersedes that pull request with a wider set of tests, and separates out the unrelated change it had also picked up.
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 #351. Fixes the interpreter crash reported there.
Problem
Deleting
__module__or__doc__on a proxy, e.g.del proxy.__doc__,crashed the interpreter when the C extension was in use. CPython invokes
the attribute setter with a NULL value for a deletion; the setter forwarded
that correctly to the wrapped object, then passed the same NULL to
PyDict_SetItemString()when updating the copy held in the proxy's owndict. Reachable from ordinary Python code, and present in every release
back to at least 1.x.
The pure Python implementation did not crash but raised
AttributeError: property '__doc__' of 'ObjectProxy' object has no deleter,so the deletion was never forwarded at all. The two implementations
disagreed, and neither matched deleting the attribute on the wrapped object
directly.
Changes
set_module/set_docdistinguish a deletion from anassignment. After forwarding a deletion, a new helper refreshes the
cached copy from the wrapped object, storing the current value if the
attribute still exists (a function's
__doc__becomesNone) ordropping the entry if it is gone. The proxy's
__self_dict__after adeletion is therefore identical to that of a proxy newly created over
the wrapped object.
deleters on the__module__and__doc__properties.__delattr__already routes names defined on the type toobject.__delattr__, which now invokes them, so no special-casing.ObjectProxy,FunctionWrapperand a user subclass;delete after set (the original crash path); class targets, where the
outcome via the proxy is compared with a direct
delattron anequivalent class so it holds across CPython versions and PyPy;
__self_dict__parity with a fresh proxy; deletion via a non-internedattribute name, which reaches the same setters through
setattro.Targets are created per test so nothing mutates the shared module.
Relationship to #351
The crash, its cause and the pure Python divergence were identified by
Ding Qiuran in #351, which also proposed a fix. This PR takes a different
approach to the cache (refresh from the wrapped object rather than delete
the entry), adds broader tests, and leaves out the unrelated
enabled != Py_Noneguard that #351 also picked up. That guard addressesa separate crash in calling an uninitialised
FunctionWrapper, whichturned out to have three affected code paths rather than two, and will be
handled in its own PR.
Verification
just testpasses across the full matrix (3.9 to 3.15, three variantseach). Cross-implementation parity for deletion was checked against
function, class, instance,
intand module targets with identicalresults.