Skip to content

gh-132942: Fix races in type lookup cache #133032

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 28, 2025
Merged

gh-132942: Fix races in type lookup cache #133032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90a7d35
gh-132942: Fix races in type lookup cache
nascheme Apr 27, 2025
5cd03e8
Add NEWS.
nascheme Apr 27, 2025
6d30841
Use release/acquire pair for entry->version.
nascheme Apr 28, 2025
3e01796
Merge 'origin/main' into gh-132942-tp-lookup-race
nascheme Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Core_and_Builtins/2025-04-26-17-50-47.gh-issue-132942.aEEZvZ.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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).
14 changes: 11 additions & 3 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5678,14 +5678,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;
}

Expand Down Expand Up @@ -5762,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) {
Expand Down Expand Up @@ -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. */
Expand Down
Loading