From 90a7d35ab3c0dd2302351917811d19c8513fd001 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Sat, 26 Apr 2025 17:24:54 -0700 Subject: [PATCH 1/3] gh-132942: Fix races in type lookup cache 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). --- Objects/typeobject.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index a720a98121dc93..184b17ce191293 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5678,8 +5678,13 @@ 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); + // We must write the value first 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_ptr_relaxed(&entry->value, value); /* borrowed */ + _Py_atomic_store_uint32_relaxed(&entry->version, version_tag); 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 @@ -5809,11 +5814,14 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef int has_version = 0; unsigned int assigned_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); assigned_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. */ From 5cd03e856b3b31669158df2707640408e29eeb0c Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Sat, 26 Apr 2025 17:50:51 -0700 Subject: [PATCH 2/3] Add NEWS. --- .../2025-04-26-17-50-47.gh-issue-132942.aEEZvZ.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-04-26-17-50-47.gh-issue-132942.aEEZvZ.rst 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 new file mode 100644 index 00000000000000..9b7cf5516182b2 --- /dev/null +++ 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). From 6d3084113488cc2861238e50046d588d6b6230e0 Mon Sep 17 00:00:00 2001 From: Neil Schemenauer Date: Mon, 28 Apr 2025 11:13:23 -0700 Subject: [PATCH 3/3] Use release/acquire pair for entry->version. --- Objects/typeobject.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 184b17ce191293..4e614daaa6955b 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -5678,19 +5678,19 @@ is_dunder_name(PyObject *name) static PyObject * update_cache(struct type_cache_entry *entry, PyObject *name, unsigned int version_tag, PyObject *value) { - // We must write the value first 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_ptr_relaxed(&entry->value, value); /* borrowed */ - _Py_atomic_store_uint32_relaxed(&entry->version, version_tag); 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; } @@ -5767,7 +5767,7 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef // 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) {