Conversation
Fix __doc__ and __module__ deletion on object proxies
|
I pushed an additional commit, b04070a, addressing another NULL-safety issue found by static analysis. Both WraptFunctionWrapperBase_call and WraptBoundFunctionWrapper_call checked: If self->enabled was NULL during an exceptional or cleared object state, this condition could still be true and NULL could be passed to PyCallable_Check() or PyObject_Not(). The checks now guard against NULL explicitly: The existing enabled-related tests pass locally. |
…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 GrahamDumpleton#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.
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 GrahamDumpleton#351, which guarded one of the fields involved. This addresses all of the fields and the other affected code paths.
|
Thank you for this, and for the care taken in tracking both problems down. I'm closing this PR as superseded rather than merging it, because the
The reasons for doing it that way rather than merging this directly: It covered two unrelated issues. The The second issue turned out to be wider than the guard covered. A different approach was taken to the cached attribute. For the Both PRs credit you for identifying the problems, in the commit messages Thanks again for the contribution. |
2 participants
Summary
When deleting the doc or module attribute from an ObjectProxy, CPython passes NULL as the new value to the C-level setter. This is the standard CPython convention for attribute deletion.
The proxy correctly forwards the deletion to the wrapped object, but then attempted to pass the same NULL pointer to PyDict_SetItemString(). This API requires a valid PyObject pointer, so deleting either attribute could cause the Python interpreter to crash.
Problem
The affected code paths are WraptObjectProxy_set_doc and WraptObjectProxy_set_module.
For normal assignment, the setter receives a valid PyObject pointer and stores the value in the proxy's internal dictionary. For deletion, however, value is NULL. The old implementation still called PyDict_SetItemString() with this NULL value.
This behavior could be triggered by normal Python code such as deleting proxy.doc or proxy.module. It was therefore necessary to handle deletion separately from assignment.
Changes
When value is NULL, both setters now remove the corresponding cached dictionary entry using PyDict_DelItemString().
The implementation also treats an already missing cache entry as a successful deletion, while preserving and returning any unexpected dictionary error.
When value is non-NULL, the existing assignment behavior remains unchanged and the value is stored using PyDict_SetItemString().
The following functions were updated:
WraptObjectProxy_set_doc
WraptObjectProxy_set_module