Compare intercepted attribute names by value, not only by identity.

The C extension intercepts __module__ and __doc__ in the attribute get
and set slots of the base object proxy, and __signature__ in the get slot
of PartialCallableObjectProxy, by comparing the name against an interned
string held in module state. That comparison was by identity only, so it
relied on the incoming name having been interned. Names originating from
Python source code always are, but a name constructed at runtime, for
example by string concatenation or decoding, is not, and for such a name
the interception was silently skipped. Getting __module__ or __doc__ then
returned the copy captured in the instance dict when the proxy was
created rather than the current value on the wrapped object, setting them
stored the value on the proxy instead of the wrapped object, and
__signature__ was forwarded to the wrapped callable.

Add a wrapt_name_equals() helper which keeps the identity comparison as
the fast path and falls back to a value comparison, guarded by a length
check so that the cost for non matching names is a single integer
comparison, following the pattern CPython uses in super_getattro(). Use
it at all three sites and add tests exercising each attribute with a
non interned name.

Author: GrahamDumpleton

docs/changes.rst

@@ -38,6 +38,19 @@ Version 2.4.1
   bound method is still reported incorrectly, for reasons outside of the
   control of ``wrapt``. See the "Known Issues" documentation for details.
 
+* The C extension intercepts the ``__module__`` and ``__doc__`` attributes
+  by name in its attribute get and set slots so that they are forwarded to
+  the wrapped object. The name was compared by identity against an interned
+  string, which relied on the attribute name having been interned. Names
+  originating from Python source code always are, but a name constructed at
+  runtime, for example by string concatenation or by decoding, is not, and
+  for such a name the interception was skipped. Getting the attribute then
+  returned the value captured when the proxy was created rather than the
+  current value on the wrapped object, and setting it stored the value on
+  the proxy rather than the wrapped object. The comparison now falls back to
+  comparing by value when the identity check fails, guarded by a length
+  check so that the cost for non matching names is unchanged.
+
 Version 2.4.0
 -------------
 

src/wrapt/_wrappers.c

@@ -205,6 +205,35 @@ static inline PyObject *wrapt_acquire_field(PyObject *owner, PyObject **field)
   return value;
 }
 
+/* Test whether an attribute name matches one of the interned strings held
+ * in module state. Attribute names arriving from Python code are interned
+ * by the compiler, so a pointer comparison against the interned string is
+ * nearly always sufficient and is the fast path. A name constructed at
+ * runtime, for example by string concatenation or decoding, is not
+ * interned and would never match by pointer, so fall back to a value
+ * comparison. That comparison is guarded by a length check so that the
+ * cost for the overwhelming majority of non matching names is a single
+ * integer comparison. This mirrors the pattern used by CPython itself in
+ * super_getattro() for __class__.
+ *
+ * The name must be a str. This holds for the attribute get and set slots
+ * where this is used, since PyObject_GetAttr() and PyObject_SetAttr()
+ * reject any other type before the slot is reached. */
+
+static inline int wrapt_name_equals(PyObject *name, PyObject *interned)
+{
+  if (name == interned)
+    return 1;
+
+  if (!PyUnicode_Check(name))
+    return 0;
+
+  if (PyUnicode_GET_LENGTH(name) != PyUnicode_GET_LENGTH(interned))
+    return 0;
+
+  return PyUnicode_Compare(name, interned) == 0;
+}
+
 /* Convenience form for the common case of the wrapped object field. */
 
 static inline PyObject *wrapt_acquire_wrapped(WraptObjectProxyObject *self)
