gh-127266: avoid data races when updating type slots

Include/internal/pycore_object.h

@@ -311,7 +311,7 @@ extern int _PyDict_CheckConsistency(PyObject *mp, int check_content);
 // Fast inlined version of PyType_HasFeature()
 static inline int
 _PyType_HasFeature(PyTypeObject *type, unsigned long feature) {
-    return ((FT_ATOMIC_LOAD_ULONG_RELAXED(type->tp_flags) & feature) != 0);
+    return ((type->tp_flags) & feature) != 0;
 }
 
 extern void _PyType_InitCache(PyInterpreterState *interp);

Include/object.h

@@ -776,11 +776,7 @@ PyType_HasFeature(PyTypeObject *type, unsigned long feature)
     // PyTypeObject is opaque in the limited C API
     flags = PyType_GetFlags(type);
 #else
-#   ifdef Py_GIL_DISABLED
-        flags = _Py_atomic_load_ulong_relaxed(&type->tp_flags);
-#   else
-        flags = type->tp_flags;
-#   endif
+    flags = type->tp_flags;
 #endif
     return ((flags & feature) != 0);
 }

Misc/NEWS.d/next/Core_and_Builtins/2025-03-14-13-08-20.gh-issue-127266._tyfBp.rst

@@ -0,0 +1,6 @@
+In the free-threaded build, avoid data races caused by updating type slots
+or type flags after the type was initially created.  For those (typically
+rare) cases, use the stop-the-world mechanism.  Remove the use of atomics
+when reading or writing type flags.  The use of atomics is not sufficient to
+avoid races (since flags are sometimes read without a lock and without
+atomics) and are no longer required.

Objects/typeobject.c

@@ -84,6 +84,17 @@ class object "PyObject *" "&PyBaseObject_Type"
 
 #endif
 
+// Modification of type slots and type flags is protected by the global type
+// lock. However, the slots and flags are read non-atomically without holding
+// the type lock.  So, we need to stop-the-world while modifying these, in
+// order to avoid data races.  This is unfortunately a bit expensive.
+#ifdef Py_GIL_DISABLED
+// If defined, type slot updates (after initial creation) will stop-the-world.
+#define TYPE_SLOT_UPDATE_NEEDS_STOP
+// If defined, type flag updates (after initial creation) will stop-the-world.
+#define TYPE_FLAGS_UPDATE_NEEDS_STOP
+#endif
+
 #define PyTypeObject_CAST(op)   ((PyTypeObject *)(op))
 
 typedef struct PySlot_Offset {
@@ -353,9 +364,7 @@ type_set_flags(PyTypeObject *tp, unsigned long flags)
         // held when flags are modified.
         ASSERT_TYPE_LOCK_HELD();
     }
-    // Since PyType_HasFeature() reads the flags without holding the type
-    // lock, we need an atomic store here.
-    FT_ATOMIC_STORE_ULONG_RELAXED(tp->tp_flags, flags);
+    tp->tp_flags = flags;
 }
 
 static void
