Skip to content

Commit 31d1342

Browse files
authored
gh-132942: Fix races in type lookup cache (gh-133032)
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).
1 parent fe462f5 commit 31d1342

File tree

2 files changed

+13
-3
lines changed

2 files changed

+13
-3
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff 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).

Objects/typeobject.c

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5678,14 +5678,19 @@ is_dunder_name(PyObject *name)
56785678
static PyObject *
56795679
update_cache(struct type_cache_entry *entry, PyObject *name, unsigned int version_tag, PyObject *value)
56805680
{
5681-
_Py_atomic_store_uint32_relaxed(&entry->version, version_tag);
56825681
_Py_atomic_store_ptr_relaxed(&entry->value, value); /* borrowed */
56835682
assert(_PyASCIIObject_CAST(name)->hash != -1);
56845683
OBJECT_STAT_INC_COND(type_cache_collisions, entry->name != Py_None && entry->name != name);
56855684
// We're releasing this under the lock for simplicity sake because it's always a
56865685
// exact unicode object or Py_None so it's safe to do so.
56875686
PyObject *old_name = entry->name;
56885687
_Py_atomic_store_ptr_relaxed(&entry->name, Py_NewRef(name));
5688+
// We must write the version last to avoid _Py_TryXGetStackRef()
5689+
// operating on an invalid (already deallocated) value inside
5690+
// _PyType_LookupRefAndVersion(). If we write the version first then a
5691+
// reader could pass the "entry_version == type_version" check but could
5692+
// be using the old entry value.
5693+
_Py_atomic_store_uint32_release(&entry->version, version_tag);
56895694
return old_name;
56905695
}
56915696

@@ -5762,7 +5767,7 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef
57625767
// synchronize-with other writing threads by doing an acquire load on the sequence
57635768
while (1) {
57645769
uint32_t sequence = _PySeqLock_BeginRead(&entry->sequence);
5765-
uint32_t entry_version = _Py_atomic_load_uint32_relaxed(&entry->version);
5770+
uint32_t entry_version = _Py_atomic_load_uint32_acquire(&entry->version);
57665771
uint32_t type_version = _Py_atomic_load_uint32_acquire(&type->tp_version_tag);
57675772
if (entry_version == type_version &&
57685773
_Py_atomic_load_ptr_relaxed(&entry->name) == name) {
@@ -5809,11 +5814,14 @@ _PyType_LookupStackRefAndVersion(PyTypeObject *type, PyObject *name, _PyStackRef
58095814
int has_version = 0;
58105815
unsigned int assigned_version = 0;
58115816
BEGIN_TYPE_LOCK();
5812-
res = find_name_in_mro(type, name, &error);
5817+
// We must assign the version before doing the lookup. If
5818+
// find_name_in_mro() blocks and releases the critical section
5819+
// then the type version can change.
58135820
if (MCACHE_CACHEABLE_NAME(name)) {
58145821
has_version = assign_version_tag(interp, type);
58155822
assigned_version = type->tp_version_tag;
58165823
}
5824+
res = find_name_in_mro(type, name, &error);
58175825
END_TYPE_LOCK();
58185826

58195827
/* Only put NULL results into cache if there was no error. */

0 commit comments

Comments
 (0)