@@ -3336,9 +3365,9 @@ static PyObject *WraptObjectProxy_getattro(WraptObjectProxyObject *self,
    * __module__/__doc__ strings that type.__module__ reads via raw dict
    * lookup. */
 
-  if (name == state->str_module)
+  if (wrapt_name_equals(name, state->str_module))
     return WraptObjectProxy_get_module(self);
-  if (name == state->str_doc)
+  if (wrapt_name_equals(name, state->str_doc))
     return WraptObjectProxy_get_doc(self);
 
   object = PyObject_GenericGetAttr((PyObject *)self, name);
@@ -3418,9 +3447,9 @@ static int WraptObjectProxy_setattro(WraptObjectProxyObject *self,
    * values in the type dict (from PyType_FromModuleAndSpec) would cause
    * wrapt_type_has_attr to match and GenericSetAttr to store locally. */
 
-  if (name == state->str_module)
+  if (wrapt_name_equals(name, state->str_module))
     return WraptObjectProxy_set_module(self, value);
-  if (name == state->str_doc)
+  if (wrapt_name_equals(name, state->str_doc))
     return WraptObjectProxy_set_doc(self, value);
 
   if (wrapt_type_has_attr(Py_TYPE(self), name))
@@ -4071,7 +4100,7 @@ static PyObject *WraptPartialCallableObjectProxy_getattro(
   if (!state)
     return NULL;
 
-  if (name == state->str_signature)
+  if (wrapt_name_equals(name, state->str_signature))
     return WraptPartialCallableObjectProxy_get_signature(self);
 
   return WraptObjectProxy_getattro((WraptObjectProxyObject *)self, name);

tests/core/test_callable_object_proxy.py

@@ -205,6 +205,22 @@ def func0(a, b, c=1):
 
         self.assertFalse(hasattr(func0, "__signature__"))
 
+    def test_attribute_via_non_interned_name(self):
+        # The C extension intercepts __signature__ by name in the attribute
+        # get slot. A name constructed at runtime is not interned and must
+        # still be matched, rather than being forwarded to the wrapped
+        # callable which has no such attribute.
+
+        def func0(a, b, c=1):
+            pass
+
+        name = "".join(list("__signature__"))
+
+        self.assertEqual(
+            getattr(wrapt.partial(func0, 1), name),
+            inspect.signature(functools.partial(func0, 1)),
+        )
+
     def test_bound_method_limitation(self):
         # When the wrapped callable is an already bound method, inspect
         # decides based on the class of the object, which the proxy reports

tests/core/test_type_module_attribute.py

@@ -325,5 +325,46 @@ def func2():
         self.assertEqual(wrapper.__module__, "module2")
 
 
+class TestNonInternedAttributeNames(unittest.TestCase):
+    """The C extension intercepts __module__ and __doc__ by name in the
+    attribute get and set slots. Names arriving from Python code are
+    interned, but names constructed at runtime are not, and the interception
+    must work for those too rather than falling through to a stale copy.
+    """
+
+    @staticmethod
+    def dynamic(name):
+        # Build an equal but distinct, non interned string object.
+        result = "".join(list(name))
+        assert result == name and result is not name
+        return result
+
+    def test_get_module_and_doc_via_non_interned_name(self):
+        def function():
+            """original doc"""
+
+        proxy = BaseObjectProxy(function)
+
+        function.__module__ = "changed.module"
+        function.__doc__ = "changed doc"
+
+        self.assertEqual(getattr(proxy, self.dynamic("__module__")), "changed.module")
+        self.assertEqual(getattr(proxy, self.dynamic("__doc__")), "changed doc")
+
+    def test_set_module_and_doc_via_non_interned_name(self):
+        def function():
+            """original doc"""
+
+        proxy = BaseObjectProxy(function)
+
+        setattr(proxy, self.dynamic("__module__"), "set.module")
+        setattr(proxy, self.dynamic("__doc__"), "set doc")
+
+        self.assertEqual(function.__module__, "set.module")
+        self.assertEqual(function.__doc__, "set doc")
+        self.assertEqual(proxy.__module__, "set.module")
+        self.assertEqual(proxy.__doc__, "set doc")
+
+
 if __name__ == "__main__":
     unittest.main()