Skip to content

Commit 5ebcdf8

Browse files
hawkinsphawkinsp
committed
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
1 parent 8bb9286 commit 5ebcdf8

2 files changed

Lines changed: 8 additions & 7 deletions

File tree

‎Misc/NEWS.d/next/Core_and_Builtins/2025-05-08-12-07-56.gh-issue-132869.SpSN9s.rst‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fixed a crash from concurrent reads and writes to a dictionary under
2+
free-threading.

‎Objects/dictobject.c‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,8 @@ ASSERT_DICT_LOCKED(PyObject *op)
168168

169169
#define IS_DICT_SHARED(mp) _PyObject_GC_IS_SHARED(mp)
170170
#define SET_DICT_SHARED(mp) _PyObject_GC_SET_SHARED(mp)
171-
#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size##_relaxed(&((const int##size##_t*)keys->dk_indices)[idx]);
172-
#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size##_relaxed(&((int##size##_t*)keys->dk_indices)[idx], (int##size##_t)value);
171+
#define LOAD_INDEX(keys, size, idx) _Py_atomic_load_int##size(&((const int##size##_t*)keys->dk_indices)[idx]);
172+
#define STORE_INDEX(keys, size, idx, value) _Py_atomic_store_int##size(&((int##size##_t*)keys->dk_indices)[idx], (int##size##_t)value);
173173
#define ASSERT_OWNED_OR_SHARED(mp) \
174174
assert(_Py_IsOwnedByCurrentThread((PyObject *)mp) || IS_DICT_SHARED(mp));
175175

@@ -1734,8 +1734,6 @@ insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp,
17341734
FT_ATOMIC_STORE_UINT32_RELAXED(mp->ma_keys->dk_version, 0);
17351735

17361736
Py_ssize_t hashpos = find_empty_slot(mp->ma_keys, hash);
1737-
dictkeys_set_index(mp->ma_keys, hashpos, mp->ma_keys->dk_nentries);
1738-
17391737
if (DK_IS_UNICODE(mp->ma_keys)) {
17401738
PyDictUnicodeEntry *ep;
17411739
ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[mp->ma_keys->dk_nentries];
@@ -1749,6 +1747,7 @@ insert_combined_dict(PyInterpreterState *interp, PyDictObject *mp,
17491747
STORE_VALUE(ep, value);
17501748
STORE_HASH(ep, hash);
17511749
}
1750+
dictkeys_set_index(mp->ma_keys, hashpos, mp->ma_keys->dk_nentries);
17521751
STORE_KEYS_USABLE(mp->ma_keys, mp->ma_keys->dk_usable - 1);
17531752
STORE_KEYS_NENTRIES(mp->ma_keys, mp->ma_keys->dk_nentries + 1);
17541753
assert(mp->ma_keys->dk_usable >= 0);
@@ -1776,9 +1775,9 @@ insert_split_key(PyDictKeysObject *keys, PyObject *key, Py_hash_t hash)
17761775
FT_ATOMIC_STORE_UINT32_RELAXED(keys->dk_version, 0);
17771776
Py_ssize_t hashpos = find_empty_slot(keys, hash);
17781777
ix = keys->dk_nentries;
1779-
dictkeys_set_index(keys, hashpos, ix);
17801778
PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(keys)[ix];
17811779
STORE_SHARED_KEY(ep->me_key, Py_NewRef(key));
1780+
dictkeys_set_index(keys, hashpos, ix);
17821781
split_keys_entry_added(keys);
17831782
}
17841783
assert (ix < SHARED_KEYS_MAX_SIZE);
@@ -1906,7 +1905,6 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp,
19061905
assert(mp->ma_values == NULL);
19071906

19081907
size_t hashpos = (size_t)hash & (PyDict_MINSIZE-1);
1909-
dictkeys_set_index(newkeys, hashpos, 0);
19101908
if (unicode) {
19111909
PyDictUnicodeEntry *ep = DK_UNICODE_ENTRIES(newkeys);
19121910
ep->me_key = key;
@@ -1918,6 +1916,7 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp,
19181916
ep->me_hash = hash;
19191917
STORE_VALUE(ep, value);
19201918
}
1919+
dictkeys_set_index(newkeys, hashpos, 0);
19211920
STORE_USED(mp, mp->ma_used + 1);
19221921
newkeys->dk_usable--;
19231922
newkeys->dk_nentries++;
@@ -2736,7 +2735,6 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
27362735
}
27372736
else {
27382737
FT_ATOMIC_STORE_UINT32_RELAXED(mp->ma_keys->dk_version, 0);
2739-
dictkeys_set_index(mp->ma_keys, hashpos, DKIX_DUMMY);
27402738
if (DK_IS_UNICODE(mp->ma_keys)) {
27412739
PyDictUnicodeEntry *ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[ix];
27422740
old_key = ep->me_key;
@@ -2750,6 +2748,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
27502748
STORE_VALUE(ep, NULL);
27512749
STORE_HASH(ep, 0);
27522750
}
2751+
dictkeys_set_index(mp->ma_keys, hashpos, DKIX_DUMMY);
27532752
Py_DECREF(old_key);
27542753
}
27552754
Py_DECREF(old_value);

0 commit comments

Comments
 (0)