Skip to content

Commit d621626

Browse files
committed
[3.13] 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).
1 parent 70f04ea commit d621626

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
@@ -5163,14 +5163,19 @@ is_dunder_name(PyObject *name)
51635163
static PyObject *
51645164
update_cache(struct type_cache_entry *entry, PyObject *name, unsigned int version_tag, PyObject *value)
51655165
{
5166-
_Py_atomic_store_uint32_relaxed(&entry->version, version_tag);
51675166
_Py_atomic_store_ptr_relaxed(&entry->value, value); /* borrowed */
51685167
assert(_PyASCIIObject_CAST(name)->hash != -1);
51695168
OBJECT_STAT_INC_COND(type_cache_collisions, entry->name != Py_None && entry->name != name);
51705169
// We're releasing this under the lock for simplicity sake because it's always a
51715170
// exact unicode object or Py_None so it's safe to do so.
51725171
PyObject *old_name = entry->name;
51735172
_Py_atomic_store_ptr_relaxed(&entry->name, Py_NewRef(name));
5173+
// We must write the version last to avoid _Py_TryXGetStackRef()
5174+
// operating on an invalid (already deallocated) value inside
5175+
// _PyType_LookupRefAndVersion(). If we write the version first then a
5176+
// reader could pass the "entry_version == type_version" check but could
5177+
// be using the old entry value.
5178+
_Py_atomic_store_uint32_release(&entry->version, version_tag);
51745179
return old_name;
51755180
}
51765181

@@ -5234,7 +5239,7 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
52345239
// synchronize-with other writing threads by doing an acquire load on the sequence
52355240
while (1) {
52365241
uint32_t sequence = _PySeqLock_BeginRead(&entry->sequence);
5237-
uint32_t entry_version = _Py_atomic_load_uint32_relaxed(&entry->version);
5242+
uint32_t entry_version = _Py_atomic_load_uint32_acquire(&entry->version);
52385243
uint32_t type_version = _Py_atomic_load_uint32_acquire(&type->tp_version_tag);
52395244
if (entry_version == type_version &&
52405245
_Py_atomic_load_ptr_relaxed(&entry->name) == name) {
@@ -5280,11 +5285,14 @@ _PyType_LookupRef(PyTypeObject *type, PyObject *name)
52805285
int has_version = 0;
52815286
int version = 0;
52825287
BEGIN_TYPE_LOCK();
5283-
res = find_name_in_mro(type, name, &error);
5288+
// We must assign the version before doing the lookup. If
5289+
// find_name_in_mro() blocks and releases the critical section
5290+
// then the type version can change.
52845291
if (MCACHE_CACHEABLE_NAME(name)) {
52855292
has_version = assign_version_tag(interp, type);
52865293
version = type->tp_version_tag;
52875294
}
5295+
res = find_name_in_mro(type, name, &error);
52885296
END_TYPE_LOCK();
52895297

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

0 commit comments

Comments
 (0)