pythongh-118362: Fix thread safety around lookups from the type cache…

… in the face of concurrent mutators (python#118454)

Add _PyType_LookupRef and use incref before setting attribute on type
Makes setting an attribute on a class and signaling type modified atomic
Avoid adding re-entrancy exposing the type cache in an inconsistent state by decrefing after type is updated

Include/cpython/object.h

@@ -275,6 +275,7 @@ typedef struct _heaptypeobject {
 
 PyAPI_FUNC(const char *) _PyType_Name(PyTypeObject *);
 PyAPI_FUNC(PyObject *) _PyType_Lookup(PyTypeObject *, PyObject *);
+PyAPI_FUNC(PyObject *) _PyType_LookupRef(PyTypeObject *, PyObject *);
 PyAPI_FUNC(PyObject *) PyType_GetDict(PyTypeObject *);
 
 PyAPI_FUNC(int) PyObject_Print(PyObject *, FILE *, int);

Include/cpython/pyatomic.h

@@ -484,6 +484,9 @@ _Py_atomic_store_int_release(int *obj, int value);
 static inline int
 _Py_atomic_load_int_acquire(const int *obj);
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value);
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value);
 

Include/cpython/pyatomic_gcc.h

@@ -516,6 +516,10 @@ static inline int
 _Py_atomic_load_int_acquire(const int *obj)
 { return __atomic_load_n(obj, __ATOMIC_ACQUIRE); }
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
+{ __atomic_store_n(obj, value, __ATOMIC_RELEASE); }
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
 { __atomic_store_n(obj, value, __ATOMIC_RELEASE); }

Include/cpython/pyatomic_msc.h

@@ -989,6 +989,19 @@ _Py_atomic_load_int_acquire(const int *obj)
 #endif
 }
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
+{
+#if defined(_M_X64) || defined(_M_IX86)
+    *(uint32_t volatile *)obj = value;
+#elif defined(_M_ARM64)
+    _Py_atomic_ASSERT_ARG_TYPE(unsigned __int32);
+    __stlr32((unsigned __int32 volatile *)obj, (unsigned __int32)value);
+#else
+#  error "no implementation of _Py_atomic_store_uint32_release"
+#endif
+}
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
 {

Include/cpython/pyatomic_std.h

@@ -911,6 +911,14 @@ _Py_atomic_load_int_acquire(const int *obj)
                                 memory_order_acquire);
 }
 
