From ff9e71e88a369a5e59fa4fea19d33172c100c7d6 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Sat, 11 Apr 2026 13:29:06 -0400 Subject: [PATCH] gh-148393: Use atomic ops on _ma_watcher_tag in free threading build Fixes data races between dict mutation and watch/unwatch on the same dict. --- Include/internal/pycore_dict.h | 4 ++-- .../internal/pycore_pyatomic_ft_wrappers.h | 12 ++++++++++ Lib/test/test_free_threading/test_dict.py | 23 +++++++++++++++++++ ...-04-11-17-28-52.gh-issue-148393.lX6gwN.rst | 2 ++ Objects/dictobject.c | 4 ++-- Python/optimizer_analysis.c | 5 ++-- 6 files changed, 44 insertions(+), 6 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-04-11-17-28-52.gh-issue-148393.lX6gwN.rst diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index d58539fa846563..5bbea187394db6 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -292,7 +292,7 @@ _PyDict_NotifyEvent(PyDict_WatchEvent event, PyObject *value) { assert(Py_REFCNT((PyObject*)mp) > 0); - int watcher_bits = mp->_ma_watcher_tag & DICT_WATCHER_MASK; + int watcher_bits = FT_ATOMIC_LOAD_UINT64_RELAXED(mp->_ma_watcher_tag) & DICT_WATCHER_MASK; if (watcher_bits) { RARE_EVENT_STAT_INC(watched_dict_modification); _PyDict_SendEvent(watcher_bits, event, mp, key, value); @@ -368,7 +368,7 @@ PyDictObject *_PyObject_MaterializeManagedDict_LockHeld(PyObject *); static inline Py_ssize_t _PyDict_UniqueId(PyDictObject *mp) { - return (Py_ssize_t)(mp->_ma_watcher_tag >> DICT_UNIQUE_ID_SHIFT); + return (Py_ssize_t)(FT_ATOMIC_LOAD_UINT64_RELAXED(mp->_ma_watcher_tag) >> DICT_UNIQUE_ID_SHIFT); } static inline void diff --git a/Include/internal/pycore_pyatomic_ft_wrappers.h b/Include/internal/pycore_pyatomic_ft_wrappers.h index c0f859a23e10b8..3155481bb5c36b 100644 --- a/Include/internal/pycore_pyatomic_ft_wrappers.h +++ b/Include/internal/pycore_pyatomic_ft_wrappers.h @@ -49,6 +49,8 @@ extern "C" { _Py_atomic_load_uint16_relaxed(&value) #define FT_ATOMIC_LOAD_UINT32_RELAXED(value) \ _Py_atomic_load_uint32_relaxed(&value) +#define FT_ATOMIC_LOAD_UINT64_RELAXED(value) \ + _Py_atomic_load_uint64_relaxed(&value) #define FT_ATOMIC_LOAD_ULONG_RELAXED(value) \ _Py_atomic_load_ulong_relaxed(&value) #define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) \ @@ -71,6 +73,12 @@ extern "C" { _Py_atomic_store_uint16_relaxed(&value, new_value) #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) \ _Py_atomic_store_uint32_relaxed(&value, new_value) +#define FT_ATOMIC_AND_UINT64(value, new_value) \ + (void)_Py_atomic_and_uint64(&value, new_value) +#define FT_ATOMIC_OR_UINT64(value, new_value) \ + (void)_Py_atomic_or_uint64(&value, new_value) +#define FT_ATOMIC_ADD_UINT64(value, new_value) \ + (void)_Py_atomic_add_uint64(&value, new_value) #define FT_ATOMIC_STORE_CHAR_RELAXED(value, new_value) \ _Py_atomic_store_char_relaxed(&value, new_value) #define FT_ATOMIC_LOAD_CHAR_RELAXED(value) \ @@ -146,6 +154,7 @@ extern "C" { #define FT_ATOMIC_LOAD_UINT8_RELAXED(value) value #define FT_ATOMIC_LOAD_UINT16_RELAXED(value) value #define FT_ATOMIC_LOAD_UINT32_RELAXED(value) value +#define FT_ATOMIC_LOAD_UINT64_RELAXED(value) value #define FT_ATOMIC_LOAD_ULONG_RELAXED(value) value #define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) value = new_value @@ -157,6 +166,9 @@ extern "C" { #define FT_ATOMIC_STORE_UINT8_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_UINT16_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_STORE_UINT32_RELAXED(value, new_value) value = new_value +#define FT_ATOMIC_AND_UINT64(value, new_value) (void)(value &= new_value) +#define FT_ATOMIC_OR_UINT64(value, new_value) (void)(value |= new_value) +#define FT_ATOMIC_ADD_UINT64(value, new_value) (void)(value += new_value) #define FT_ATOMIC_LOAD_CHAR_RELAXED(value) value #define FT_ATOMIC_STORE_CHAR_RELAXED(value, new_value) value = new_value #define FT_ATOMIC_LOAD_UCHAR_RELAXED(value) value diff --git a/Lib/test/test_free_threading/test_dict.py b/Lib/test/test_free_threading/test_dict.py index 1ffd924e9f477a..55272a00c3ad50 100644 --- a/Lib/test/test_free_threading/test_dict.py +++ b/Lib/test/test_free_threading/test_dict.py @@ -245,6 +245,29 @@ def reader(): with threading_helper.start_threads([t1, t2]): pass + @unittest.skipIf(_testcapi is None, "requires _testcapi") + def test_racing_watch_unwatch_dict(self): + # gh-148393: race between PyDict_Watch / PyDict_Unwatch + # and concurrent dict mutation reading _ma_watcher_tag. + wid = _testcapi.add_dict_watcher(0) + try: + d = {} + ITERS = 1000 + + def writer(): + for i in range(ITERS): + d[i] = i + del d[i] + + def watcher(): + for _ in range(ITERS): + _testcapi.watch_dict(wid, d) + _testcapi.unwatch_dict(wid, d) + + threading_helper.run_concurrently([writer, watcher]) + finally: + _testcapi.clear_dict_watcher(wid) + def test_racing_dict_update_and_method_lookup(self): # gh-144295: test race between dict modifications and method lookups. # Uses BytesIO because the race requires a type without Py_TPFLAGS_INLINE_VALUES diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-04-11-17-28-52.gh-issue-148393.lX6gwN.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-11-17-28-52.gh-issue-148393.lX6gwN.rst new file mode 100644 index 00000000000000..33c4b75bfb944c --- /dev/null +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-04-11-17-28-52.gh-issue-148393.lX6gwN.rst @@ -0,0 +1,2 @@ +Fix data races between :c:func:`PyDict_Watch` / :c:func:`PyDict_Unwatch` +and concurrent dict mutation in the :term:`free-threaded build`. diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 67bc4319e0bae2..b5300eb410c69c 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -8028,7 +8028,7 @@ PyDict_Watch(int watcher_id, PyObject* dict) if (validate_watcher_id(interp, watcher_id)) { return -1; } - ((PyDictObject*)dict)->_ma_watcher_tag |= (1LL << watcher_id); + FT_ATOMIC_OR_UINT64(((PyDictObject*)dict)->_ma_watcher_tag, (1LL << watcher_id)); return 0; } @@ -8043,7 +8043,7 @@ PyDict_Unwatch(int watcher_id, PyObject* dict) if (validate_watcher_id(interp, watcher_id)) { return -1; } - ((PyDictObject*)dict)->_ma_watcher_tag &= ~(1LL << watcher_id); + FT_ATOMIC_AND_UINT64(((PyDictObject*)dict)->_ma_watcher_tag, ~(1LL << watcher_id)); return 0; } diff --git a/Python/optimizer_analysis.c b/Python/optimizer_analysis.c index ca9bcc8a40c35e..6742488a0d06c2 100644 --- a/Python/optimizer_analysis.c +++ b/Python/optimizer_analysis.c @@ -119,14 +119,15 @@ static int get_mutations(PyObject* dict) { assert(PyDict_CheckExact(dict)); PyDictObject *d = (PyDictObject *)dict; - return (d->_ma_watcher_tag >> DICT_MAX_WATCHERS) & ((1 << DICT_WATCHED_MUTATION_BITS)-1); + uint64_t tag = FT_ATOMIC_LOAD_UINT64_RELAXED(d->_ma_watcher_tag); + return (tag >> DICT_MAX_WATCHERS) & ((1 << DICT_WATCHED_MUTATION_BITS) - 1); } static void increment_mutations(PyObject* dict) { assert(PyDict_CheckExact(dict)); PyDictObject *d = (PyDictObject *)dict; - d->_ma_watcher_tag += (1 << DICT_MAX_WATCHERS); + FT_ATOMIC_ADD_UINT64(d->_ma_watcher_tag, (1 << DICT_MAX_WATCHERS)); } /* The first two dict watcher IDs are reserved for CPython,