[3.13] gh-132942: Fix races in type lookup cache (gh-133114)

nascheme · web-flow · commit ca46ec85f8c3 · 2025-04-28T22:08:09.000Z
Two races related to the type lookup cache, when used in the
free-threaded build.  This caused test_opcache to sometimes fail (as
well as other hard to re-produce failures).
diff --git a/Misc/NEWS.d/next/Core and Builtins/2025-04-26-17-50-47.gh-issue-132942.aEEZvZ.rst b/Misc/NEWS.d/next/Core and Builtins/2025-04-26-17-50-47.gh-issue-132942.aEEZvZ.rst
@@ -0,0 +1,2 @@
+Fix two races in the type lookup cache.  This affected the free-threaded
+build and could cause crashes (apparently quite difficult to trigger).
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
@@ -5164,14 +5164,19 @@ is_dunder_name(PyObject *name)
 static PyObject *
 update_cache(struct type_cache_entry *entry, PyObject *name, unsigned int version_tag, PyObject *value)
 {
-    _Py_atomic_store_uint32_relaxed(&entry->version, version_tag);
     _Py_atomic_store_ptr_relaxed(&entry->value, value); /* borrowed */
     assert(_PyASCIIObject_CAST(name)->hash != -1);
     OBJECT_STAT_INC_COND(type_cache_collisions, entry->name != Py_None && entry->name != name);
     // We're releasing this under the lock for simplicity sake because it's always a
     // exact unicode object or Py_None so it's safe to do so.
     PyObject *old_name = entry->name;
     _Py_atomic_store_ptr_relaxed(&entry->name, Py_NewRef(name));
+    // We must write the version last to avoid _Py_TryXGetStackRef()
+    // operating on an invalid (already deallocated) value inside
+    // _PyType_LookupRefAndVersion().  If we write the version first then a
+    // reader could pass the "entry_version == type_version" check but could
+    // be using the old entry value.
+    _Py_atomic_store_uint32_release(&entry->version, version_tag);
     return old_name;
 }
 
@@ -5235,7 +5240,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
     // synchronize-with other writing threads by doing an acquire load on the sequence
     while (1) {
         uint32_t sequence = _PySeqLock_BeginRead(&entry->sequence);
-        uint32_t entry_version = _Py_atomic_load_uint32_relaxed(&entry->version);
+        uint32_t entry_version = _Py_atomic_load_uint32_acquire(&entry->version);
         uint32_t type_version = _Py_atomic_load_uint32_acquire(&type->tp_version_tag);
         if (entry_version == type_version &&
             _Py_atomic_load_ptr_relaxed(&entry->name) == name) {
@@ -5281,11 +5286,14 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
     int has_version = 0;
     int version = 0;
     BEGIN_TYPE_LOCK();
-    res = find_name_in_mro(type, name, &error);
+    // We must assign the version before doing the lookup.  If
+    // find_name_in_mro() blocks and releases the critical section
+    // then the type version can change.
     if (MCACHE_CACHEABLE_NAME(name)) {
         has_version = assign_version_tag(interp, type);
         version = type->tp_version_tag;
     }
+    res = find_name_in_mro(type, name, &error);
     END_TYPE_LOCK();
 
     /* Only put NULL results into cache if there was no error. */

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+Fix two races in the type lookup cache. This affected the free-threaded`
	`2`	`+build and could cause crashes (apparently quite difficult to trigger).`