From 6ca497dc264b972ea3adcb723e2529a9dbe22398 Mon Sep 17 00:00:00 2001 From: Graham Dumpleton Date: Sun, 20 Sep 2026 17:08:33 +1000 Subject: [PATCH] Forward deletion of __module__ and __doc__ on a proxy to the wrapped 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. --- docs/changes.rst | 19 ++++ src/wrapt/_wrappers.c | 71 ++++++++++-- src/wrapt/wrappers.py | 8 ++ tests/core/test_type_module_attribute.py | 137 +++++++++++++++++++++++ 4 files changed, 225 insertions(+), 10 deletions(-) diff --git a/docs/changes.rst b/docs/changes.rst index d407794d..ec1acd61 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -6,6 +6,25 @@ Version 2.4.2 **Bugs Fixed** +* Deleting the ``__module__`` or ``__doc__`` attribute of a proxy object, + using ``del proxy.__doc__`` for example, crashed the Python interpreter + when the C extension was in use. The attribute setters, which CPython + also invokes for a deletion with no value, correctly forwarded the + deletion to the wrapped object but then attempted to store the missing + value in the proxy's own dictionary. The pure Python implementation did + not crash, but instead failed with an ``AttributeError`` as the + properties used for these attributes had no deleter, so the deletion was + never forwarded to the wrapped object at all. + + Both implementations now forward the deletion to the wrapped object, so + the outcome is the same as deleting the attribute on the wrapped object + directly. For a function this leaves the attribute with a value of + ``None``, while for a class the ``TypeError`` raised by Python is + propagated. The C extension also refreshes the copy of the attribute it + holds in the proxy's own dictionary, so that the state of the proxy after + the deletion is the same as for a proxy newly created over the wrapped + object. + Version 2.4.1 ------------- diff --git a/src/wrapt/_wrappers.c b/src/wrapt/_wrappers.c index 0cd0b81f..890c8f45 100644 --- a/src/wrapt/_wrappers.c +++ b/src/wrapt/_wrappers.c @@ -3033,9 +3033,54 @@ static PyObject *WraptObjectProxy_get_module(WraptObjectProxyObject *self) /* ------------------------------------------------------------------------- */ +/* Refresh the copy of __module__ or __doc__ held in the proxy's own dict + * after the attribute has been deleted from the wrapped object. Deleting + * __doc__ from a function, for example, leaves the attribute present with + * a value of None, whereas on other objects it may be gone entirely. Record + * what __init__ would have for a fresh proxy over the wrapped object in its + * current state: the value if the attribute still exists, otherwise no + * entry at all. */ + +static int WraptObjectProxy_refresh_cached_attr(WraptObjectProxyObject *self, + PyObject *wrapped, + PyObject *name) +{ + PyObject *object = PyObject_GetAttr(wrapped, name); + + if (object) + { + int result = PyDict_SetItem(self->dict, name, object); + Py_DECREF(object); + return result; + } + + if (!PyErr_ExceptionMatches(PyExc_AttributeError)) + return -1; + + PyErr_Clear(); + + if (PyDict_DelItem(self->dict, name) == -1) + { + if (!PyErr_ExceptionMatches(PyExc_KeyError)) + return -1; + PyErr_Clear(); + } + + return 0; +} + +/* ------------------------------------------------------------------------- */ + +/* The setters for __module__ and __doc__ are called with a NULL value when + * the attribute is being deleted. PyObject_SetAttr() forwards a deletion to + * the wrapped object, but the copy held in the proxy's own dict must then be + * refreshed rather than stored, as the dict API rejects a NULL value. */ + static int WraptObjectProxy_set_module(WraptObjectProxyObject *self, PyObject *value) { + int result; + if (!self->wrapped) { if (raise_uninitialized_wrapper_error(self) == -1) @@ -3049,14 +3094,16 @@ static int WraptObjectProxy_set_module(WraptObjectProxyObject *self, PyObject *wrapped = wrapt_acquire_wrapped(self); if (PyObject_SetAttr(wrapped, state->str_module, value) == -1) - { - Py_DECREF(wrapped); - return -1; - } + result = -1; + else if (value) + result = PyDict_SetItem(self->dict, state->str_module, value); + else + result = WraptObjectProxy_refresh_cached_attr(self, wrapped, + state->str_module); Py_DECREF(wrapped); - return PyDict_SetItemString(self->dict, "__module__", value); + return result; } /* ------------------------------------------------------------------------- */ @@ -3087,6 +3134,8 @@ static PyObject *WraptObjectProxy_get_doc(WraptObjectProxyObject *self) static int WraptObjectProxy_set_doc(WraptObjectProxyObject *self, PyObject *value) { + int result; + if (!self->wrapped) { if (raise_uninitialized_wrapper_error(self) == -1) @@ -3100,14 +3149,16 @@ static int WraptObjectProxy_set_doc(WraptObjectProxyObject *self, PyObject *wrapped = wrapt_acquire_wrapped(self); if (PyObject_SetAttr(wrapped, state->str_doc, value) == -1) - { - Py_DECREF(wrapped); - return -1; - } + result = -1; + else if (value) + result = PyDict_SetItem(self->dict, state->str_doc, value); + else + result = WraptObjectProxy_refresh_cached_attr(self, wrapped, + state->str_doc); Py_DECREF(wrapped); - return PyDict_SetItemString(self->dict, "__doc__", value); + return result; } /* ------------------------------------------------------------------------- */ diff --git a/src/wrapt/wrappers.py b/src/wrapt/wrappers.py index e489a22b..10498576 100644 --- a/src/wrapt/wrappers.py +++ b/src/wrapt/wrappers.py @@ -36,6 +36,10 @@ def __module__(self): def __module__(self, value): self.__wrapped__.__module__ = value + @__module__.deleter + def __module__(self): + del self.__wrapped__.__module__ + @property def __doc__(self): return self.__wrapped__.__doc__ @@ -44,6 +48,10 @@ def __doc__(self): def __doc__(self, value): self.__wrapped__.__doc__ = value + @__doc__.deleter + def __doc__(self): + del self.__wrapped__.__doc__ + # We similar use a property for __dict__. We need __dict__ to be # explicit to ensure that vars() works as expected. diff --git a/tests/core/test_type_module_attribute.py b/tests/core/test_type_module_attribute.py index fde9ee6a..06cadb74 100644 --- a/tests/core/test_type_module_attribute.py +++ b/tests/core/test_type_module_attribute.py @@ -272,6 +272,129 @@ class MyProxy(wrapt.ObjectProxy): self.assertEqual(target.__module__, "override_module") +# -- Deleting __module__ and __doc__ tests -- + + +class TestDeleteModuleAndDoc(unittest.TestCase): + """Verify deleting __module__ and __doc__ on instances is forwarded to + the wrapped object, in the same way as setting them. Targets are + created per test rather than shared, as deletion changes them.""" + + @staticmethod + def make_function(): + def target(): + "target documentation" + pass + return target + + @staticmethod + def make_class(): + class Target: + "target documentation" + return Target + + @staticmethod + def delete_outcome(obj, name): + # Return None if deletion succeeds, else the exception type and + # message, so the outcome via a proxy can be compared with the + # outcome of the same deletion made directly on the target. + try: + delattr(obj, name) + except Exception as e: + return (type(e), str(e)) + return None + + def test_delete_module_on_object_proxy(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_on_object_proxy(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_after_set(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + wrapper.__module__ = "override_module" + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_after_set(self): + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + wrapper.__doc__ = "override doc" + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_on_function_wrapper(self): + def my_wrapper(wrapped, instance, args, kwargs): + return wrapped(*args, **kwargs) + target = self.make_function() + wrapper = wrapt.FunctionWrapper(target, my_wrapper) + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_on_function_wrapper(self): + def my_wrapper(wrapped, instance, args, kwargs): + return wrapped(*args, **kwargs) + target = self.make_function() + wrapper = wrapt.FunctionWrapper(target, my_wrapper) + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_on_user_subclass(self): + class MyProxy(wrapt.ObjectProxy): + pass + target = self.make_function() + wrapper = MyProxy(target) + del wrapper.__module__ + self.assertIsNone(target.__module__) + self.assertIsNone(wrapper.__module__) + + def test_delete_doc_on_user_subclass(self): + class MyProxy(wrapt.ObjectProxy): + pass + target = self.make_function() + wrapper = MyProxy(target) + del wrapper.__doc__ + self.assertIsNone(target.__doc__) + self.assertIsNone(wrapper.__doc__) + + def test_delete_module_on_class_target(self): + # A class does not permit deletion of __module__. The outcome via + # the proxy must match a direct deletion on an equivalent class. + expected = self.delete_outcome(self.make_class(), "__module__") + wrapper = wrapt.ObjectProxy(self.make_class()) + self.assertEqual(self.delete_outcome(wrapper, "__module__"), expected) + + def test_delete_doc_on_class_target(self): + expected = self.delete_outcome(self.make_class(), "__doc__") + wrapper = wrapt.ObjectProxy(self.make_class()) + self.assertEqual(self.delete_outcome(wrapper, "__doc__"), expected) + + def test_proxy_state_matches_fresh_proxy_after_delete(self): + # After deletion the proxy's own instance dictionary must be the + # same as for a proxy newly created over the wrapped object in + # its current state, so the C extension's cached copies of the + # attributes do not go stale or linger. + target = self.make_function() + wrapper = wrapt.ObjectProxy(target) + del wrapper.__module__ + del wrapper.__doc__ + fresh = wrapt.ObjectProxy(target) + self.assertEqual(dict(wrapper.__self_dict__), dict(fresh.__self_dict__)) + + # -- Wrapped replacement tests -- @@ -365,6 +488,20 @@ def function(): self.assertEqual(proxy.__module__, "set.module") self.assertEqual(proxy.__doc__, "set doc") + def test_delete_module_and_doc_via_non_interned_name(self): + def function(): + """original doc""" + + proxy = BaseObjectProxy(function) + + delattr(proxy, self.dynamic("__module__")) + delattr(proxy, self.dynamic("__doc__")) + + self.assertIsNone(function.__module__) + self.assertIsNone(function.__doc__) + self.assertIsNone(proxy.__module__) + self.assertIsNone(proxy.__doc__) + if __name__ == "__main__": unittest.main()