[3.15] gh-155400: Fix deadlock with type_lock_prevent_release() (GH-155401) (#155822)

Co-authored-by: Neil Schemenauer <nas-github@arctrix.com>
Co-authored-by: Kumar Aditya <kumaraditya@python.org>

Author: 3 people

Lib/test/test_free_threading/test_type.py

@@ -181,5 +181,63 @@ def wrapper():
         for reader in readers:
             reader.join()
 
+    def test_concurrent_setattr_deadlock(self):
+        # gh-155400: two threads assigning to a special method of the same
+        # class could deadlock.  One thread held the type lock and waited for
+        # the type dict mutex, which its critical section had released when it
+        # blocked on the stop-the-world mutex, while the other held the type
+        # dict mutex and waited for the type lock.
+        # This is fairly difficult to trigger the race but this N seems to do
+        # it at least sometimes.
+        N = 200
+        done = False
+
+        class Base:
+            pass
+
+        def setter():
+            func = lambda self: "x"
+            barrier.wait()
+            while not done:
+                Base.__repr__ = func
+                try:
+                    del Base.__repr__
+                except AttributeError:
+                    pass
+
+        def subclasser():
+            barrier.wait()
+            while not done:
+                type('Sub', (Base,), {})()
+
+        def lister():
+            barrier.wait()
+            while not done:
+                Base.__subclasses__()
+
+        def basesetter():
+            nonlocal done
+            barrier.wait()
+            for _ in range(N):
+                class A:
+                    pass
+                class C:
+                    pass
+                class B(A):
+                    pass
+                B.__bases__ = (C,)
+            done = True
+
+        # The setter threads are the ones that deadlock.  The others are there
+        # to keep the type lock and the stop-the-world mutex contended, which
+        # is what gets the setters into the window where it happens.
+        targets = (setter, setter, subclasser, subclasser,
+                   lister, lister, basesetter)
+        barrier = threading.Barrier(len(targets))
+        threads = [Thread(target=target) for target in targets]
+        with threading_helper.start_threads(threads):
+            pass
+
+
 if __name__ == "__main__":
     unittest.main()

Misc/NEWS.d/next/Core_and_Builtins/2026-08-07-18-32-15.gh-issue-155400.Kq3Vpt.rst

@@ -0,0 +1,5 @@
+Fix a deadlock in the free-threaded build between two threads assigning to a
+special method of the same class.  While applying the type slot updates with
+the world stopped, only the type lock was prevented from being released; the
+type dict mutex could still be released and re-acquired, in the wrong order,
+if the thread blocked on the stop-the-world mutex.

Objects/typeobject.c

@@ -60,6 +60,13 @@ class object "PyObject *" "&PyBaseObject_Type"
 #define NEXT_VERSION_TAG(interp) \
     (interp)->types.next_version_tag
 
+// Storage for the mutexes saved by type_lock_prevent_release().  Defined for
+// both builds so the call sites don't need to be conditionally compiled.
+typedef struct {
+    PyMutex *mutex1;
+    PyMutex *mutex2;
+} pinned_mutexes_t;
+
 #ifdef Py_GIL_DISABLED
 
 // There's a global lock for types that ensures that tp_version_tag and
@@ -138,44 +145,54 @@ types_start_world(void)
     assert(!types_world_is_stopped());
 }
 
-// This is used to temporarily prevent the TYPE_LOCK from being suspended
-// when held by the topmost critical section.
+// Temporarily prevent the mutexes held by the topmost critical section from
+// being released when the current thread blocks (blocking detaches the thread,
+// which suspends its critical sections and releases the mutexes they hold).
+//
+// All of the mutexes held by the critical section are pinned, not just
+// TYPE_LOCK.  If only TYPE_LOCK was pinned then _PyCriticalSection_Resume()
+// would have to re-acquire the other mutex while TYPE_LOCK is held.  That
+// deadlocks against a thread that holds that mutex and is waiting for
+// TYPE_LOCK, which is exactly what BEGIN_TYPE_DICT_LOCK() does: the type dict
+// mutex is on the heap and TYPE_LOCK is in _PyRuntime, so the address ordering
+// used by two-mutex critical sections usually acquires the dict mutex first.
+// By pinning both mutexes there is nothing to re-acquire on resume.
+//
+// Holding the mutexes while blocked does not prevent the world from being
+// stopped: a thread waiting on either of them parks with _PY_LOCK_DETACH and
+// so is detached while it waits.
 static void
