gh-118362: Fix thread safety around lookups from the type cache in the face of concurrent mutators (#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

Author: DinoV

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.