diff --git a/CHANGES/1317.bugfix.rst b/CHANGES/1317.bugfix.rst new file mode 100644 index 000000000..82f70ba2a --- /dev/null +++ b/CHANGES/1317.bugfix.rst @@ -0,0 +1,20 @@ +Fixed a memory-safety race condition resulting in segmentation faults +(use-after-free) when iterating and modifying a :class:`~multidict.MultiDict` +concurrently in CPython free-threaded mode (3.13t+). Read and write accesses +to the internal ``md->keys`` buffer are now wrapped in +:c:macro:`Py_BEGIN_CRITICAL_SECTION`. + +Operations that read a second multidict, namely the copy constructors, +:meth:`~multidict.MultiDict.extend`, :meth:`~multidict.MultiDict.update` and +:meth:`~multidict.MultiDict.merge`, now lock both objects with +:c:macro:`Py_BEGIN_CRITICAL_SECTION2`. Previously only the destination was +locked, so a concurrent insertion could resize the source and free the entry +array being walked. + +The consistency checks compiled into debug builds no longer run outside the +critical section they belong to. They walk the whole table, so on a debug +free-threaded build one thread could abort on another thread's transient +state, or read an array a third thread had already freed. Most of them +duplicated a check that the ``md_*`` helpers already perform under the lock +and were removed +-- by :user:`rodrigobnogueira`. diff --git a/multidict/__init__.py b/multidict/__init__.py index 653494fb4..1e33bf876 100644 --- a/multidict/__init__.py +++ b/multidict/__init__.py @@ -35,6 +35,26 @@ getversion, istr, ) + + if not TYPE_CHECKING: + import sys + import warnings + + # ``sys._is_gil_enabled`` is CPython-private, and this branch is taken + # on alternative implementations too (``_compat`` forces it on PyPy), + # so probe for the attribute rather than inferring it from the version. + # A diagnostic must not be able to break the import it diagnoses. + if not getattr(sys, "_is_gil_enabled", lambda: True)(): + warnings.warn( + "The multidict C extension is not in use, either because it " + "is unavailable or because MULTIDICT_NO_EXTENSIONS is set. " + "The pure-Python fallback is not thread-safe under " + "free-threaded CPython (GIL disabled): concurrent mutation " + "can leave a MultiDict internally inconsistent, so confine " + "each instance to one thread.", + RuntimeWarning, + stacklevel=2, + ) else: from collections.abc import ItemsView, KeysView, ValuesView diff --git a/multidict/_multidict.c b/multidict/_multidict.c index 73c002296..dc00fbc87 100644 --- a/multidict/_multidict.c +++ b/multidict/_multidict.c @@ -64,11 +64,35 @@ _multidict_getone(MultiDictObject *self, PyObject *key, PyObject *_default) } } +/* The multidict `arg` reads its items from, or NULL when `arg` is any other + kind of source. md_update_from_ht() and md_clone_from_ht() walk that + object's hash table directly, so the caller has to hold its critical + section; see the locking notes in hashtable.h. Each entry point calls this + once and passes the result down, so the object that gets locked and the + object that gets walked are the same pointer, not merely two reads that + happen to agree. */ +static inline MultiDictObject * +_multidict_extend_source(mod_state *state, PyObject *arg) +{ + if (arg == NULL) { + return NULL; + } + if (AnyMultiDict_Check(state, arg)) { + return (MultiDictObject *)arg; + } + if (AnyMultiDictProxy_Check(state, arg)) { + return ((MultiDictProxyObject *)arg)->md; + } + return NULL; +} + +/* `other` is the result of _multidict_extend_source(state, arg); the caller + has already resolved it to decide which critical section to take, and + passes it in so the type checks are not repeated on this hot path. */ static inline int -_multidict_extend(MultiDictObject *self, PyObject *arg, PyObject *kwds, - const char *name, UpdateOp op) +_multidict_extend(MultiDictObject *self, PyObject *arg, MultiDictObject *other, + PyObject *kwds, const char *name, UpdateOp op) { - mod_state *state = self->state; PyObject *seq = NULL; if (kwds && !PyArg_ValidateKeywordArguments(kwds)) { @@ -76,13 +100,7 @@ _multidict_extend(MultiDictObject *self, PyObject *arg, PyObject *kwds, } if (arg != NULL) { - if (AnyMultiDict_Check(state, arg)) { - MultiDictObject *other = (MultiDictObject *)arg; - if (md_update_from_ht(self, other, op) < 0) { - goto fail; - } - } else if (AnyMultiDictProxy_Check(state, arg)) { - MultiDictObject *other = ((MultiDictProxyObject *)arg)->md; + if (other != NULL) { if (md_update_from_ht(self, other, op) < 0) { goto fail; } @@ -134,6 +152,20 @@ _multidict_extend(MultiDictObject *self, PyObject *arg, PyObject *kwds, return -1; } +/* The body shared by extend(), update() and merge(), always run with `self` + locked and, when the source is another multidict, with that object locked + as well. */ +static inline int +_multidict_extend_locked(MultiDictObject *self, PyObject *arg, + MultiDictObject *other, PyObject *kwds, + const char *name, UpdateOp op, Py_ssize_t size) +{ + if (md_reserve(self, size) < 0) { + return -1; + } + return _multidict_extend(self, arg, other, kwds, name, op); +} + static inline Py_ssize_t _multidict_extend_parse_args(mod_state *state, PyObject *args, PyObject *kwds, const char *name, PyObject **parg) @@ -220,6 +252,26 @@ _multidict_clone_fast(mod_state *state, MultiDictObject *self, bool is_ci, return ret; } +/* The body shared by the MultiDict and CIMultiDict constructors, run under + the same locking rule as _multidict_extend_locked(). */ +static inline int +_multidict_init_locked(MultiDictObject *self, mod_state *state, bool is_ci, + PyObject *args, PyObject *arg, MultiDictObject *other, + PyObject *kwds, const char *name, Py_ssize_t size) +{ + int tmp = _multidict_clone_fast(state, self, is_ci, args, kwds); + if (tmp < 0) { + return -1; + } + if (tmp == 1) { + return 0; + } + if (md_init(self, state, is_ci, size) < 0) { + return -1; + } + return _multidict_extend(self, arg, other, kwds, name, Extend); +} + static inline PyObject * multidict_copy(MultiDictObject *self) { @@ -229,7 +281,11 @@ multidict_copy(MultiDictObject *self) } MultiDictObject *new_md = (MultiDictObject *)ret; - if (md_clone_from_ht(new_md, self) < 0) { + int tmp; + Py_BEGIN_CRITICAL_SECTION(self); + tmp = md_clone_from_ht(new_md, self); + Py_END_CRITICAL_SECTION(); + if (tmp < 0) { goto fail; } ASSERT_CONSISTENT(new_md, false); @@ -264,12 +320,20 @@ multidict_getall(MultiDictObject *self, PyObject *const *args, &_default) < 0) { return NULL; } - if (md_get_all(self, key, &list) < 0) { + int tmp; + Py_BEGIN_CRITICAL_SECTION(self); + tmp = md_get_all(self, key, &list); + if (tmp >= 0) { + // md_get_all() has no consistency check of its own, so keep one + // here. It has to run inside the section: ASSERT_CONSISTENT() + // walks the whole table. + ASSERT_CONSISTENT(self, false); + } + Py_END_CRITICAL_SECTION(); + if (tmp < 0) { return NULL; } - ASSERT_CONSISTENT(self, false); - if (list == NULL) { if (_default != NULL) { Py_INCREF(_default); @@ -300,7 +364,11 @@ multidict_getone(MultiDictObject *self, PyObject *const *args, &_default) < 0) { return NULL; } - return _multidict_getone(self, key, _default); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = _multidict_getone(self, key, _default); + Py_END_CRITICAL_SECTION(); + return ret; } static inline PyObject * @@ -329,8 +397,10 @@ multidict_get(MultiDictObject *self, PyObject *const *args, Py_ssize_t nargs, } decref_default = true; } - ASSERT_CONSISTENT(self, false); - PyObject *ret = _multidict_getone(self, key, _default); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = _multidict_getone(self, key, _default); + Py_END_CRITICAL_SECTION(); if (decref_default) { Py_CLEAR(_default); } @@ -340,19 +410,31 @@ multidict_get(MultiDictObject *self, PyObject *const *args, Py_ssize_t nargs, static PyObject * multidict_keys(MultiDictObject *self) { - return multidict_keysview_new(self); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = multidict_keysview_new(self); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_items(MultiDictObject *self) { - return multidict_itemsview_new(self); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = multidict_itemsview_new(self); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_values(MultiDictObject *self) { - return multidict_valuesview_new(self); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = multidict_valuesview_new(self); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * @@ -400,7 +482,10 @@ multidict_repr(MultiDictObject *self) Py_ReprLeave((PyObject *)self); return NULL; } - PyObject *ret = md_repr(self, name, true, true); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = md_repr(self, name, true, true); + Py_END_CRITICAL_SECTION(); Py_ReprLeave((PyObject *)self); Py_CLEAR(name); return ret; @@ -409,35 +494,55 @@ multidict_repr(MultiDictObject *self) static Py_ssize_t multidict_mp_len(MultiDictObject *self) { - return md_len(self); + Py_ssize_t len; + Py_BEGIN_CRITICAL_SECTION(self); + len = md_len(self); + Py_END_CRITICAL_SECTION(); + return len; } static PyObject * multidict_mp_subscript(MultiDictObject *self, PyObject *key) { - return _multidict_getone(self, key, NULL); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = _multidict_getone(self, key, NULL); + Py_END_CRITICAL_SECTION(); + return ret; } static int multidict_mp_as_subscript(MultiDictObject *self, PyObject *key, PyObject *val) { + int ret; + Py_BEGIN_CRITICAL_SECTION(self); if (val == NULL) { - return md_del(self, key); + ret = md_del(self, key); } else { - return md_replace(self, key, val); + ret = md_replace(self, key, val); } + Py_END_CRITICAL_SECTION(); + return ret; } static int multidict_sq_contains(MultiDictObject *self, PyObject *key) { - return md_contains(self, key, NULL); + int ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = md_contains(self, key, NULL); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_tp_iter(MultiDictObject *self) { - return multidict_keys_iter_new(self); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = multidict_keys_iter_new(self); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * @@ -459,9 +564,14 @@ multidict_tp_richcompare(MultiDictObject *self, PyObject *other, int op) mod_state *state = self->state; if (AnyMultiDict_Check(state, other)) { + Py_BEGIN_CRITICAL_SECTION2(self, other); cmp = md_eq(self, (MultiDictObject *)other); + Py_END_CRITICAL_SECTION2(); } else if (AnyMultiDictProxy_Check(state, other)) { - cmp = md_eq(self, ((MultiDictProxyObject *)other)->md); + MultiDictObject *other_md = ((MultiDictProxyObject *)other)->md; + Py_BEGIN_CRITICAL_SECTION2(self, other_md); + cmp = md_eq(self, other_md); + Py_END_CRITICAL_SECTION2(); } else { bool fits = false; fits = PyDict_Check(other); @@ -476,7 +586,9 @@ multidict_tp_richcompare(MultiDictObject *self, PyObject *other, int op) Py_CLEAR(keys); } if (fits) { + Py_BEGIN_CRITICAL_SECTION(self); cmp = md_eq_to_mapping(self, other); + Py_END_CRITICAL_SECTION(); } else { cmp = 0; // e.g., multidict is not equal to a list } @@ -540,30 +652,27 @@ multidict_tp_init(MultiDictObject *self, PyObject *args, PyObject *kwds) { mod_state *state = get_mod_state_by_def((PyObject *)self); PyObject *arg = NULL; + int ret = -1; Py_ssize_t size = _multidict_extend_parse_args(state, args, kwds, "MultiDict", &arg); if (size < 0) { - goto fail; - } - int tmp = _multidict_clone_fast(state, self, false, args, kwds); - if (tmp < 0) { - goto fail; - } else if (tmp == 1) { goto done; } - if (md_init(self, state, false, size) < 0) { - goto fail; - } - if (_multidict_extend(self, arg, kwds, "MultiDict", Extend) < 0) { - goto fail; + MultiDictObject *other = _multidict_extend_source(state, arg); + if (other != NULL) { + Py_BEGIN_CRITICAL_SECTION2(self, other); + ret = _multidict_init_locked( + self, state, false, args, arg, other, kwds, "MultiDict", size); + Py_END_CRITICAL_SECTION2(); + } else { + Py_BEGIN_CRITICAL_SECTION(self); + ret = _multidict_init_locked( + self, state, false, args, arg, other, kwds, "MultiDict", size); + Py_END_CRITICAL_SECTION(); } done: Py_CLEAR(arg); - ASSERT_CONSISTENT(self, false); - return 0; -fail: - Py_CLEAR(arg); - return -1; + return ret; } static PyObject * @@ -576,10 +685,13 @@ multidict_add(MultiDictObject *self, PyObject *const *args, Py_ssize_t nargs, 0) { return NULL; } - if (md_add(self, key, val) < 0) { + int ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = md_add(self, key, val); + Py_END_CRITICAL_SECTION(); + if (ret < 0) { return NULL; } - ASSERT_CONSISTENT(self, false); Py_RETURN_NONE; } @@ -587,19 +699,28 @@ static PyObject * multidict_extend(MultiDictObject *self, PyObject *args, PyObject *kwds) { PyObject *arg = NULL; + int failed = 0; Py_ssize_t size = _multidict_extend_parse_args(self->state, args, kwds, "extend", &arg); if (size < 0) { goto fail; } - if (md_reserve(self, size) < 0) { - goto fail; - } - if (_multidict_extend(self, arg, kwds, "extend", Extend) < 0) { - goto fail; + MultiDictObject *other = _multidict_extend_source(self->state, arg); + if (other != NULL) { + Py_BEGIN_CRITICAL_SECTION2(self, other); + failed = _multidict_extend_locked( + self, arg, other, kwds, "extend", Extend, size) < 0; + Py_END_CRITICAL_SECTION2(); + } else { + Py_BEGIN_CRITICAL_SECTION(self); + failed = _multidict_extend_locked( + self, arg, other, kwds, "extend", Extend, size) < 0; + Py_END_CRITICAL_SECTION(); } Py_CLEAR(arg); - ASSERT_CONSISTENT(self, false); + if (failed) { + return NULL; + } Py_RETURN_NONE; fail: Py_CLEAR(arg); @@ -609,11 +730,14 @@ multidict_extend(MultiDictObject *self, PyObject *args, PyObject *kwds) static PyObject * multidict_clear(MultiDictObject *self) { - if (md_clear(self) < 0) { + int ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = md_clear(self); + Py_END_CRITICAL_SECTION(); + if (ret < 0) { return NULL; } - ASSERT_CONSISTENT(self, false); Py_RETURN_NONE; } @@ -644,8 +768,11 @@ multidict_setdefault(MultiDictObject *self, PyObject *const *args, } decref_default = true; } - ASSERT_CONSISTENT(self, false); - if (md_set_default(self, key, _default, &ret) < 0) { + int tmp; + Py_BEGIN_CRITICAL_SECTION(self); + tmp = md_set_default(self, key, _default, &ret); + Py_END_CRITICAL_SECTION(); + if (tmp < 0) { return NULL; } if (decref_default) { @@ -671,11 +798,14 @@ multidict_popone(MultiDictObject *self, PyObject *const *args, &_default) < 0) { return NULL; } - if (md_pop_one(self, key, &ret_val) < 0) { + int tmp; + Py_BEGIN_CRITICAL_SECTION(self); + tmp = md_pop_one(self, key, &ret_val); + Py_END_CRITICAL_SECTION(); + if (tmp < 0) { return NULL; } - ASSERT_CONSISTENT(self, false); if (ret_val == NULL) { if (_default != NULL) { Py_INCREF(_default); @@ -706,11 +836,14 @@ multidict_pop(MultiDictObject *self, PyObject *const *args, Py_ssize_t nargs, &_default) < 0) { return NULL; } - if (md_pop_one(self, key, &ret_val) < 0) { + int tmp2; + Py_BEGIN_CRITICAL_SECTION(self); + tmp2 = md_pop_one(self, key, &ret_val); + Py_END_CRITICAL_SECTION(); + if (tmp2 < 0) { return NULL; } - ASSERT_CONSISTENT(self, false); if (ret_val == NULL) { if (_default != NULL) { Py_INCREF(_default); @@ -741,11 +874,14 @@ multidict_popall(MultiDictObject *self, PyObject *const *args, &_default) < 0) { return NULL; } - if (md_pop_all(self, key, &ret_val) < 0) { + int tmp; + Py_BEGIN_CRITICAL_SECTION(self); + tmp = md_pop_all(self, key, &ret_val); + Py_END_CRITICAL_SECTION(); + if (tmp < 0) { return NULL; } - ASSERT_CONSISTENT(self, false); if (ret_val == NULL) { if (_default != NULL) { Py_INCREF(_default); @@ -762,26 +898,39 @@ multidict_popall(MultiDictObject *self, PyObject *const *args, static PyObject * multidict_popitem(MultiDictObject *self) { - return md_pop_item(self); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self); + ret = md_pop_item(self); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_update(MultiDictObject *self, PyObject *args, PyObject *kwds) { PyObject *arg = NULL; + int failed = 0; Py_ssize_t size = _multidict_extend_parse_args(self->state, args, kwds, "update", &arg); if (size < 0) { goto fail; } - if (md_reserve(self, size) < 0) { - goto fail; - } - if (_multidict_extend(self, arg, kwds, "update", Update) < 0) { - goto fail; + MultiDictObject *other = _multidict_extend_source(self->state, arg); + if (other != NULL) { + Py_BEGIN_CRITICAL_SECTION2(self, other); + failed = _multidict_extend_locked( + self, arg, other, kwds, "update", Update, size) < 0; + Py_END_CRITICAL_SECTION2(); + } else { + Py_BEGIN_CRITICAL_SECTION(self); + failed = _multidict_extend_locked( + self, arg, other, kwds, "update", Update, size) < 0; + Py_END_CRITICAL_SECTION(); } Py_CLEAR(arg); - ASSERT_CONSISTENT(self, false); + if (failed) { + return NULL; + } Py_RETURN_NONE; fail: Py_CLEAR(arg); @@ -792,19 +941,28 @@ static PyObject * multidict_merge(MultiDictObject *self, PyObject *args, PyObject *kwds) { PyObject *arg = NULL; + int failed = 0; Py_ssize_t size = _multidict_extend_parse_args(self->state, args, kwds, "merge", &arg); if (size < 0) { goto fail; } - if (md_reserve(self, size) < 0) { - goto fail; - } - if (_multidict_extend(self, arg, kwds, "merge", Merge) < 0) { - goto fail; + MultiDictObject *other = _multidict_extend_source(self->state, arg); + if (other != NULL) { + Py_BEGIN_CRITICAL_SECTION2(self, other); + failed = _multidict_extend_locked( + self, arg, other, kwds, "merge", Merge, size) < 0; + Py_END_CRITICAL_SECTION2(); + } else { + Py_BEGIN_CRITICAL_SECTION(self); + failed = _multidict_extend_locked( + self, arg, other, kwds, "merge", Merge, size) < 0; + Py_END_CRITICAL_SECTION(); } Py_CLEAR(arg); - ASSERT_CONSISTENT(self, false); + if (failed) { + return NULL; + } Py_RETURN_NONE; fail: Py_CLEAR(arg); @@ -858,8 +1016,11 @@ PyDoc_STRVAR(sizeof__doc__, "D.__sizeof__() -> size of D in memory, in bytes"); static PyObject * multidict_sizeof(MultiDictObject *self) { - Py_ssize_t size = sizeof(MultiDictObject); + Py_ssize_t size; + Py_BEGIN_CRITICAL_SECTION(self); + size = sizeof(MultiDictObject); if (self->keys != &empty_htkeys) size += htkeys_sizeof(self->keys); + Py_END_CRITICAL_SECTION(); return PyLong_FromSsize_t(size); } @@ -998,30 +1159,27 @@ cimultidict_tp_init(MultiDictObject *self, PyObject *args, PyObject *kwds) { mod_state *state = get_mod_state_by_def((PyObject *)self); PyObject *arg = NULL; + int ret = -1; Py_ssize_t size = _multidict_extend_parse_args(state, args, kwds, "CIMultiDict", &arg); if (size < 0) { - goto fail; - } - int tmp = _multidict_clone_fast(state, self, true, args, kwds); - if (tmp < 0) { - goto fail; - } else if (tmp == 1) { goto done; } - if (md_init(self, state, true, size) < 0) { - goto fail; - } - if (_multidict_extend(self, arg, kwds, "CIMultiDict", Extend) < 0) { - goto fail; + MultiDictObject *other = _multidict_extend_source(state, arg); + if (other != NULL) { + Py_BEGIN_CRITICAL_SECTION2(self, other); + ret = _multidict_init_locked( + self, state, true, args, arg, other, kwds, "CIMultiDict", size); + Py_END_CRITICAL_SECTION2(); + } else { + Py_BEGIN_CRITICAL_SECTION(self); + ret = _multidict_init_locked( + self, state, true, args, arg, other, kwds, "CIMultiDict", size); + Py_END_CRITICAL_SECTION(); } done: Py_CLEAR(arg); - ASSERT_CONSISTENT(self, false); - return 0; -fail: - Py_CLEAR(arg); - return -1; + return ret; } PyDoc_STRVAR( @@ -1114,19 +1272,31 @@ multidict_proxy_get(MultiDictProxyObject *self, PyObject *const *args, static PyObject * multidict_proxy_keys(MultiDictProxyObject *self) { - return multidict_keysview_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_keysview_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_proxy_items(MultiDictProxyObject *self) { - return multidict_itemsview_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_itemsview_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_proxy_values(MultiDictProxyObject *self) { - return multidict_valuesview_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_valuesview_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * @@ -1147,25 +1317,41 @@ multidict_proxy_reduce(MultiDictProxyObject *self) static Py_ssize_t multidict_proxy_mp_len(MultiDictProxyObject *self) { - return md_len(self->md); + Py_ssize_t ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_len(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_proxy_mp_subscript(MultiDictProxyObject *self, PyObject *key) { - return _multidict_getone(self->md, key, NULL); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = _multidict_getone(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); + return ret; } static int multidict_proxy_sq_contains(MultiDictProxyObject *self, PyObject *key) { - return md_contains(self->md, key, NULL); + int ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_contains(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * multidict_proxy_tp_iter(MultiDictProxyObject *self) { - return multidict_keys_iter_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_keys_iter_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static PyObject * @@ -1206,7 +1392,10 @@ multidict_proxy_repr(MultiDictProxyObject *self) PyObject *name = PyObject_GetAttr((PyObject *)Py_TYPE(self), self->md->state->str_name); if (name == NULL) return NULL; - PyObject *ret = md_repr(self->md, name, true, true); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_repr(self->md, name, true, true); + Py_END_CRITICAL_SECTION(); Py_CLEAR(name); return ret; } diff --git a/multidict/_multilib/hashtable.h b/multidict/_multilib/hashtable.h index f2ba9868a..7106be35c 100644 --- a/multidict/_multilib/hashtable.h +++ b/multidict/_multilib/hashtable.h @@ -68,6 +68,63 @@ indices still has O(1) amortized time, it is ok. in the left and right arguments. `.copy()` and constuction from multidict is super fast. + +Thread Safety (CPython 3.13t+ free-threaded mode) +================================================== + +This module declares Py_MOD_GIL_NOT_USED, opting into free-threaded execution +on CPython 3.13t+. All public entry points that read or write md->keys are +protected by Py_BEGIN_CRITICAL_SECTION(md) / Py_END_CRITICAL_SECTION(), which +acquires CPython's per-object mutex (a no-op on pre-3.13 builds via the +pythoncapi_compat.h shim). + +Locking granularity follows CPython's own dict pattern: each public-facing +method (in _multidict.c, iter.h, views.h) acquires the critical section on +the MultiDictObject. Internal helpers (_md_resize, md_next, _md_add_with_hash, +etc.) remain unlocked — they are always called from within an already-locked +public entry point. + +Operations that read a SECOND multidict must lock both objects with +Py_BEGIN_CRITICAL_SECTION2(self, other), the way CPython's dict_merge() does. +md_update_from_ht(), md_clone_from_ht() and md_eq() all take a raw entry_t* +into other->keys and walk it, so an unlocked concurrent insert on `other` can +resize it and free the array out from under the walk. Because critical +sections are NOT reentrant, the second lock cannot be taken inside those +helpers (the caller already holds `self`); it is taken by the public entry +point instead — the constructors, extend(), update(), merge() and +tp_richcompare. _multidict_extend_source() resolves the argument once, and +that same pointer is both locked and walked, so the object the lock protects +is by construction the object the walk reads. + +ASSERT_CONSISTENT() walks the whole table, so it must only ever be evaluated +while the object's critical section is held. Evaluating it outside is not a +stale read but an unsynchronised walk of a buffer another thread may resize +and free. Most md_* helpers already assert under the lock, so public entry +points do not repeat it; multidict_copy() is the exception and asserts a +freshly built object that is not yet reachable from another thread. + +Known gap: a critical section is SUSPENDED whenever the holding thread +detaches, which includes any blocking call and every stop-the-world safepoint. +It is therefore not enough on its own wherever the table is left in a +transiently violated state across a call into Python. md_find_next() marks +visited entries with entry->hash = -1 and md_finder_cleanup() restores them, +and the value comparisons in views.h run user code inside that window, so +another thread can still observe a marked entry (getall() dropping a value, +for one). Closing that needs the markers restored before each callback, or +the callbacks hoisted out of the marked window the way CPython's dict does; +locking alone does not do it. + +Re-entrancy constraint: Py_BEGIN_CRITICAL_SECTION uses a NON-RECURSIVE mutex. +Several operations call back into Python code while holding the lock: + - _ci_key_to_identity() calls PyObject_CallMethodNoArgs(key, "lower") + - _str_cmp() calls PyUnicode_RichCompare() + - md_repr() calls PyUnicodeWriter_WriteRepr() / PyObject_Repr() +These callbacks are safe because they never re-enter any multidict method on +the SAME MultiDictObject instance. str.lower() returns a new string without +touching the dict, and value __repr__/__eq__ should not mutate the container. +If a user subclass violates this invariant (e.g., a value's __repr__ mutates +the multidict it belongs to), the result is a deadlock on 3.13t+ — the same +constraint that CPython's built-in dict has. */ /* GROWTH_RATE. Growth rate upon hitting maximum load. diff --git a/multidict/_multilib/iter.h b/multidict/_multilib/iter.h index a3227b4be..9e8168d8d 100644 --- a/multidict/_multilib/iter.h +++ b/multidict/_multilib/iter.h @@ -76,7 +76,10 @@ multidict_items_iter_iternext(MultidictIter *self) PyObject *value = NULL; PyObject *ret = NULL; - int res = md_next(self->md, &self->current, NULL, &key, &value); + int res; + Py_BEGIN_CRITICAL_SECTION(self->md); + res = md_next(self->md, &self->current, NULL, &key, &value); + Py_END_CRITICAL_SECTION(); if (res < 0) { return NULL; } @@ -102,7 +105,10 @@ multidict_values_iter_iternext(MultidictIter *self) { PyObject *value = NULL; - int res = md_next(self->md, &self->current, NULL, NULL, &value); + int res; + Py_BEGIN_CRITICAL_SECTION(self->md); + res = md_next(self->md, &self->current, NULL, NULL, &value); + Py_END_CRITICAL_SECTION(); if (res < 0) { return NULL; } @@ -119,7 +125,10 @@ multidict_keys_iter_iternext(MultidictIter *self) { PyObject *key = NULL; - int res = md_next(self->md, &self->current, NULL, &key, NULL); + int res; + Py_BEGIN_CRITICAL_SECTION(self->md); + res = md_next(self->md, &self->current, NULL, &key, NULL); + Py_END_CRITICAL_SECTION(); if (res < 0) { return NULL; } @@ -159,7 +168,11 @@ multidict_iter_clear(MultidictIter *self) static inline PyObject * multidict_iter_len(MultidictIter *self) { - return PyLong_FromLong(md_len(self->md)); + Py_ssize_t len; + Py_BEGIN_CRITICAL_SECTION(self->md); + len = md_len(self->md); + Py_END_CRITICAL_SECTION(); + return PyLong_FromLong(len); } PyDoc_STRVAR(length_hint_doc, diff --git a/multidict/_multilib/views.h b/multidict/_multilib/views.h index d03c3a27f..bbcbc1c45 100644 --- a/multidict/_multilib/views.h +++ b/multidict/_multilib/views.h @@ -56,7 +56,11 @@ multidict_view_clear(_Multidict_ViewObject *self) static inline Py_ssize_t multidict_view_len(_Multidict_ViewObject *self) { - return md_len(self->md); + Py_ssize_t len; + Py_BEGIN_CRITICAL_SECTION(self->md); + len = md_len(self->md); + Py_END_CRITICAL_SECTION(); + return len; } static inline PyObject * @@ -64,7 +68,10 @@ multidict_view_richcompare(_Multidict_ViewObject *self, PyObject *other, int op) { int tmp; - Py_ssize_t self_size = md_len(self->md); + Py_ssize_t self_size; + Py_BEGIN_CRITICAL_SECTION(self->md); + self_size = md_len(self->md); + Py_END_CRITICAL_SECTION(); Py_ssize_t size = PyObject_Length(other); if (size < 0) { PyErr_Clear(); @@ -169,7 +176,11 @@ multidict_itemsview_new(MultiDictObject *md) static inline PyObject * multidict_itemsview_iter(_Multidict_ViewObject *self) { - return multidict_items_iter_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_items_iter_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static inline PyObject * @@ -188,7 +199,10 @@ multidict_itemsview_repr(_Multidict_ViewObject *self) Py_ReprLeave((PyObject *)self); return NULL; } - PyObject *ret = md_repr(self->md, name, true, true); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_repr(self->md, name, true, true); + Py_END_CRITICAL_SECTION(); Py_ReprLeave((PyObject *)self); Py_CLEAR(name); return ret; @@ -260,6 +274,7 @@ multidict_itemsview_and1(_Multidict_ViewObject *self, PyObject *other) PyObject *arg = NULL; PyObject *ret = NULL; md_finder_t finder = {0}; + int failed = 0; PyObject *iter = PyObject_GetIter(other); if (iter == NULL) { @@ -273,11 +288,13 @@ multidict_itemsview_and1(_Multidict_ViewObject *self, PyObject *other) if (ret == NULL) { goto fail; } + Py_BEGIN_CRITICAL_SECTION(self->md); while ((arg = PyIter_Next(iter))) { int tmp = _multidict_itemsview_parse_item( self, arg, &identity, &key, &value); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { Py_CLEAR(arg); continue; @@ -285,24 +302,28 @@ multidict_itemsview_and1(_Multidict_ViewObject *self, PyObject *other) if (md_init_finder(self->md, identity, &finder) < 0) { assert(PyErr_Occurred()); - goto fail; + failed = 1; + goto cs_done; } while ((tmp = md_find_next(&finder, &key2, &value2)) > 0) { tmp = PyObject_RichCompareBool(value, value2, Py_EQ); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } if (tmp > 0) { if (_set_add(ret, key2, value2) < 0) { - goto fail; + failed = 1; + goto cs_done; } } Py_CLEAR(key2); Py_CLEAR(value2); } if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } md_finder_cleanup(&finder); Py_CLEAR(arg); @@ -311,18 +332,26 @@ multidict_itemsview_and1(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(value); } if (PyErr_Occurred()) { - goto fail; + failed = 1; } - Py_CLEAR(iter); - return ret; -fail: - md_finder_cleanup(&finder); +cs_done:; + if (failed) { + md_finder_cleanup(&finder); + Py_CLEAR(key2); + Py_CLEAR(value2); + } + Py_END_CRITICAL_SECTION(); Py_CLEAR(arg); Py_CLEAR(identity); Py_CLEAR(key); - Py_CLEAR(key2); Py_CLEAR(value); - Py_CLEAR(value2); + Py_CLEAR(iter); + if (failed) { + Py_CLEAR(ret); + return NULL; + } + return ret; +fail: Py_CLEAR(iter); Py_CLEAR(ret); return NULL; @@ -338,6 +367,7 @@ multidict_itemsview_and2(_Multidict_ViewObject *self, PyObject *other) PyObject *arg = NULL; PyObject *ret = NULL; md_finder_t finder = {0}; + int failed = 0; PyObject *iter = PyObject_GetIter(other); if (iter == NULL) { @@ -351,11 +381,13 @@ multidict_itemsview_and2(_Multidict_ViewObject *self, PyObject *other) if (ret == NULL) { goto fail; } + Py_BEGIN_CRITICAL_SECTION(self->md); while ((arg = PyIter_Next(iter))) { int tmp = _multidict_itemsview_parse_item( self, arg, &identity, &key, &value); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { Py_CLEAR(arg); continue; @@ -363,23 +395,27 @@ multidict_itemsview_and2(_Multidict_ViewObject *self, PyObject *other) if (md_init_finder(self->md, identity, &finder) < 0) { assert(PyErr_Occurred()); - goto fail; + failed = 1; + goto cs_done; } while ((tmp = md_find_next(&finder, NULL, &value2)) > 0) { tmp = PyObject_RichCompareBool(value, value2, Py_EQ); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } if (tmp > 0) { if (_set_add(ret, key, value2) < 0) { - goto fail; + failed = 1; + goto cs_done; } } Py_CLEAR(value2); } if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } md_finder_cleanup(&finder); Py_CLEAR(arg); @@ -388,17 +424,25 @@ multidict_itemsview_and2(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(value); } if (PyErr_Occurred()) { - goto fail; + failed = 1; } - Py_CLEAR(iter); - return ret; -fail: - md_finder_cleanup(&finder); +cs_done:; + if (failed) { + md_finder_cleanup(&finder); + Py_CLEAR(value2); + } + Py_END_CRITICAL_SECTION(); Py_CLEAR(arg); Py_CLEAR(identity); Py_CLEAR(key); Py_CLEAR(value); - Py_CLEAR(value2); + Py_CLEAR(iter); + if (failed) { + Py_CLEAR(ret); + return NULL; + } + return ret; +fail: Py_CLEAR(iter); Py_CLEAR(ret); return NULL; @@ -438,6 +482,7 @@ multidict_itemsview_or1(_Multidict_ViewObject *self, PyObject *other) PyObject *arg = NULL; PyObject *ret = NULL; md_finder_t finder = {0}; + int failed = 0; PyObject *iter = PyObject_GetIter(other); if (iter == NULL) { @@ -451,14 +496,17 @@ multidict_itemsview_or1(_Multidict_ViewObject *self, PyObject *other) if (ret == NULL) { goto fail; } + Py_BEGIN_CRITICAL_SECTION(self->md); while ((arg = PyIter_Next(iter))) { int tmp = _multidict_itemsview_parse_item( self, arg, &identity, &key, &value); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { if (PySet_Add(ret, arg) < 0) { - goto fail; + failed = 1; + goto cs_done; } Py_CLEAR(arg); continue; @@ -466,13 +514,15 @@ multidict_itemsview_or1(_Multidict_ViewObject *self, PyObject *other) if (md_init_finder(self->md, identity, &finder) < 0) { assert(PyErr_Occurred()); - goto fail; + failed = 1; + goto cs_done; } while ((tmp = md_find_next(&finder, NULL, &value2)) > 0) { tmp = PyObject_RichCompareBool(value, value2, Py_EQ); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } if (tmp > 0) { Py_CLEAR(value2); @@ -481,10 +531,12 @@ multidict_itemsview_or1(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(value2); } if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { if (PySet_Add(ret, arg) < 0) { - goto fail; + failed = 1; + goto cs_done; } } md_finder_cleanup(&finder); @@ -494,17 +546,25 @@ multidict_itemsview_or1(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(value); } if (PyErr_Occurred()) { - goto fail; + failed = 1; } - Py_CLEAR(iter); - return ret; -fail: - md_finder_cleanup(&finder); +cs_done:; + if (failed) { + md_finder_cleanup(&finder); + Py_CLEAR(value2); + } + Py_END_CRITICAL_SECTION(); Py_CLEAR(arg); Py_CLEAR(identity); Py_CLEAR(key); Py_CLEAR(value); - Py_CLEAR(value2); + Py_CLEAR(iter); + if (failed) { + Py_CLEAR(ret); + return NULL; + } + return ret; +fail: Py_CLEAR(iter); Py_CLEAR(ret); return NULL; @@ -555,10 +615,15 @@ multidict_itemsview_or2(_Multidict_ViewObject *self, PyObject *other) } Py_CLEAR(iter); + Py_BEGIN_CRITICAL_SECTION(self->md); md_init_pos(self->md, &pos); + Py_END_CRITICAL_SECTION(); while (true) { - int tmp = md_next(self->md, &pos, &identity, &key, &value); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_next(self->md, &pos, &identity, &key, &value); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } else if (tmp == 0) { @@ -664,10 +729,15 @@ multidict_itemsview_sub1(_Multidict_ViewObject *self, PyObject *other) } Py_CLEAR(iter); + Py_BEGIN_CRITICAL_SECTION(self->md); md_init_pos(self->md, &pos); + Py_END_CRITICAL_SECTION(); while (true) { - int tmp = md_next(self->md, &pos, &identity, &key, &value); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_next(self->md, &pos, &identity, &key, &value); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } else if (tmp == 0) { @@ -715,6 +785,7 @@ multidict_itemsview_sub2(_Multidict_ViewObject *self, PyObject *other) PyObject *ret = NULL; PyObject *iter = PyObject_GetIter(other); md_finder_t finder = {0}; + int failed = 0; if (iter == NULL) { if (PyErr_ExceptionMatches(PyExc_TypeError)) { @@ -727,14 +798,17 @@ multidict_itemsview_sub2(_Multidict_ViewObject *self, PyObject *other) if (ret == NULL) { goto fail; } + Py_BEGIN_CRITICAL_SECTION(self->md); while ((arg = PyIter_Next(iter))) { int tmp = _multidict_itemsview_parse_item( self, arg, &identity, NULL, &value); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { if (PySet_Add(ret, arg) < 0) { - goto fail; + failed = 1; + goto cs_done; } Py_CLEAR(arg); continue; @@ -742,13 +816,15 @@ multidict_itemsview_sub2(_Multidict_ViewObject *self, PyObject *other) if (md_init_finder(self->md, identity, &finder) < 0) { assert(PyErr_Occurred()); - goto fail; + failed = 1; + goto cs_done; } while ((tmp = md_find_next(&finder, NULL, &value2)) > 0) { tmp = PyObject_RichCompareBool(value, value2, Py_EQ); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } if (tmp > 0) { Py_CLEAR(value2); @@ -757,10 +833,12 @@ multidict_itemsview_sub2(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(value2); } if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { if (PySet_Add(ret, arg) < 0) { - goto fail; + failed = 1; + goto cs_done; } } md_finder_cleanup(&finder); @@ -770,16 +848,25 @@ multidict_itemsview_sub2(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(value); } if (PyErr_Occurred()) { - goto fail; + failed = 1; } - Py_CLEAR(iter); - return ret; -fail: - md_finder_cleanup(&finder); +cs_done:; + if (failed) { + md_finder_cleanup(&finder); + Py_CLEAR(value2); + } + Py_END_CRITICAL_SECTION(); Py_CLEAR(arg); Py_CLEAR(identity); Py_CLEAR(key); Py_CLEAR(value); + Py_CLEAR(iter); + if (failed) { + Py_CLEAR(ret); + return NULL; + } + return ret; +fail: Py_CLEAR(iter); Py_CLEAR(ret); return NULL; @@ -918,10 +1005,12 @@ multidict_itemsview_contains(_Multidict_ViewObject *self, PyObject *obj) goto done; } + Py_BEGIN_CRITICAL_SECTION(self->md); + if (md_init_finder(self->md, identity, &finder) < 0) { assert(PyErr_Occurred()); ret = -1; - goto done; + goto cs_done; } while ((tmp = md_find_next(&finder, NULL, &value2)) > 0) { @@ -929,24 +1018,26 @@ multidict_itemsview_contains(_Multidict_ViewObject *self, PyObject *obj) Py_CLEAR(value2); if (tmp < 0) { ret = -1; - goto done; + goto cs_done; } if (tmp > 0) { ret = 1; - goto done; + goto cs_done; } } if (tmp < 0) { ret = -1; - goto done; + goto cs_done; } -done: +cs_done:; md_finder_cleanup(&finder); + ASSERT_CONSISTENT(self->md, false); + Py_END_CRITICAL_SECTION(); +done: Py_CLEAR(identity); Py_CLEAR(key); Py_CLEAR(value); - ASSERT_CONSISTENT(self->md, false); return ret; } @@ -962,12 +1053,16 @@ multidict_itemsview_isdisjoint(_Multidict_ViewObject *self, PyObject *other) PyObject *identity = NULL; PyObject *value = NULL; PyObject *value2 = NULL; + int failed = 0; + int disjoint = 1; + Py_BEGIN_CRITICAL_SECTION(self->md); while ((arg = PyIter_Next(iter))) { int tmp = _multidict_itemsview_parse_item( self, arg, &identity, NULL, &value); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } else if (tmp == 0) { Py_CLEAR(arg); continue; @@ -975,47 +1070,52 @@ multidict_itemsview_isdisjoint(_Multidict_ViewObject *self, PyObject *other) if (md_init_finder(self->md, identity, &finder) < 0) { assert(PyErr_Occurred()); - goto fail; + failed = 1; + goto cs_done; } while ((tmp = md_find_next(&finder, NULL, &value2)) > 0) { tmp = PyObject_RichCompareBool(value, value2, Py_EQ); Py_CLEAR(value2); if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } if (tmp > 0) { - md_finder_cleanup(&finder); - Py_CLEAR(iter); - Py_CLEAR(arg); - Py_CLEAR(identity); - Py_CLEAR(value); - ASSERT_CONSISTENT(self->md, false); - Py_RETURN_FALSE; + disjoint = 0; + goto cs_done; } } if (tmp < 0) { - goto fail; + failed = 1; + goto cs_done; } md_finder_cleanup(&finder); Py_CLEAR(arg); Py_CLEAR(identity); Py_CLEAR(value); } - Py_CLEAR(iter); if (PyErr_Occurred()) { - return NULL; + failed = 1; } - ASSERT_CONSISTENT(self->md, false); - Py_RETURN_TRUE; -fail: +cs_done:; md_finder_cleanup(&finder); + ASSERT_CONSISTENT(self->md, false); + Py_END_CRITICAL_SECTION(); Py_CLEAR(iter); Py_CLEAR(arg); Py_CLEAR(identity); Py_CLEAR(value); - Py_CLEAR(value2); - return NULL; + + if (failed) { + return NULL; + } + + if (disjoint) { + Py_RETURN_TRUE; + } else { + Py_RETURN_FALSE; + } } PyDoc_STRVAR(itemsview_isdisjoint_doc, @@ -1090,7 +1190,11 @@ multidict_keysview_new(MultiDictObject *md) static inline PyObject * multidict_keysview_iter(_Multidict_ViewObject *self) { - return multidict_keys_iter_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_keys_iter_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static inline PyObject * @@ -1101,7 +1205,10 @@ multidict_keysview_repr(_Multidict_ViewObject *self) if (name == NULL) { return NULL; } - PyObject *ret = md_repr(self->md, name, true, false); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_repr(self->md, name, true, false); + Py_END_CRITICAL_SECTION(); Py_CLEAR(name); return ret; } @@ -1129,7 +1236,10 @@ multidict_keysview_and1(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(key); continue; } - int tmp = md_contains(self->md, key, &key2); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_contains(self->md, key, &key2); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } @@ -1176,7 +1286,10 @@ multidict_keysview_and2(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(key); continue; } - int tmp = md_contains(self->md, key, NULL); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_contains(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } @@ -1248,7 +1361,10 @@ multidict_keysview_or1(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(key); continue; } - int tmp = md_contains(self->md, key, NULL); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_contains(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } @@ -1315,10 +1431,15 @@ multidict_keysview_or2(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(iter); md_pos_t pos; + Py_BEGIN_CRITICAL_SECTION(self->md); md_init_pos(self->md, &pos); + Py_END_CRITICAL_SECTION(); while (true) { - int tmp = md_next(self->md, &pos, &identity, &key, NULL); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_next(self->md, &pos, &identity, &key, NULL); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } else if (tmp == 0) { @@ -1396,7 +1517,9 @@ multidict_keysview_sub1(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(key); continue; } + Py_BEGIN_CRITICAL_SECTION(self->md); tmp = md_contains(self->md, key, &key2); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } @@ -1444,7 +1567,9 @@ multidict_keysview_sub2(_Multidict_ViewObject *self, PyObject *other) Py_CLEAR(key); continue; } + Py_BEGIN_CRITICAL_SECTION(self->md); tmp = md_contains(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); if (tmp < 0) { goto fail; } @@ -1554,7 +1679,11 @@ multidict_keysview_xor(_Multidict_ViewObject *self, PyObject *other) static inline int multidict_keysview_contains(_Multidict_ViewObject *self, PyObject *key) { - return md_contains(self->md, key, NULL); + int ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_contains(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); + return ret; } static inline PyObject * @@ -1566,7 +1695,10 @@ multidict_keysview_isdisjoint(_Multidict_ViewObject *self, PyObject *other) } PyObject *key = NULL; while ((key = PyIter_Next(iter))) { - int tmp = md_contains(self->md, key, NULL); + int tmp; + Py_BEGIN_CRITICAL_SECTION(self->md); + tmp = md_contains(self->md, key, NULL); + Py_END_CRITICAL_SECTION(); Py_CLEAR(key); if (tmp < 0) { Py_CLEAR(iter); @@ -1646,7 +1778,11 @@ multidict_valuesview_new(MultiDictObject *md) static inline PyObject * multidict_valuesview_iter(_Multidict_ViewObject *self) { - return multidict_values_iter_new(self->md); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = multidict_values_iter_new(self->md); + Py_END_CRITICAL_SECTION(); + return ret; } static inline PyObject * @@ -1665,7 +1801,10 @@ multidict_valuesview_repr(_Multidict_ViewObject *self) Py_ReprLeave((PyObject *)self); return NULL; } - PyObject *ret = md_repr(self->md, name, false, true); + PyObject *ret; + Py_BEGIN_CRITICAL_SECTION(self->md); + ret = md_repr(self->md, name, false, true); + Py_END_CRITICAL_SECTION(); Py_ReprLeave((PyObject *)self); Py_CLEAR(name); return ret; diff --git a/tests/test_free_threading.py b/tests/test_free_threading.py new file mode 100644 index 000000000..4186f4705 --- /dev/null +++ b/tests/test_free_threading.py @@ -0,0 +1,182 @@ +from __future__ import annotations + +import os +import subprocess +import sys +import threading +import traceback + +import pytest + +from multidict import CIMultiDict, MultiDict, MutableMultiMapping + + +@pytest.mark.c_extension +def test_race_condition_iterator_vs_mutation( + any_multidict_class: type[CIMultiDict[str] | MultiDict[str]], +) -> None: + """Test that concurrent iterations and mutations do not cause a memory safety violation. + + This test specifically triggers use-after-free scenarios if the underlying C extension + hash table ``md->keys`` resizes concurrently during an unresolved iteration sequence. + Under free-threaded CPython (GIL disabled), this previously resulted in a SIGSEGV. + + With the issue fixed, the code securely catches size mutations and cleanly raises + a standard Python ``RuntimeError`` ('MultiDict is changed during iteration'), preventing + crashes. + """ + if getattr(any_multidict_class, "__module__", "").endswith("_multidict_py"): + pytest.skip("Test is only applicable to the C extension") + + md: MutableMultiMapping[str] = any_multidict_class() + for i in range(8): + md[f"init-{i}"] = f"v{i}" + + errors: list[tuple[str, int, str, str, str]] = [] + + def writer(target: MutableMultiMapping[str]) -> None: + for i in range(256): + try: + target[f"k-{i % 64}"] = f"v{i}" + # add() and popone() reach md_add() and md_pop_one(), whose + # entry points used to run ASSERT_CONSISTENT() outside their + # critical section. + target.add(f"k-{i % 64}", f"a{i}") + target.popone(f"k-{i % 64}", None) + target.setdefault(f"k-{i % 64}", f"d{i}") + except RuntimeError: # pragma: no cover + # "MultiDict changed during iteration" is expected under contention + pass + except Exception as e: # pragma: no cover + errors.append( + ("writer", i, type(e).__name__, str(e), traceback.format_exc()) + ) + + def reader(target: MutableMultiMapping[str]) -> None: + for i in range(256): + try: + list(target.items()) + list(target.keys()) + list(target.values()) + # getall()/get() walk the table with a finder open, which is + # what makes an unlocked consistency check observable. + target.getall(f"k-{i % 64}", None) + target.get(f"k-{i % 64}", None) + except RuntimeError: + # "MultiDict changed during iteration" is exactly the expected + # and memory-safe outcome when iterating a resizing dictionary. + pass + except Exception as e: # pragma: no cover + errors.append(("reader", i, type(e).__name__, str(e), "")) + + threads = [ + threading.Thread(target=f, args=(md,)) for f in [writer, reader, writer, reader] + ] + + for t in threads: + t.start() + for t in threads: + t.join() + + # The test passes if it survives without a segmentation fault (SIGSEGV/SIGABRT). + # If the C-extension is thread-safe, no Python exceptions other than RuntimeError + # (handled above) should inadvertently surface to the user. + assert not errors, f"Unexpected errors during concurrent execution: {errors}" + + +@pytest.mark.c_extension +def test_race_condition_extend_vs_source_mutation( + any_multidict_class: type[CIMultiDict[str] | MultiDict[str]], +) -> None: + """Test that reading a second multidict is safe while that one mutates. + + ``md_update_from_ht()`` takes a raw ``entry_t *`` into the *source* + multidict's table and walks it. The destination is locked by the calling + entry point, but before this fix the source never was, so a concurrent + insert could resize it and free the array mid-walk. Under free-threaded + CPython that was a reliable SIGSEGV; the fix locks both objects with + ``Py_BEGIN_CRITICAL_SECTION2``, which makes each walk atomic against the + mutating thread and leaves no window to observe a torn table. + """ + if getattr(any_multidict_class, "__module__", "").endswith("_multidict_py"): + pytest.skip("Test is only applicable to the C extension") + + extenders, mutators = 8, 3 + source: MutableMultiMapping[str] = any_multidict_class( + [(f"init-{i}", f"v{i}") for i in range(64)] + ) + sizes: list[int] = [] + stop = threading.Event() + # Start every thread together; without this the extenders can finish + # before a mutator has resized anything and the race never opens. + ready = threading.Barrier(extenders + mutators) + + def extender() -> None: + ready.wait() + for _ in range(200): + # Every entry point that reads a second multidict: the copy + # constructor, extend(), update() and merge(). + dst = any_multidict_class() + dst.extend(source) + dst.update(source) + dst.merge(source) + sizes.append(len(any_multidict_class(source))) + + def mutator(tag: str) -> None: + # A private key namespace per thread, so every delete succeeds and the + # loop needs no exception handling. Growing well past the load factor + # and shrinking back forces repeated _md_resize() calls on the source, + # which is what frees the array being walked. + ready.wait() + while not stop.is_set(): + for i in range(256): + source[f"{tag}-{i}"] = str(i) + for i in range(256): + del source[f"{tag}-{i}"] + + extender_threads = [threading.Thread(target=extender) for _ in range(extenders)] + mutator_threads = [ + threading.Thread(target=mutator, args=(f"grow{n}",)) for n in range(mutators) + ] + + for t in extender_threads + mutator_threads: + t.start() + # Only stop resizing once every extender is done, so the source keeps + # being reshaped underneath all of them and not just the slowest few. + for t in extender_threads: + t.join() + stop.set() + for t in mutator_threads: + t.join() + + # Surviving without SIGSEGV is the point of the test. The sizes are a + # cheap consistency check: the 64 seeded keys are never removed, so every + # snapshot must have seen at least those. + assert sizes + assert min(sizes) >= 64 + + +@pytest.mark.skipif( + sys.version_info < (3, 13), + reason="Free-threaded CPython warning requires Python 3.13+", +) +def test_pure_python_free_threaded_warning() -> None: + """Test that a RuntimeWarning is emitted on free-threaded CPython without C ext.""" + script = ( + "import sys\n" + "sys._is_gil_enabled = lambda: False\n" + "import warnings\n" + "with warnings.catch_warnings(record=True) as w:\n" + " warnings.simplefilter('always')\n" + " import multidict\n" + "msgs = [str(x.message) for x in w if issubclass(x.category, RuntimeWarning)]\n" + "assert any('not thread-safe' in m for m in msgs), " + "f'Expected thread-safety warning, got: {msgs}'\n" + ) + result = subprocess.run( + [sys.executable, "-c", script], + env={**os.environ, "MULTIDICT_NO_EXTENSIONS": "1"}, + capture_output=True, + text=True, + ) + assert result.returncode == 0, result.stderr