-type_lock_prevent_release(void)
+type_lock_prevent_release(pinned_mutexes_t *pinned)
 {
     PyThreadState *tstate = _PyThreadState_GET();
-    uintptr_t *tagptr = &tstate->critical_section;
-    PyCriticalSection *c = (PyCriticalSection *)(*tagptr & ~_Py_CRITICAL_SECTION_MASK);
-    if (!(*tagptr & _Py_CRITICAL_SECTION_TWO_MUTEXES)) {
-        assert(c->_cs_mutex == TYPE_LOCK);
-        c->_cs_mutex = NULL;
-    }
-    else {
+    uintptr_t tag = tstate->critical_section;
+    PyCriticalSection *c = (PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK);
+    pinned->mutex1 = c->_cs_mutex;
+    pinned->mutex2 = NULL;
+    c->_cs_mutex = NULL;
+    if ((tag & _Py_CRITICAL_SECTION_TWO_MUTEXES) != 0) {
         PyCriticalSection2 *c2 = (PyCriticalSection2 *)c;
-        if (c->_cs_mutex == TYPE_LOCK) {
-            c->_cs_mutex = c2->_cs_mutex2;
-            c2->_cs_mutex2 = NULL;
-        } else {
-            assert(c2->_cs_mutex2 == TYPE_LOCK);
-            c2->_cs_mutex2 = NULL;
-        }
+        pinned->mutex2 = c2->_cs_mutex2;
+        c2->_cs_mutex2 = NULL;
     }
+    assert(pinned->mutex1 == TYPE_LOCK || pinned->mutex2 == TYPE_LOCK);
 }
 
 static void
-type_lock_allow_release(void)
+type_lock_allow_release(pinned_mutexes_t *pinned)
 {
     PyThreadState *tstate = _PyThreadState_GET();
-    uintptr_t *tagptr = &tstate->critical_section;
-    PyCriticalSection *c = (PyCriticalSection *)(*tagptr & ~_Py_CRITICAL_SECTION_MASK);
-    if (!(*tagptr & _Py_CRITICAL_SECTION_TWO_MUTEXES)) {
-        assert(c->_cs_mutex == NULL);
-        c->_cs_mutex = TYPE_LOCK;
-    }
-    else {
+    uintptr_t tag = tstate->critical_section;
+    PyCriticalSection *c = (PyCriticalSection *)(tag & ~_Py_CRITICAL_SECTION_MASK);
+    assert(c->_cs_mutex == NULL);
+    c->_cs_mutex = pinned->mutex1;
+    if ((tag & _Py_CRITICAL_SECTION_TWO_MUTEXES) != 0) {
         PyCriticalSection2 *c2 = (PyCriticalSection2 *)c;
         assert(c2->_cs_mutex2 == NULL);
-        c2->_cs_mutex2 = TYPE_LOCK;
+        c2->_cs_mutex2 = pinned->mutex2;
+    }
+    else {
+        assert(pinned->mutex2 == NULL);
     }
 }
 
@@ -192,8 +209,8 @@ type_lock_allow_release(void)
 #define types_world_is_stopped() 1
 #define types_stop_world()
 #define types_start_world()
-#define type_lock_prevent_release()
-#define type_lock_allow_release()
+#define type_lock_prevent_release(pinned) ((void)(pinned))
+#define type_lock_allow_release(pinned) ((void)(pinned))
 
 #endif
 
@@ -664,14 +681,15 @@ set_tp_mro(PyTypeObject *self, PyObject *mro, int initial)
             PyUnstable_Object_EnableDeferredRefcount(mro);
         }
     }
