From 5ebcdf8e3c2a59c00aef91cab845dee6a644c48c Mon Sep 17 00:00:00 2001 From: Peter Hawkins Date: Wed, 7 May 2025 17:19:22 +0000 Subject: [PATCH] gh-132869: Fix crash due to memory ordering problem in dictobject under free threading. Currently the reads and writes to a dictionary entry and to its contents are unordered. For example in `do_lookup` we have the following code: ``` for (;;) { ix = dictkeys_get_index(dk, i); if (ix >= 0) { int cmp = check_lookup(mp, dk, ep0, ix, key, hash); ``` where `dictkeys_get_index` performs a relaxed atomic read of `dk_indices[i]`, where the `check_lookup` function might be, say, compare_unicode_unicode which makes a relaxed load of the me_key value in that index. ``` PyDictUnicodeEntry *ep = &((PyDictUnicodeEntry *)ep0)[ix]; PyObject *ep_key = FT_ATOMIC_LOAD_PTR_RELAXED(ep->me_key); assert(ep_key != NULL); ``` However, the writer also does not order these two writes appropriately; for example `insert_combined_dict` does the following: ``` Py_ssize_t hashpos = find_empty_slot(mp->ma_keys, hash); dictkeys_set_index(mp->ma_keys, hashpos, mp->ma_keys->dk_nentries); if (DK_IS_UNICODE(mp->ma_keys)) { PyDictUnicodeEntry *ep; ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[mp->ma_keys->dk_nentries]; STORE_KEY(ep, key); STORE_VALUE(ep, value); } else { PyDictKeyEntry *ep; ep = &DK_ENTRIES(mp->ma_keys)[mp->ma_keys->dk_nentries]; STORE_KEY(ep, key); STORE_VALUE(ep, value); STORE_HASH(ep, hash); } mp->ma_version_tag = new_version; ``` where the `dk_indices` value is set first, followed by setting the `me_key`, both as relaxed writes. This is problematic because the write to `dk_indices` may be ordered first, either by the program order on x86, or allowed by the relaxed memory ordering semantics of ARM. The reader above will be able to observe a state where the index has been set but the key value is still null, leading to a crash in the reproducer of #132869. The fix is two-fold: * order the index write after the the write to its contents, and * use sequentially consistent reads and writes. It would suffice to use load-acquire and store-release here but those atomic operations do not exist in the CPython atomic headers at the necessary types. I was only able to reproduce the crash under CPython 3.13, but I do not see any reason the bug is fixed on the 3.14 branch either since the code does not seem to have changed. Fixes #132869 --- .../2025-05-08-12-07-56.gh-issue-132869.SpSN9s.rst | 2 ++ Objects/dictobject.c | 13 ++++++------- 2 files changed, 8 insertions(+), 7 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2025-05-08-12-07-56.gh-issue-132869.SpSN9s.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2025-05-08-12-07-56.gh-issue-132869.SpSN9s.rst b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-08-12-07-56.gh-issue-132869.SpSN9s.rst new file mode 100644 index 000000000000000..2dafedb23a22ade --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2025-05-08-12-07-56.gh-issue-132869.SpSN9s.rst @@ -0,0 +1,2 @@ +Fixed a crash from concurrent reads and writes to a dictionary under +free-threading. diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 59b0cf1ce7d4228..e54670446145562 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -168,8 +168,8 @@ ASSERT_DICT_LOCKED(PyObject *op) #define IS_DICT_SHARED(mp) _PyObject_GC_IS_SHARED(mp) #define SET_DICT_SHARED(mp) _PyObject_GC_SET_SHARED(mp) -#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size##_relaxed(&((const int##size##_t*)keys->dk_indices)[idx]); -#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size##_relaxed(&((int##size##_t*)keys->dk_indices)[idx], (int##size##_t)value); +#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size(&((const int##size##_t*)keys->dk_indices)[idx]); +#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size(&((int##size##_t*)keys->dk_indices)[idx], (int##size##_t)value); #define ASSERT_OWNED_OR_SHARED(mp) \ assert(_Py_IsOwnedByCurrentThread((PyObject *)mp) || IS_DICT_SHARED(mp)); @@ -1734,8 +1734,6 @@ insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp, FT_ATOMIC_STORE_UINT32_RELAXED(mp->ma_keys->dk_version, 0); Py_ssize_t hashpos = find_empty_slot(mp->ma_keys, hash); - dictkeys_set_index(mp->ma_keys, hashpos, mp->ma_keys->dk_nentries); - if (DK_IS_UNICODE(mp->ma_keys)) { PyDictUnicodeEntry *ep; ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[mp->ma_keys->dk_nentries]; @@ -1749,6 +1747,7 @@ insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp, STORE_VALUE(ep, value); STORE_HASH(ep, hash); } + dictkeys_set_index(mp->ma_keys, hashpos, mp->ma_keys->dk_nentries); STORE_KEYS_USABLE(mp->ma_keys, mp->ma_keys->dk_usable - 1); STORE_KEYS_NENTRIES(mp->ma_keys, mp->ma_keys->dk_nentries + 1); assert(mp->ma_keys->dk_usable >= 0); @@ -1776,9 +1775,9 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash) FT_ATOMIC_STORE_UINT32_RELAXED(keys->dk_version, 0); Py_ssize_t hashpos = find_empty_slot(keys, hash); ix = keys->dk_nentries; - dictkeys_set_index(keys, hashpos, ix); PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(keys)[ix]; STORE_SHARED_KEY(ep->me_key, Py_NewRef(key)); + dictkeys_set_index(keys, hashpos, ix); split_keys_entry_added(keys); } assert (ix < SHARED_KEYS_MAX_SIZE); @@ -1906,7 +1905,6 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp, assert(mp->ma_values == NULL); size_t hashpos = (size_t)hash & (PyDict_MINSIZE-1); - dictkeys_set_index(newkeys, hashpos, 0); if (unicode) { PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(newkeys); ep->me_key = key; @@ -1918,6 +1916,7 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp, ep->me_hash = hash; STORE_VALUE(ep, value); } + dictkeys_set_index(newkeys, hashpos, 0); STORE_USED(mp, mp->ma_used + 1); newkeys->dk_usable--; newkeys->dk_nentries++; @@ -2736,7 +2735,6 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix, } else { FT_ATOMIC_STORE_UINT32_RELAXED(mp->ma_keys->dk_version, 0); - dictkeys_set_index(mp->ma_keys, hashpos, DKIX_DUMMY); if (DK_IS_UNICODE(mp->ma_keys)) { PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[ix]; old_key = ep->me_key; @@ -2750,6 +2748,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix, STORE_VALUE(ep, NULL); STORE_HASH(ep, 0); } + dictkeys_set_index(mp->ma_keys, hashpos, DKIX_DUMMY); Py_DECREF(old_key); } Py_DECREF(old_value);