+static inline void
+_Py_atomic_store_uint32_release(uint32_t *obj, uint32_t value)
+{
+    _Py_USING_STD;
+    atomic_store_explicit((_Atomic(uint32_t)*)obj, value,
+                          memory_order_release);
+}
+
 static inline void
 _Py_atomic_store_uint64_release(uint64_t *obj, uint64_t value)
 {

Include/internal/pycore_dict.h

@@ -106,6 +106,9 @@ PyAPI_FUNC(PyObject *)_PyDict_LoadGlobal(PyDictObject *, PyDictObject *, PyObjec
 /* Consumes references to key and value */
 PyAPI_FUNC(int) _PyDict_SetItem_Take2(PyDictObject *op, PyObject *key, PyObject *value);
 extern int _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, PyObject *name, PyObject *value);
+extern int _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value);
+extern int _PyDict_GetItemRef_Unicode_LockHeld(PyDictObject *op, PyObject *key, PyObject **result);
+extern int _PyDict_GetItemRef_KnownHash(PyDictObject *op, PyObject *key, Py_hash_t hash, PyObject **result);
 
 extern int _PyDict_Pop_KnownHash(
     PyDictObject *dict,

Include/internal/pycore_object.h

@@ -658,6 +658,7 @@ extern PyObject *_PyType_NewManagedObject(PyTypeObject *type);
 extern PyTypeObject* _PyType_CalculateMetaclass(PyTypeObject *, PyObject *);
 extern PyObject* _PyType_GetDocFromInternalDoc(const char *, const char *);
 extern PyObject* _PyType_GetTextSignatureFromInternalDoc(const char *, const char *, int);
+extern int _PyObject_SetAttributeErrorContext(PyObject *v, PyObject* name);
 
 void _PyObject_InitInlineValues(PyObject *obj, PyTypeObject *tp);
 extern int _PyObject_StoreInstanceAttribute(PyObject *obj,

Lib/test/test_class.py

@@ -882,6 +882,25 @@ class Foo:
         f.a = 3
         self.assertEqual(f.a, 3)
 
+    def test_store_attr_type_cache(self):
+        """Verifies that the type cache doesn't provide a value which  is
+        inconsistent from the dict."""
+        class X:
+            def __del__(inner_self):
+                v = C.a
+                self.assertEqual(v, C.__dict__['a'])
+
+        class C:
+            a = X()
+
+        # prime the cache
+        C.a
+        C.a
+
+        # destructor shouldn't be able to see inconsisent state
+        C.a = X()
+        C.a = X()
+
 
 if __name__ == '__main__':
     unittest.main()

Lib/test/test_free_threading/test_type.py

@@ -0,0 +1,112 @@
+import unittest
+
+from threading import Thread
+from multiprocessing.dummy import Pool
+from unittest import TestCase
+
+from test.support import is_wasi
+
+
+NTHREADS = 6
+BOTTOM = 0
+TOP = 1000
+ITERS = 100
+
+class A:
+    attr = 1
+
+@unittest.skipIf(is_wasi, "WASI has no threads.")
+class TestType(TestCase):
+    def test_attr_cache(self):
+        def read(id0):
+            for _ in range(ITERS):
+                for _ in range(BOTTOM, TOP):
+                    A.attr
+
+        def write(id0):
+            for _ in range(ITERS):
+                for _ in range(BOTTOM, TOP):
+                    # Make _PyType_Lookup cache hot first
+                    A.attr
+                    A.attr
+                    x = A.attr
+                    x += 1
+                    A.attr = x
+
+
+        with Pool(NTHREADS) as pool:
+            pool.apply_async(read, (1,))
+            pool.apply_async(write, (1,))
+            pool.close()
+            pool.join()
+
+    def test_attr_cache_consistency(self):
+        class C:
+            x = 0
+
+        DONE = False
+        def writer_func():
+            for i in range(3000):
+                C.x
+                C.x
+                C.x += 1
+            nonlocal DONE
+            DONE = True
+
+        def reader_func():
+            while True:
+                # We should always see a greater value read from the type than the
+                # dictionary
+                a = C.__dict__['x']
+                b = C.x
+                self.assertGreaterEqual(b, a)
+
+                if DONE:
+                    break
+
+        self.run_one(writer_func, reader_func)
+
+    def test_attr_cache_consistency_subclass(self):
+        class C:
+            x = 0
+
+        class D(C):
+            pass
+
+        DONE = False
+        def writer_func():
+            for i in range(3000):
+                D.x
+                D.x
+                C.x += 1
+            nonlocal DONE
+            DONE = True
+
+        def reader_func():
+            while True:
+                # We should always see a greater value read from the type than the
+                # dictionary
+                a = C.__dict__['x']
+                b = D.x
+                self.assertGreaterEqual(b, a)
+
+                if DONE:
+                    break
+
+        self.run_one(writer_func, reader_func)
+
+    def run_one(self, writer_func, reader_func):
+        writer = Thread(target=writer_func)
+        readers = []
+        for x in range(30):
+            reader = Thread(target=reader_func)
+            readers.append(reader)
+            reader.start()
+
+        writer.start()
+        writer.join()
+        for reader in readers:
+            reader.join()
+
+if __name__ == "__main__":
+    unittest.main()

Misc/NEWS.d/next/Core and Builtins/2024-05-01-14-20-28.gh-issue-118492.VUsSfn.rst

@@ -0,0 +1,2 @@
+Fix an issue where the type cache can expose a previously accessed attribute
+when a finalizer is run.

Modules/_collectionsmodule.c

@@ -2511,9 +2511,9 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
     /* Only take the fast path when get() and __setitem__()
      * have not been overridden.
      */
-    mapping_get = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(get));
+    mapping_get = _PyType_LookupRef(Py_TYPE(mapping), &_Py_ID(get));
     dict_get = _PyType_Lookup(&PyDict_Type, &_Py_ID(get));
-    mapping_setitem = _PyType_Lookup(Py_TYPE(mapping), &_Py_ID(__setitem__));
+    mapping_setitem = _PyType_LookupRef(Py_TYPE(mapping), &_Py_ID(__setitem__));
     dict_setitem = _PyType_Lookup(&PyDict_Type, &_Py_ID(__setitem__));
 
     if (mapping_get != NULL && mapping_get == dict_get &&
@@ -2587,6 +2587,8 @@ _collections__count_elements_impl(PyObject *module, PyObject *mapping,
     }
 
 done:
+    Py_XDECREF(mapping_get);
+    Py_XDECREF(mapping_setitem);
     Py_DECREF(it);
     Py_XDECREF(key);
     Py_XDECREF(newval);

Modules/_ctypes/_ctypes.c

@@ -1096,7 +1096,7 @@ static int
 UnionType_setattro(PyObject *self, PyObject *key, PyObject *value)
 {
     /* XXX Should we disallow deleting _fields_? */
-    if (-1 == PyObject_GenericSetAttr(self, key, value))
+    if (-1 == PyType_Type.tp_setattro(self, key, value))
         return -1;
 
     if (PyUnicode_Check(key) &&

Modules/_lsprof.c

@@ -177,8 +177,7 @@ normalizeUserObj(PyObject *obj)
         PyObject *modname = fn->m_module;
 
         if (name != NULL) {
-            PyObject *mo = _PyType_Lookup(Py_TYPE(self), name);
-            Py_XINCREF(mo);
+            PyObject *mo = _PyType_LookupRef(Py_TYPE(self), name);
             Py_DECREF(name);
             if (mo != NULL) {
                 PyObject *res = PyObject_Repr(mo);

Modules/_testcapi/gc.c

@@ -99,10 +99,11 @@ slot_tp_del(PyObject *self)
         return;
     }
     /* Execute __del__ method, if any. */
-    del = _PyType_Lookup(Py_TYPE(self), tp_del);
+    del = _PyType_LookupRef(Py_TYPE(self), tp_del);
     Py_DECREF(tp_del);
     if (del != NULL) {
         res = PyObject_CallOneArg(del, self);
+        Py_DECREF(del);
         if (res == NULL)
             PyErr_WriteUnraisable(del);
         else

Objects/classobject.c

@@ -188,15 +188,18 @@ method_getattro(PyObject *obj, PyObject *name)
             if (PyType_Ready(tp) < 0)
                 return NULL;
         }
-        descr = _PyType_Lookup(tp, name);
+        descr = _PyType_LookupRef(tp, name);
     }
 
     if (descr != NULL) {
         descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
-        if (f != NULL)
-            return f(descr, obj, (PyObject *)Py_TYPE(obj));
+        if (f != NULL) {
+            PyObject *res = f(descr, obj, (PyObject *)Py_TYPE(obj));
+            Py_DECREF(descr);
+            return res;
+        }
         else {
-            return Py_NewRef(descr);
+            return descr;
         }
     }
 
@@ -410,14 +413,17 @@ instancemethod_getattro(PyObject *self, PyObject *name)
         if (PyType_Ready(tp) < 0)
             return NULL;
     }
-    descr = _PyType_Lookup(tp, name);
+    descr = _PyType_LookupRef(tp, name);
 
     if (descr != NULL) {
         descrgetfunc f = TP_DESCR_GET(Py_TYPE(descr));
-        if (f != NULL)
-            return f(descr, self, (PyObject *)Py_TYPE(self));
+        if (f != NULL) {
+            PyObject *res = f(descr, self, (PyObject *)Py_TYPE(self));
+            Py_DECREF(descr);
+            return res;
+        }
         else {
-            return Py_NewRef(descr);
+            return descr;
         }
     }