diff --git a/Lib/test/test_free_threading/test_dict.py b/Lib/test/test_free_threading/test_dict.py index ad23290a92ab345..0444571bafb56b4 100644 --- a/Lib/test/test_free_threading/test_dict.py +++ b/Lib/test/test_free_threading/test_dict.py @@ -163,6 +163,122 @@ def work(d): for t in worker_threads: t.join() + def test_racing_load_attr_with_hint_and_resize(self): + """Concurrent LOAD_ATTR_WITH_HINT vs dict resize must not UAF. + + The specialized hint path snapshots ma_keys and reads me_value from + that table. A concurrent resize can publish a new keys table and + free/overwrite the old value while a reader still holds the stale + table (QSBR-retained). Stress the interleaving hard. + + The attribute ``x`` is only ever assigned (never deleted). Readers + must therefore always observe a ``str`` value without crashing. + """ + class C: + pass + + obj = C() + # Force a materialized combined dict with enough entries that the + # attribute is specialized to LOAD_ATTR_WITH_HINT and subsequent + # inserts trigger resizes. + for i in range(32): + setattr(obj, f"_pad{i}", i) + obj.x = "initial" + + # Warm up specialization on the reader side. + for _ in range(200): + _ = obj.x + + stop = False + errors = [] + + def reader(): + try: + while not stop: + val = obj.x + # Value must always be a str from the writer or the + # initial assignment; never a dangling/reused object. + if not isinstance(val, str): + errors.append(f"unexpected type: {type(val)!r}") + return + except Exception as e: + errors.append(repr(e)) + + def writer(): + n = 0 + try: + while not stop: + # Update x (may free previous value while readers hold + # a stale keys table pointing at it). + obj.x = f"v{n}" + n += 1 + # Churn *other* keys to force repeated resizes. Only + # one writer runs so setattr/delattr of _tmp* cannot + # race with itself. + if n % 3 == 0: + for i in range(16): + setattr(obj, f"_tmp{i}", n) + for i in range(16): + delattr(obj, f"_tmp{i}") + except Exception as e: + errors.append(repr(e)) + + readers = [Thread(target=reader) for _ in range(4)] + # Single writer: keeps ``x`` continuously defined and avoids + # racing delattr on the padding keys. + writers = [Thread(target=writer)] + for t in readers + writers: + t.start() + time.sleep(0.5) + stop = True + for t in readers + writers: + t.join(timeout=5) + self.assertFalse(t.is_alive()) + self.assertEqual(errors, []) + self.assertIsInstance(obj.x, str) + + def test_racing_dict_resize_and_lookup(self): + """Lock-free lookups racing with repeated resizes must not crash.""" + d = {f"k{i}": i for i in range(8)} + d["x"] = 0 + stop = False + errors = [] + + def reader(): + try: + while not stop: + for i in range(8): + _ = d.get(f"k{i}") + _ = d.get("x") + # Direct subscript of a key that is never deleted. + _ = d["x"] + except Exception as e: + errors.append(repr(e)) + + def resizer(): + n = 0 + try: + while not stop: + d["x"] = n + for i in range(64): + d[f"tmp{i}"] = n + for i in range(64): + d.pop(f"tmp{i}", None) + n += 1 + except Exception as e: + errors.append(repr(e)) + + threads = [Thread(target=reader) for _ in range(4)] + threads += [Thread(target=resizer)] + for t in threads: + t.start() + time.sleep(0.5) + stop = True + for t in threads: + t.join(timeout=5) + self.assertFalse(t.is_alive()) + self.assertEqual(errors, []) + def test_racing_set_object_dict(self): """Races assigning to __dict__ should be thread safe""" diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-07-14-18-30-00.gh-issue-149816.dhxla1.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-07-14-18-30-00.gh-issue-149816.dhxla1.rst new file mode 100644 index 000000000000000..ca9c687560bd0a3 --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-07-14-18-30-00.gh-issue-149816.dhxla1.rst @@ -0,0 +1,5 @@ +Fix a use-after-free in the free-threaded build when a dictionary is resized +concurrently with lock-free lookups (including the specialized +``LOAD_ATTR_WITH_HINT`` path). Old keys and values tables are now cleared +before QSBR-delayed reclamation, and lock-free lookup treats a poisoned +entry as a signal to retry under the dict lock. diff --git a/Modules/_testinternalcapi/test_cases.c.h b/Modules/_testinternalcapi/test_cases.c.h index a17648a33d4fe4a..df0f7c9150c9f92 100644 --- a/Modules/_testinternalcapi/test_cases.c.h +++ b/Modules/_testinternalcapi/test_cases.c.h @@ -9618,9 +9618,19 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } - STAT_INC(LOAD_ATTR, hit); #ifdef Py_GIL_DISABLED - int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr); + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys)) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + if (FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + _PyStackRef tmp_attr; + int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &tmp_attr); if (!increfed) { if (true) { UPDATE_MISS_STATS(LOAD_ATTR); @@ -9628,7 +9638,22 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys) || + FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + _PyFrame_SetStackPointer(frame, stack_pointer); + _PyFrame_StackPointerValidate(frame); + PyStackRef_CLOSE(tmp_attr); + _PyFrame_StackPointerInvalidate(frame); + if (true) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + } + STAT_INC(LOAD_ATTR, hit); + attr = tmp_attr; #else + STAT_INC(LOAD_ATTR, hit); attr = PyStackRef_FromPyObjectNew(attr_o); #endif o = owner; diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 246ffe6c18e9b51..e78f7729aeeabd8 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -490,6 +490,39 @@ dictkeys_incref(PyDictKeysObject *dk) INCREF_KEYS(dk); } +/* Clear key/value pointers in a keys table without changing refcounts. + * + * Under free-threading, old keys tables may outlive the logical free via + * QSBR so that concurrent lock-free readers can still touch the memory. + * Those readers must not observe live object pointers in a table whose + * contents have already been transferred or released: a later update of + * the dict can free those objects while a reader still holds the stale + * table. Nulling the slots forces try-incref paths to fail cleanly. + * + * Callers that still own the references (e.g. dictkeys_decref) must + * snapshot them before calling this helper; callers that transferred + * ownership (e.g. dictresize) must not decref them afterwards. + */ +static void +dictkeys_clear_entries(PyDictKeysObject *keys) +{ + Py_ssize_t i, n = keys->dk_nentries; + if (DK_IS_UNICODE(keys)) { + PyDictUnicodeEntry *entries = DK_UNICODE_ENTRIES(keys); + for (i = 0; i < n; i++) { + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_key, NULL); + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_value, NULL); + } + } + else { + PyDictKeyEntry *entries = DK_ENTRIES(keys); + for (i = 0; i < n; i++) { + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_key, NULL); + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_value, NULL); + } + } +} + static inline void dictkeys_decref(PyDictKeysObject *dk, bool use_qsbr) { @@ -506,16 +539,27 @@ dictkeys_decref(PyDictKeysObject *dk, bool use_qsbr) PyDictUnicodeEntry *entries = DK_UNICODE_ENTRIES(dk); Py_ssize_t i, n; for (i = 0, n = dk->dk_nentries; i < n; i++) { - Py_XDECREF(entries[i].me_key); - Py_XDECREF(entries[i].me_value); + /* Snapshot then null *before* decref so a concurrent + * lock-free reader of a QSBR-retained table cannot follow + * a pointer to an object we are about to free. */ + PyObject *key = entries[i].me_key; + PyObject *value = entries[i].me_value; + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_key, NULL); + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_value, NULL); + Py_XDECREF(key); + Py_XDECREF(value); } } else { PyDictKeyEntry *entries = DK_ENTRIES(dk); Py_ssize_t i, n; for (i = 0, n = dk->dk_nentries; i < n; i++) { - Py_XDECREF(entries[i].me_key); - Py_XDECREF(entries[i].me_value); + PyObject *key = entries[i].me_key; + PyObject *value = entries[i].me_value; + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_key, NULL); + FT_ATOMIC_STORE_PTR_RELEASE(entries[i].me_value, NULL); + Py_XDECREF(key); + Py_XDECREF(value); } } free_keys_object(dk, use_qsbr); @@ -886,6 +930,11 @@ free_keys_object(PyDictKeysObject *keys, bool use_qsbr) } #ifdef Py_GIL_DISABLED if (use_qsbr) { + /* Ownership of any remaining entry references has already been + * transferred or released by the caller. Poison the slots so + * concurrent readers of this QSBR-retained table cannot revive + * a transferred/freed object via a stale me_key/me_value. */ + dictkeys_clear_entries(keys); _PyMem_FreeDelayed(ptr, size); return; } @@ -931,6 +980,13 @@ free_values(PyDictValues *values, bool use_qsbr) assert(values->embedded == 0); #ifdef Py_GIL_DISABLED if (use_qsbr) { + /* Same rationale as free_keys_object: the values array may outlive + * the logical free via QSBR. Callers have either transferred + * ownership of the stored objects or already decref'd them; clear + * the slots so concurrent lock-free readers fail try-incref. */ + for (uint8_t i = 0; i < values->capacity; i++) { + FT_ATOMIC_STORE_PTR_RELEASE(values->values[i], NULL); + } _PyMem_FreeDelayed(values, values_size_from_count(values->capacity)); return; } @@ -1490,30 +1546,32 @@ compare_unicode_generic_threadsafe(PyDictObject *mp, PyDictKeysObject *dk, assert(startkey == NULL || PyUnicode_CheckExact(ep->me_key)); assert(!PyUnicode_CheckExact(key)); - if (startkey != NULL) { - if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { - return DKIX_KEY_CHANGED; - } + if (startkey == NULL) { + /* See compare_unicode_unicode_threadsafe. */ + return DKIX_KEY_CHANGED; + } + if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { + return DKIX_KEY_CHANGED; + } - if (unicode_get_hash(startkey) == hash) { - int cmp = PyObject_RichCompareBool(startkey, key, Py_EQ); - Py_DECREF(startkey); - if (cmp < 0) { - return DKIX_ERROR; - } - if (dk == _Py_atomic_load_ptr_relaxed(&mp->ma_keys) && - startkey == _Py_atomic_load_ptr_relaxed(&ep->me_key)) { - return cmp; - } - else { - /* The dict was mutated, restart */ - return DKIX_KEY_CHANGED; - } + if (unicode_get_hash(startkey) == hash) { + int cmp = PyObject_RichCompareBool(startkey, key, Py_EQ); + Py_DECREF(startkey); + if (cmp < 0) { + return DKIX_ERROR; + } + if (dk == _Py_atomic_load_ptr_relaxed(&mp->ma_keys) && + startkey == _Py_atomic_load_ptr_relaxed(&ep->me_key)) { + return cmp; } else { - Py_DECREF(startkey); + /* The dict was mutated, restart */ + return DKIX_KEY_CHANGED; } } + else { + Py_DECREF(startkey); + } return 0; } @@ -1534,22 +1592,27 @@ compare_unicode_unicode_threadsafe(PyDictObject *mp, PyDictKeysObject *dk, assert(PyUnicode_CheckExact(startkey)); return 1; } - if (startkey != NULL) { - if (_Py_IsImmortal(startkey)) { - assert(PyUnicode_CheckExact(startkey)); - return unicode_get_hash(startkey) == hash && unicode_eq(startkey, key); + if (startkey == NULL) { + /* Active indices never point at a NULL me_key under the normal + * delete protocol (index is set to DKIX_DUMMY first). A NULL + * key with ix >= 0 means a concurrent resize/clear has poisoned + * this QSBR-retained table; force a locked retry. */ + return DKIX_KEY_CHANGED; + } + if (_Py_IsImmortal(startkey)) { + assert(PyUnicode_CheckExact(startkey)); + return unicode_get_hash(startkey) == hash && unicode_eq(startkey, key); + } + else { + if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { + return DKIX_KEY_CHANGED; } - else { - if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { - return DKIX_KEY_CHANGED; - } - assert(PyUnicode_CheckExact(startkey)); - if (unicode_get_hash(startkey) == hash && unicode_eq(startkey, key)) { - Py_DECREF(startkey); - return 1; - } + assert(PyUnicode_CheckExact(startkey)); + if (unicode_get_hash(startkey) == hash && unicode_eq(startkey, key)) { Py_DECREF(startkey); + return 1; } + Py_DECREF(startkey); } return 0; } @@ -1569,9 +1632,14 @@ compare_generic_threadsafe(PyDictObject *mp, PyDictKeysObject *dk, if (startkey == key) { return 1; } + if (startkey == NULL) { + /* See compare_unicode_unicode_threadsafe: NULL me_key under an + * active index means the keys table is being reclaimed. */ + return DKIX_KEY_CHANGED; + } Py_ssize_t ep_hash = _Py_atomic_load_ssize_relaxed(&ep->me_hash); if (ep_hash == hash) { - if (startkey == NULL || !_Py_TryIncrefCompare(&ep->me_key, startkey)) { + if (!_Py_TryIncrefCompare(&ep->me_key, startkey)) { return DKIX_KEY_CHANGED; } int cmp = PyObject_RichCompareBool(startkey, key, Py_EQ); @@ -1696,7 +1764,8 @@ _Py_dict_lookup_threadsafe(PyDictObject *mp, PyObject *key, Py_hash_t hash, PyOb } static Py_ssize_t -lookup_threadsafe_unicode(PyDictKeysObject *dk, PyObject *key, Py_hash_t hash, _PyStackRef *value_addr) +lookup_threadsafe_unicode(PyDictObject *mp, PyDictKeysObject *dk, PyObject *key, + Py_hash_t hash, _PyStackRef *value_addr) { assert(dk->dk_kind == DICT_KEYS_UNICODE); assert(PyUnicode_CheckExact(key)); @@ -1714,10 +1783,23 @@ lookup_threadsafe_unicode(PyDictKeysObject *dk, PyObject *key, Py_hash_t hash, _ return DKIX_EMPTY; } if (_PyObject_HasDeferredRefcount(value)) { + /* Deferred-refcount objects skip the CAS on *addr_of_value, + * so re-validate that we still see the same keys table and + * the same slot value before handing out a borrowed tagged + * ref. Otherwise a concurrent resize can free the value + * while we return a stale pointer. */ + if (dk != _Py_atomic_load_ptr(&mp->ma_keys) || + value != _Py_atomic_load_ptr(addr_of_value)) { + return DKIX_KEY_CHANGED; + } *value_addr = (_PyStackRef){ .bits = (uintptr_t)value | Py_TAG_REFCNT }; return ix; } if (_Py_TryIncrefCompare(addr_of_value, value)) { + if (dk != _Py_atomic_load_ptr(&mp->ma_keys)) { + Py_DECREF(value); + return DKIX_KEY_CHANGED; + } *value_addr = PyStackRef_FromPyObjectSteal(value); return ix; } @@ -1734,7 +1816,7 @@ _Py_dict_lookup_threadsafe_stackref(PyDictObject *mp, PyObject *key, Py_hash_t h PyDictKeysObject *dk = _Py_atomic_load_ptr_acquire(&mp->ma_keys); if (dk->dk_kind == DICT_KEYS_UNICODE && PyUnicode_CheckExact(key)) { - Py_ssize_t ix = lookup_threadsafe_unicode(dk, key, hash, value_addr); + Py_ssize_t ix = lookup_threadsafe_unicode(mp, dk, key, hash, value_addr); if (ix != DKIX_KEY_CHANGED) { return ix; } @@ -1796,7 +1878,7 @@ _PyDict_GetMethodStackRef(PyDictObject *mp, PyObject *key, _PyStackRef *method) PyDictKeysObject *dk = _Py_atomic_load_ptr_acquire(&mp->ma_keys); if (dk->dk_kind == DICT_KEYS_UNICODE) { _PyStackRef ref; - Py_ssize_t ix = lookup_threadsafe_unicode(dk, key, hash, &ref); + Py_ssize_t ix = lookup_threadsafe_unicode(mp, dk, key, hash, &ref); if (ix >= 0) { assert(!PyStackRef_IsNull(ref)); PyStackRef_XSETREF(*method, ref); diff --git a/Python/bytecodes.c b/Python/bytecodes.c index 4d7b338e2dbd4c3..5ee4bc2cd16a7d4 100644 --- a/Python/bytecodes.c +++ b/Python/bytecodes.c @@ -2980,13 +2980,31 @@ dummy_func( if (attr_o == NULL) { EXIT_IF(true); } - STAT_INC(LOAD_ATTR, hit); #ifdef Py_GIL_DISABLED - int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr); + /* A concurrent resize publishes a new keys table and may + * free or overwrite values while we hold a stale table + * pointer (QSBR-retained). Reject the hit unless ma_keys + * still matches and the slot still holds attr_o before we + * try to take a reference. A second check after the + * try-incref closes the remaining window, including for + * deferred-refcount objects whose try-incref path does not + * re-validate the slot itself. */ + EXIT_IF(dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys)); + EXIT_IF(FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o); + _PyStackRef tmp_attr; + int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &tmp_attr); if (!increfed) { EXIT_IF(true); } + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys) || + FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + PyStackRef_CLOSE(tmp_attr); + EXIT_IF(true); + } + STAT_INC(LOAD_ATTR, hit); + attr = tmp_attr; #else + STAT_INC(LOAD_ATTR, hit); attr = PyStackRef_FromPyObjectNew(attr_o); #endif o = owner; diff --git a/Python/executor_cases.c.h b/Python/executor_cases.c.h index e45bbd7cceb295f..90d043556b2894e 100644 --- a/Python/executor_cases.c.h +++ b/Python/executor_cases.c.h @@ -12352,9 +12352,22 @@ JUMP_TO_JUMP_TARGET(); } } - STAT_INC(LOAD_ATTR, hit); #ifdef Py_GIL_DISABLED - int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr); + + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys)) { + UOP_STAT_INC(uopcode, miss); + _tos_cache0 = owner; + SET_CURRENT_CACHED_VALUES(1); + JUMP_TO_JUMP_TARGET(); + } + if (FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + UOP_STAT_INC(uopcode, miss); + _tos_cache0 = owner; + SET_CURRENT_CACHED_VALUES(1); + JUMP_TO_JUMP_TARGET(); + } + _PyStackRef tmp_attr; + int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &tmp_attr); if (!increfed) { if (true) { UOP_STAT_INC(uopcode, miss); @@ -12363,7 +12376,28 @@ JUMP_TO_JUMP_TARGET(); } } + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys) || + FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + stack_pointer[0] = owner; + stack_pointer += 1; + ASSERT_WITHIN_STACK_BOUNDS(__FILE__, __LINE__); + _PyFrame_SetStackPointer(frame, stack_pointer); + _PyFrame_StackPointerValidate(frame); + PyStackRef_CLOSE(tmp_attr); + _PyFrame_StackPointerInvalidate(frame); + if (true) { + UOP_STAT_INC(uopcode, miss); + _tos_cache0 = owner; + SET_CURRENT_CACHED_VALUES(1); + stack_pointer += -1; + ASSERT_WITHIN_STACK_BOUNDS(__FILE__, __LINE__); + JUMP_TO_JUMP_TARGET(); + } + } + STAT_INC(LOAD_ATTR, hit); + attr = tmp_attr; #else + STAT_INC(LOAD_ATTR, hit); attr = PyStackRef_FromPyObjectNew(attr_o); #endif o = owner; diff --git a/Python/generated_cases.c.h b/Python/generated_cases.c.h index 6178dc70c1b80e7..9056b830e9c7fe7 100644 --- a/Python/generated_cases.c.h +++ b/Python/generated_cases.c.h @@ -9617,9 +9617,19 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } - STAT_INC(LOAD_ATTR, hit); #ifdef Py_GIL_DISABLED - int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &attr); + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys)) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + if (FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + _PyStackRef tmp_attr; + int increfed = _Py_TryIncrefCompareStackRef(&ep->me_value, attr_o, &tmp_attr); if (!increfed) { if (true) { UPDATE_MISS_STATS(LOAD_ATTR); @@ -9627,7 +9637,22 @@ JUMP_TO_PREDICTED(LOAD_ATTR); } } + if (dk != FT_ATOMIC_LOAD_PTR(dict->ma_keys) || + FT_ATOMIC_LOAD_PTR(ep->me_value) != attr_o) { + _PyFrame_SetStackPointer(frame, stack_pointer); + _PyFrame_StackPointerValidate(frame); + PyStackRef_CLOSE(tmp_attr); + _PyFrame_StackPointerInvalidate(frame); + if (true) { + UPDATE_MISS_STATS(LOAD_ATTR); + assert(_PyOpcode_Deopt[opcode] == (LOAD_ATTR)); + JUMP_TO_PREDICTED(LOAD_ATTR); + } + } + STAT_INC(LOAD_ATTR, hit); + attr = tmp_attr; #else + STAT_INC(LOAD_ATTR, hit); attr = PyStackRef_FromPyObjectNew(attr_o); #endif o = owner;