@@ -1584,9 +1593,10 @@ type_set_abstractmethods(PyObject *tp, PyObject *value, void *Py_UNUSED(closure)
     BEGIN_TYPE_LOCK();
     type_modified_unlocked(type);
     if (abstract)
-        type_add_flags(type, Py_TPFLAGS_IS_ABSTRACT);
+        _PyType_SetFlags(type, 0, Py_TPFLAGS_IS_ABSTRACT);
     else
-        type_clear_flags(type, Py_TPFLAGS_IS_ABSTRACT);
+        _PyType_SetFlags(type, Py_TPFLAGS_IS_ABSTRACT, 0);
+
     END_TYPE_LOCK();
 
     return 0;
@@ -3490,6 +3500,9 @@ static int update_slot(PyTypeObject *, PyObject *);
 static void fixup_slot_dispatchers(PyTypeObject *);
 static int type_new_set_names(PyTypeObject *);
 static int type_new_init_subclass(PyTypeObject *, PyObject *);
+#ifdef Py_GIL_DISABLED
+static bool has_slotdef(PyObject *);
+#endif
 
 /*
  * Helpers for  __dict__ descriptor.  We don't want to expose the dicts
@@ -3687,7 +3700,7 @@ type_init(PyObject *cls, PyObject *args, PyObject *kwds)
 unsigned long
 PyType_GetFlags(PyTypeObject *type)
 {
-    return FT_ATOMIC_LOAD_ULONG_RELAXED(type->tp_flags);
+    return type->tp_flags;
 }
 
 
@@ -5779,7 +5792,17 @@ void
 _PyType_SetFlags(PyTypeObject *self, unsigned long mask, unsigned long flags)
 {
     BEGIN_TYPE_LOCK();
-    type_set_flags_with_mask(self, mask, flags);
+    unsigned long new_flags = (self->tp_flags & ~mask) | flags;
+    if (new_flags != self->tp_flags) {
+#ifdef TYPE_FLAGS_UPDATE_NEEDS_STOP
+        PyInterpreterState *interp = _PyInterpreterState_GET();
+        _PyEval_StopTheWorld(interp);
+#endif
+        self->tp_flags = new_flags;
+#ifdef TYPE_FLAGS_UPDATE_NEEDS_STOP
+        _PyEval_StartTheWorld(interp);
+#endif
+    }
     END_TYPE_LOCK();
 }
 
@@ -5943,6 +5966,21 @@ _Py_type_getattro(PyObject *tp, PyObject *name)
     return _Py_type_getattro_impl(type, name, NULL);
 }
 
+#ifdef TYPE_SLOT_UPDATE_NEEDS_STOP
+static int
+update_slot_world_stopped(PyTypeObject *type, PyObject *name)
+{
+    int ret;
+    PyInterpreterState *interp = _PyInterpreterState_GET();
+    _PyEval_StopTheWorld(interp);
+    ret = update_slot(type, name);
+    _PyEval_StartTheWorld(interp);
+    return ret;
+}
+#endif
+
+// Called by type_setattro().  Updates both the type dict and
+// any type slots that correspond to the modified entry.
 static int
 type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name,
                  PyObject *value, PyObject **old_value)
@@ -5972,9 +6010,15 @@ type_update_dict(PyTypeObject *type, PyDictObject *dict, PyObject *name,
         return -1;
     }
 
+#ifdef TYPE_SLOT_UPDATE_NEEDS_STOP
+    if (is_dunder_name(name) && has_slotdef(name)) {
+        return update_slot_world_stopped(type, name);
+    }
+#else
     if (is_dunder_name(name)) {
         return update_slot(type, name);
     }
+#endif
 
     return 0;
 }
@@ -11002,6 +11046,20 @@ resolve_slotdups(PyTypeObject *type, PyObject *name)
 #undef ptrs
 }
 
+#ifdef TYPE_SLOT_UPDATE_NEEDS_STOP
+// Return true if "name" corresponds to at least one slot definition.  This is
+// a more accurate but more expensive test compared to is_dunder_name().
+static bool
+has_slotdef(PyObject *name)
+{
+    for (pytype_slotdef *p = slotdefs; p->name_strobj; p++) {
+        if (p->name_strobj == name) {
+            return true;
+        }
+    }
+    return false;
+}
+#endif
 
 /* Common code for update_slots_callback() and fixup_slot_dispatchers().
  *
@@ -11241,20 +11299,33 @@ fixup_slot_dispatchers(PyTypeObject *type)
     END_TYPE_LOCK();
 }
 
+// Called when __bases__ is re-assigned.
 static void
 update_all_slots(PyTypeObject* type)
 {
     pytype_slotdef *p;
 
     ASSERT_TYPE_LOCK_HELD();
 
+#ifdef TYPE_SLOT_UPDATE_NEEDS_STOP
+    // Similar to update_slot_world_stopped(), this is required to avoid
+    // races.  We don't use update_slot_world_stopped() here because we want
+    // to stop once rather than once per slot.
+    PyInterpreterState *interp = _PyInterpreterState_GET();
+    _PyEval_StopTheWorld(interp);
+#endif
+
     /* Clear the VALID_VERSION flag of 'type' and all its subclasses. */
     type_modified_unlocked(type);
 
     for (p = slotdefs; p->name; p++) {
         /* update_slot returns int but can't actually fail */
         update_slot(type, p->name_strobj);
     }
+
+#ifdef TYPE_SLOT_UPDATE_NEEDS_STOP
+    _PyEval_StartTheWorld(interp);
+#endif
 }
 
 

Tools/tsan/suppressions_free_threading.txt

@@ -14,15 +14,13 @@
 
 race_top:assign_version_tag
 race_top:_multiprocessing_SemLock_acquire_impl
-race_top:_Py_slot_tp_getattr_hook
 race_top:dump_traceback
 race_top:fatal_error
 race_top:_multiprocessing_SemLock_release_impl
 race_top:_PyFrame_GetCode
 race_top:_PyFrame_Initialize
 race_top:_PyObject_TryGetInstanceAttribute
 race_top:PyUnstable_InterpreterFrame_GetLine
-race_top:type_modified_unlocked
 race_top:write_thread_id
 
 # gh-129068: race on shared range iterators (test_free_threading.test_zip.ZipThreading.test_threading)
@@ -31,9 +29,6 @@ race_top:rangeiter_next
 # gh-129748: test.test_free_threading.test_slots.TestSlots.test_object
 race_top:mi_block_set_nextx
 
-# gh-127266: type slot updates are not thread-safe (test_opcache.test_load_attr_method_lazy_dict)
-race_top:update_one_slot
-
 # https://gist.github.com/mpage/6962e8870606cfc960e159b407a0cb40
 thread:pthread_create