+    pinned_mutexes_t pinned;
     if (!initial) {
-        type_lock_prevent_release();
+        type_lock_prevent_release(&pinned);
         types_stop_world();
     }
     self->tp_mro = mro;
     if (!initial) {
         types_start_world();
-        type_lock_allow_release();
+        type_lock_allow_release(&pinned);
     }
 }
 
@@ -1952,13 +1970,14 @@ type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, PyTypeObject *b
     PyObject *old_bases = lookup_tp_bases(type);
     assert(old_bases != NULL);
     PyTypeObject *old_base = type->tp_base;
+    pinned_mutexes_t pinned;
 
-    type_lock_prevent_release();
+    type_lock_prevent_release(&pinned);
     types_stop_world();
     set_tp_bases(type, Py_NewRef(new_bases), 0);
     type->tp_base = (PyTypeObject *)Py_NewRef(best_base);
     types_start_world();
-    type_lock_allow_release();
+    type_lock_allow_release(&pinned);
 
     PyObject *temp = PyList_New(0);
     if (temp == NULL) {
@@ -2019,12 +2038,12 @@ type_set_bases_unlocked(PyTypeObject *type, PyObject *new_bases, PyTypeObject *b
     if (lookup_tp_bases(type) == new_bases) {
         assert(type->tp_base == best_base);
 
-        type_lock_prevent_release();
+        type_lock_prevent_release(&pinned);
         types_stop_world();
         set_tp_bases(type, old_bases, 0);
         type->tp_base = old_base;
         types_start_world();
-        type_lock_allow_release();
+        type_lock_allow_release(&pinned);
 
         Py_DECREF(new_bases);
         Py_DECREF(best_base);
@@ -3932,16 +3951,22 @@ apply_type_slot_updates(slot_update_t *updates)
     // to update the dict.  That's because TYPE_LOCK was acquired using a
     // critical section.
     //
-    // The type_lock_prevent_release() call prevents the TYPE_LOCK mutex from
-    // being released even if we block on the STM mutex.  We need to take care
-    // that we do not deadlock because of that.  It is safe because we always
-    // acquire locks in the same order: first the TYPE_LOCK mutex and then the
-    // STM mutex.
-    type_lock_prevent_release();
+    // The type_lock_prevent_release() call prevents the mutexes held by the
+    // critical section (TYPE_LOCK and the type dict mutex) from being released
+    // even if we block on the STW mutex.  We need to take care that we do not
+    // deadlock because of that.  It is safe because a thread waiting for either
+    // of those mutexes detaches while it waits and so does not hold up the
+    // stop-the-world.  Pinning both mutexes rather than only TYPE_LOCK is what
+    // makes this safe: otherwise the dict mutex would be released when we
+    // block and _PyCriticalSection_Resume() would have to re-acquire it while
+    // holding TYPE_LOCK, deadlocking with a thread that holds the dict mutex
+    // and is waiting for TYPE_LOCK.
+    pinned_mutexes_t pinned;
+    type_lock_prevent_release(&pinned);
     types_stop_world();
     apply_slot_updates(updates);
     types_start_world();
-    type_lock_allow_release();
+    type_lock_allow_release(&pinned);
 }
 
 #else
@@ -6526,11 +6551,12 @@ _PyType_SetFlagsRecursive(PyTypeObject *self, unsigned long mask, unsigned long
     }
     /* Keep TYPE_LOCK held while waiting for stop-the-world so no thread
        can reassign a version tag before the flag update. */
-    type_lock_prevent_release();
+    pinned_mutexes_t pinned;
+    type_lock_prevent_release(&pinned);
     types_stop_world();
     set_flags_recursive(self, mask, flags);
     types_start_world();
-    type_lock_allow_release();
+    type_lock_allow_release(&pinned);
     END_TYPE_LOCK();
 }