From b12d1753d6b62aa81c35b965779c639ca1d08a4f Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Wed, 10 Jul 2024 14:04:12 -0400 Subject: [PATCH] gh-117657: Fix TSan race in _PyDict_CheckConsistency (GH-121551) The only remaining race in dictobject.c was in _PyDict_CheckConsistency when the dictionary has shared keys. (cherry picked from commit 3ec719fabf936ea7a012a76445b860759155de86) Co-authored-by: Sam Gross --- Objects/dictobject.c | 24 ++++++++++++++-------- Tools/tsan/suppressions_free_threading.txt | 8 -------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 81be64fcd9a368..3cd267556342ec 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -165,16 +165,15 @@ ASSERT_DICT_LOCKED(PyObject *op) #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 ASSERT_OWNED_OR_SHARED(mp) \ assert(_Py_IsOwnedByCurrentThread((PyObject *)mp) || IS_DICT_SHARED(mp)); -#define LOAD_KEYS_NENTRIES(d) #define LOCK_KEYS_IF_SPLIT(keys, kind) \ if (kind == DICT_KEYS_SPLIT) { \ - LOCK_KEYS(dk); \ + LOCK_KEYS(keys); \ } #define UNLOCK_KEYS_IF_SPLIT(keys, kind) \ if (kind == DICT_KEYS_SPLIT) { \ - UNLOCK_KEYS(dk); \ + UNLOCK_KEYS(keys); \ } static inline Py_ssize_t @@ -208,7 +207,7 @@ set_values(PyDictObject *mp, PyDictValues *values) #define INCREF_KEYS(dk) _Py_atomic_add_ssize(&dk->dk_refcnt, 1) // Dec refs the keys object, giving the previous value #define DECREF_KEYS(dk) _Py_atomic_add_ssize(&dk->dk_refcnt, -1) -#define LOAD_KEYS_NENTIRES(keys) _Py_atomic_load_ssize_relaxed(&keys->dk_nentries) +#define LOAD_KEYS_NENTRIES(keys) _Py_atomic_load_ssize_relaxed(&keys->dk_nentries) #define INCREF_KEYS_FT(dk) dictkeys_incref(dk) #define DECREF_KEYS_FT(dk, shared) dictkeys_decref(_PyInterpreterState_GET(), dk, shared) @@ -234,7 +233,7 @@ static inline void split_keys_entry_added(PyDictKeysObject *keys) #define STORE_SHARED_KEY(key, value) key = value #define INCREF_KEYS(dk) dk->dk_refcnt++ #define DECREF_KEYS(dk) dk->dk_refcnt-- -#define LOAD_KEYS_NENTIRES(keys) keys->dk_nentries +#define LOAD_KEYS_NENTRIES(keys) keys->dk_nentries #define INCREF_KEYS_FT(dk) #define DECREF_KEYS_FT(dk, shared) #define LOCK_KEYS_IF_SPLIT(keys, kind) @@ -689,10 +688,15 @@ _PyDict_CheckConsistency(PyObject *op, int check_content) int splitted = _PyDict_HasSplitTable(mp); Py_ssize_t usable = USABLE_FRACTION(DK_SIZE(keys)); + // In the free-threaded build, shared keys may be concurrently modified, + // so use atomic loads. + Py_ssize_t dk_usable = FT_ATOMIC_LOAD_SSIZE_ACQUIRE(keys->dk_usable); + Py_ssize_t dk_nentries = FT_ATOMIC_LOAD_SSIZE_ACQUIRE(keys->dk_nentries); + CHECK(0 <= mp->ma_used && mp->ma_used <= usable); - CHECK(0 <= keys->dk_usable && keys->dk_usable <= usable); - CHECK(0 <= keys->dk_nentries && keys->dk_nentries <= usable); - CHECK(keys->dk_usable + keys->dk_nentries <= usable); + CHECK(0 <= dk_usable && dk_usable <= usable); + CHECK(0 <= dk_nentries && dk_nentries <= usable); + CHECK(dk_usable + dk_nentries <= usable); if (!splitted) { /* combined table */ @@ -709,6 +713,7 @@ _PyDict_CheckConsistency(PyObject *op, int check_content) } if (check_content) { + LOCK_KEYS_IF_SPLIT(keys, keys->dk_kind); for (Py_ssize_t i=0; i < DK_SIZE(keys); i++) { Py_ssize_t ix = dictkeys_get_index(keys, i); CHECK(DKIX_DUMMY <= ix && ix <= usable); @@ -764,6 +769,7 @@ _PyDict_CheckConsistency(PyObject *op, int check_content) CHECK(mp->ma_values->values[index] != NULL); } } + UNLOCK_KEYS_IF_SPLIT(keys, keys->dk_kind); } return 1; @@ -4032,7 +4038,7 @@ dict_equal_lock_held(PyDictObject *a, PyDictObject *b) /* can't be equal if # of entries differ */ return 0; /* Same # of entries -- check all of 'em. Exit early on any diff. */ - for (i = 0; i < LOAD_KEYS_NENTIRES(a->ma_keys); i++) { + for (i = 0; i < LOAD_KEYS_NENTRIES(a->ma_keys); i++) { PyObject *key, *aval; Py_hash_t hash; if (DK_IS_UNICODE(a->ma_keys)) { diff --git a/Tools/tsan/suppressions_free_threading.txt b/Tools/tsan/suppressions_free_threading.txt index 7ca062456a9b7e..80ffcc12fba0a9 100644 --- a/Tools/tsan/suppressions_free_threading.txt +++ b/Tools/tsan/suppressions_free_threading.txt @@ -27,16 +27,8 @@ race_top:_add_to_weak_set race_top:_in_weak_set race_top:_PyEval_EvalFrameDefault race_top:assign_version_tag -race_top:insertdict -race_top:lookup_tp_dict race_top:new_reference -race_top:_PyDict_CheckConsistency -race_top:_Py_dict_lookup_threadsafe race_top:_multiprocessing_SemLock_acquire_impl -race_top:dictiter_new -race_top:dictresize -race_top:insert_to_emptydict -race_top:insertdict race_top:list_get_item_ref race_top:make_pending_calls race_top:_Py_slot_tp_getattr_hook