From c815827dcbbbc36023cc78a97942e545457a739d Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Thu, 8 May 2025 16:22:21 +0530 Subject: [PATCH 1/5] fix thread safety of ordered dict --- Include/internal/pycore_dict.h | 6 + Objects/clinic/odictobject.c.h | 106 +++++++++++++++++- Objects/dictobject.c | 16 +-- Objects/odictobject.c | 194 +++++++++++++++++++++++---------- 4 files changed, 258 insertions(+), 64 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index 754eb88a85c33c..d62955e1c1d8f2 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -30,6 +30,10 @@ PyAPI_FUNC(int) _PyDict_SetItem_KnownHash(PyObject *mp, PyObject *key, // Export for '_asyncio' shared extension PyAPI_FUNC(int) _PyDict_DelItem_KnownHash(PyObject *mp, PyObject *key, Py_hash_t hash); + +extern int _PyDict_DelItem_KnownHash_LockHeld(PyObject *mp, PyObject *key, + Py_hash_t hash); + extern int _PyDict_Contains_KnownHash(PyObject *, PyObject *, Py_hash_t); // "Id" variants @@ -47,6 +51,8 @@ extern int _PyDict_HasOnlyStringKeys(PyObject *mp); // Export for '_ctypes' shared extension PyAPI_FUNC(Py_ssize_t) _PyDict_SizeOf(PyDictObject *); +extern Py_ssize_t _PyDict_SizeOf_LockHeld(PyDictObject *); + #define _PyDict_HasSplitTable(d) ((d)->ma_values != NULL) /* Like PyDict_Merge, but override can be 0, 1 or 2. If override is 0, diff --git a/Objects/clinic/odictobject.c.h b/Objects/clinic/odictobject.c.h index e71c29a1b268d0..894e9be91bbdce 100644 --- a/Objects/clinic/odictobject.c.h +++ b/Objects/clinic/odictobject.c.h @@ -6,6 +6,7 @@ preserve # include "pycore_gc.h" // PyGC_Head # include "pycore_runtime.h" // _Py_ID() #endif +#include "pycore_critical_section.h"// Py_BEGIN_CRITICAL_SECTION() #include "pycore_modsupport.h" // _PyArg_UnpackKeywords() PyDoc_STRVAR(OrderedDict_fromkeys__doc__, @@ -73,6 +74,53 @@ OrderedDict_fromkeys(PyObject *type, PyObject *const *args, Py_ssize_t nargs, Py return return_value; } +PyDoc_STRVAR(OrderedDict___sizeof____doc__, +"__sizeof__($self, /)\n" +"--\n" +"\n"); + +#define ORDEREDDICT___SIZEOF___METHODDEF \ + {"__sizeof__", (PyCFunction)OrderedDict___sizeof__, METH_NOARGS, OrderedDict___sizeof____doc__}, + +static Py_ssize_t +OrderedDict___sizeof___impl(PyODictObject *self); + +static PyObject * +OrderedDict___sizeof__(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + PyObject *return_value = NULL; + Py_ssize_t _return_value; + + Py_BEGIN_CRITICAL_SECTION(self); + _return_value = OrderedDict___sizeof___impl((PyODictObject *)self); + Py_END_CRITICAL_SECTION(); + if ((_return_value == -1) && PyErr_Occurred()) { + goto exit; + } + return_value = PyLong_FromSsize_t(_return_value); + +exit: + return return_value; +} + +PyDoc_STRVAR(OrderedDict___reduce____doc__, +"__reduce__($self, /)\n" +"--\n" +"\n" +"Return state information for pickling"); + +#define ORDEREDDICT___REDUCE___METHODDEF \ + {"__reduce__", (PyCFunction)OrderedDict___reduce__, METH_NOARGS, OrderedDict___reduce____doc__}, + +static PyObject * +OrderedDict___reduce___impl(PyODictObject *od); + +static PyObject * +OrderedDict___reduce__(PyObject *od, PyObject *Py_UNUSED(ignored)) +{ + return OrderedDict___reduce___impl((PyODictObject *)od); +} + PyDoc_STRVAR(OrderedDict_setdefault__doc__, "setdefault($self, /, key, default=None)\n" "--\n" @@ -135,7 +183,9 @@ OrderedDict_setdefault(PyObject *self, PyObject *const *args, Py_ssize_t nargs, } default_value = args[1]; skip_optional_pos: + Py_BEGIN_CRITICAL_SECTION(self); return_value = OrderedDict_setdefault_impl((PyODictObject *)self, key, default_value); + Py_END_CRITICAL_SECTION(); exit: return return_value; @@ -204,7 +254,9 @@ OrderedDict_pop(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyObjec } default_value = args[1]; skip_optional_pos: + Py_BEGIN_CRITICAL_SECTION(self); return_value = OrderedDict_pop_impl((PyODictObject *)self, key, default_value); + Py_END_CRITICAL_SECTION(); exit: return return_value; @@ -272,12 +324,62 @@ OrderedDict_popitem(PyObject *self, PyObject *const *args, Py_ssize_t nargs, PyO goto exit; } skip_optional_pos: + Py_BEGIN_CRITICAL_SECTION(self); return_value = OrderedDict_popitem_impl((PyODictObject *)self, last); + Py_END_CRITICAL_SECTION(); exit: return return_value; } +PyDoc_STRVAR(OrderedDict_clear__doc__, +"clear($self, /)\n" +"--\n" +"\n" +"Remove all items from ordered dict."); + +#define ORDEREDDICT_CLEAR_METHODDEF \ + {"clear", (PyCFunction)OrderedDict_clear, METH_NOARGS, OrderedDict_clear__doc__}, + +static PyObject * +OrderedDict_clear_impl(PyODictObject *self); + +static PyObject * +OrderedDict_clear(PyObject *self, PyObject *Py_UNUSED(ignored)) +{ + PyObject *return_value = NULL; + + Py_BEGIN_CRITICAL_SECTION(self); + return_value = OrderedDict_clear_impl((PyODictObject *)self); + Py_END_CRITICAL_SECTION(); + + return return_value; +} + +PyDoc_STRVAR(OrderedDict_copy__doc__, +"copy($self, /)\n" +"--\n" +"\n" +"A shallow copy of ordered dict."); + +#define ORDEREDDICT_COPY_METHODDEF \ + {"copy", (PyCFunction)OrderedDict_copy, METH_NOARGS, OrderedDict_copy__doc__}, + +static PyObject * +OrderedDict_copy_impl(PyObject *od); + +static PyObject * +OrderedDict_copy(PyObject *od, PyObject *Py_UNUSED(ignored)) +{ + PyObject *return_value = NULL; + + Py_BEGIN_CRITICAL_SECTION(od); + return_value = OrderedDict_copy_impl(od); + Py_END_CRITICAL_SECTION(); + + return return_value; +} + PyDoc_STRVAR(OrderedDict_move_to_end__doc__, "move_to_end($self, /, key, last=True)\n" "--\n" @@ -342,9 +444,11 @@ OrderedDict_move_to_end(PyObject *self, PyObject *const *args, Py_ssize_t nargs, goto exit; } skip_optional_pos: + Py_BEGIN_CRITICAL_SECTION(self); return_value = OrderedDict_move_to_end_impl((PyODictObject *)self, key, last); + Py_END_CRITICAL_SECTION(); exit: return return_value; } -/*[clinic end generated code: output=7d8206823bb1f419 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=7bc997ca7900f06f input=a9049054013a1b77]*/ diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 59b0cf1ce7d422..2dd8de55f4ff83 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2770,8 +2770,8 @@ PyDict_DelItem(PyObject *op, PyObject *key) return _PyDict_DelItem_KnownHash(op, key, hash); } -static int -delitem_knownhash_lock_held(PyObject *op, PyObject *key, Py_hash_t hash) +int +_PyDict_DelItem_KnownHash_LockHeld(PyObject *op, PyObject *key, Py_hash_t hash) { Py_ssize_t ix; PyDictObject *mp; @@ -2806,7 +2806,7 @@ _PyDict_DelItem_KnownHash(PyObject *op, PyObject *key, Py_hash_t hash) { int res; Py_BEGIN_CRITICAL_SECTION(op); - res = delitem_knownhash_lock_held(op, key, hash); + res = _PyDict_DelItem_KnownHash_LockHeld(op, key, hash); Py_END_CRITICAL_SECTION(); return res; } @@ -4647,9 +4647,11 @@ dict_tp_clear(PyObject *op) static PyObject *dictiter_new(PyDictObject *, PyTypeObject *); -static Py_ssize_t -sizeof_lock_held(PyDictObject *mp) +Py_ssize_t +_PyDict_SizeOf_LockHeld(PyDictObject *mp) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(mp); + size_t res = _PyObject_SIZE(Py_TYPE(mp)); if (_PyDict_HasSplitTable(mp)) { res += shared_keys_usable_size(mp->ma_keys) * sizeof(PyObject*); @@ -4668,7 +4670,7 @@ _PyDict_SizeOf(PyDictObject *mp) { Py_ssize_t res; Py_BEGIN_CRITICAL_SECTION(mp); - res = sizeof_lock_held(mp); + res = _PyDict_SizeOf_LockHeld(mp); Py_END_CRITICAL_SECTION(); return res; @@ -6858,7 +6860,7 @@ _PyDict_SetItem_LockHeld(PyDictObject *dict, PyObject *name, PyObject *value) dict_unhashable_type(name); return -1; } - return delitem_knownhash_lock_held((PyObject *)dict, name, hash); + return _PyDict_DelItem_KnownHash_LockHeld((PyObject *)dict, name, hash); } else { return setitem_lock_held(dict, name, value); } diff --git a/Objects/odictobject.c b/Objects/odictobject.c index 891f6197401503..eb3a7ab7ed2f10 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -535,6 +535,7 @@ struct _odictnode { static Py_ssize_t _odict_get_index_raw(PyODictObject *od, PyObject *key, Py_hash_t hash) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); PyObject *value = NULL; PyDictKeysObject *keys = ((PyDictObject *)od)->ma_keys; Py_ssize_t ix; @@ -559,6 +560,7 @@ _odict_get_index_raw(PyODictObject *od, PyObject *key, Py_hash_t hash) static int _odict_resize(PyODictObject *od) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); Py_ssize_t size, i; _ODictNode **fast_nodes, *node; @@ -595,6 +597,7 @@ _odict_resize(PyODictObject *od) static Py_ssize_t _odict_get_index(PyODictObject *od, PyObject *key, Py_hash_t hash) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); PyDictKeysObject *keys; assert(key != NULL); @@ -615,6 +618,7 @@ _odict_get_index(PyODictObject *od, PyObject *key, Py_hash_t hash) static _ODictNode * _odict_find_node_hash(PyODictObject *od, PyObject *key, Py_hash_t hash) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); Py_ssize_t index; if (_odict_EMPTY(od)) @@ -629,6 +633,7 @@ _odict_find_node_hash(PyODictObject *od, PyObject *key, Py_hash_t hash) static _ODictNode * _odict_find_node(PyODictObject *od, PyObject *key) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); Py_ssize_t index; Py_hash_t hash; @@ -647,6 +652,7 @@ _odict_find_node(PyODictObject *od, PyObject *key) static void _odict_add_head(PyODictObject *od, _ODictNode *node) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); _odictnode_PREV(node) = NULL; _odictnode_NEXT(node) = _odict_FIRST(od); if (_odict_FIRST(od) == NULL) @@ -660,6 +666,7 @@ _odict_add_head(PyODictObject *od, _ODictNode *node) static void _odict_add_tail(PyODictObject *od, _ODictNode *node) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); _odictnode_PREV(node) = _odict_LAST(od); _odictnode_NEXT(node) = NULL; if (_odict_LAST(od) == NULL) @@ -674,6 +681,7 @@ _odict_add_tail(PyODictObject *od, _ODictNode *node) static int _odict_add_new_node(PyODictObject *od, PyObject *key, Py_hash_t hash) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); Py_ssize_t i; _ODictNode *node; @@ -718,6 +726,7 @@ _odict_add_new_node(PyODictObject *od, PyObject *key, Py_hash_t hash) static void _odict_remove_node(PyODictObject *od, _ODictNode *node) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); if (_odict_FIRST(od) == node) _odict_FIRST(od) = _odictnode_NEXT(node); else if (_odictnode_PREV(node) != NULL) @@ -753,6 +762,7 @@ static int _odict_clear_node(PyODictObject *od, _ODictNode *node, PyObject *key, Py_hash_t hash) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); Py_ssize_t i; assert(key != NULL); @@ -950,31 +960,34 @@ OrderedDict_fromkeys_impl(PyTypeObject *type, PyObject *seq, PyObject *value) return _PyDict_FromKeys((PyObject *)type, seq, value); } -/* __sizeof__() */ - -/* OrderedDict.__sizeof__() does not have a docstring. */ -PyDoc_STRVAR(odict_sizeof__doc__, ""); +/*[clinic input] +@critical_section +OrderedDict.__sizeof__ -> Py_ssize_t +[clinic start generated code]*/ -static PyObject * -odict_sizeof(PyObject *op, PyObject *Py_UNUSED(ignored)) +static Py_ssize_t +OrderedDict___sizeof___impl(PyODictObject *self) +/*[clinic end generated code: output=1a8560db8cf83ac5 input=655e989ae24daa6a]*/ { - PyODictObject *od = _PyODictObject_CAST(op); - Py_ssize_t res = _PyDict_SizeOf((PyDictObject *)od); - res += sizeof(_ODictNode *) * od->od_fast_nodes_size; /* od_fast_nodes */ - if (!_odict_EMPTY(od)) { - res += sizeof(_ODictNode) * PyODict_SIZE(od); /* linked-list */ + Py_ssize_t res = _PyDict_SizeOf_LockHeld((PyDictObject *)self); + res += sizeof(_ODictNode *) * self->od_fast_nodes_size; /* od_fast_nodes */ + if (!_odict_EMPTY(self)) { + res += sizeof(_ODictNode) * PyODict_SIZE(self); /* linked-list */ } - return PyLong_FromSsize_t(res); + return res; } -/* __reduce__() */ +/*[clinic input] +OrderedDict.__reduce__ + self as od: self(type="PyODictObject *") -PyDoc_STRVAR(odict_reduce__doc__, "Return state information for pickling"); +Return state information for pickling +[clinic start generated code]*/ static PyObject * -odict_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) +OrderedDict___reduce___impl(PyODictObject *od) +/*[clinic end generated code: output=71eeb81f760a6a8e input=b0467c7ec400fe5e]*/ { - register PyODictObject *od = _PyODictObject_CAST(op); PyObject *state, *result = NULL; PyObject *items_iter, *items, *args = NULL; @@ -1007,10 +1020,14 @@ odict_reduce(PyObject *op, PyObject *Py_UNUSED(ignored)) return result; } + /* setdefault(): Skips __missing__() calls. */ +static int PyODict_SetItem_LockHeld(PyObject *self, PyObject *key, + PyObject *value); /*[clinic input] +@critical_section OrderedDict.setdefault key: object @@ -1024,7 +1041,7 @@ Return the value for key if key is in the dictionary, else default. static PyObject * OrderedDict_setdefault_impl(PyODictObject *self, PyObject *key, PyObject *default_value) -/*[clinic end generated code: output=97537cb7c28464b6 input=38e098381c1efbc6]*/ +/*[clinic end generated code: output=97537cb7c28464b6 input=d7b93e92734f99b5]*/ { PyObject *result = NULL; @@ -1034,7 +1051,7 @@ OrderedDict_setdefault_impl(PyODictObject *self, PyObject *key, if (PyErr_Occurred()) return NULL; assert(_odict_find_node(self, key) == NULL); - if (PyODict_SetItem((PyObject *)self, key, default_value) >= 0) { + if (PyODict_SetItem_LockHeld((PyObject *)self, key, default_value) >= 0) { result = Py_NewRef(default_value); } } @@ -1064,10 +1081,9 @@ static PyObject * _odict_popkey_hash(PyObject *od, PyObject *key, PyObject *failobj, Py_hash_t hash) { - PyObject *value = NULL; - - Py_BEGIN_CRITICAL_SECTION(od); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); + PyObject *value = NULL; _ODictNode *node = _odict_find_node_hash(_PyODictObject_CAST(od), key, hash); if (node != NULL) { /* Pop the node first to avoid a possible dict resize (due to @@ -1092,7 +1108,6 @@ _odict_popkey_hash(PyObject *od, PyObject *key, PyObject *failobj, PyErr_SetObject(PyExc_KeyError, key); } } - Py_END_CRITICAL_SECTION(); done: return value; @@ -1100,6 +1115,7 @@ _odict_popkey_hash(PyObject *od, PyObject *key, PyObject *failobj, /* Skips __missing__() calls. */ /*[clinic input] +@critical_section OrderedDict.pop key: object @@ -1114,7 +1130,7 @@ raise a KeyError. static PyObject * OrderedDict_pop_impl(PyODictObject *self, PyObject *key, PyObject *default_value) -/*[clinic end generated code: output=7a6447d104e7494b input=7efe36601007dff7]*/ +/*[clinic end generated code: output=7a6447d104e7494b input=a79988887b4a651f]*/ { Py_hash_t hash = PyObject_Hash(key); if (hash == -1) @@ -1126,6 +1142,7 @@ OrderedDict_pop_impl(PyODictObject *self, PyObject *key, /* popitem() */ /*[clinic input] +@critical_section OrderedDict.popitem last: bool = True @@ -1137,7 +1154,7 @@ Pairs are returned in LIFO order if last is true or FIFO order if false. static PyObject * OrderedDict_popitem_impl(PyODictObject *self, int last) -/*[clinic end generated code: output=98e7d986690d49eb input=d992ac5ee8305e1a]*/ +/*[clinic end generated code: output=98e7d986690d49eb input=8aafc7433e0a40e7]*/ { PyObject *key, *value, *item = NULL; _ODictNode *node; @@ -1191,17 +1208,19 @@ static PyObject * mutablemapping_update(PyObject *, PyObject *, PyObject *); #define odict_update mutablemapping_update -/* clear() */ +/*[clinic input] +@critical_section +OrderedDict.clear -PyDoc_STRVAR(odict_clear__doc__, - "od.clear() -> None. Remove all items from od."); +Remove all items from ordered dict. +[clinic start generated code]*/ static PyObject * -odict_clear(PyObject *op, PyObject *Py_UNUSED(ignored)) +OrderedDict_clear_impl(PyODictObject *self) +/*[clinic end generated code: output=a1a76d1322f556c5 input=08b12322e74c535c]*/ { - register PyODictObject *od = _PyODictObject_CAST(op); - PyDict_Clear(op); - _odict_clear_nodes(od); + PyDict_Clear((PyObject *)self); + _odict_clear_nodes(self); Py_RETURN_NONE; } @@ -1211,12 +1230,18 @@ odict_clear(PyObject *op, PyObject *Py_UNUSED(ignored)) static int _PyODict_SetItem_KnownHash(PyObject *, PyObject *, PyObject *, Py_hash_t); -PyDoc_STRVAR(odict_copy__doc__, "od.copy() -> a shallow copy of od"); +/*[clinic input] +@critical_section +OrderedDict.copy + self as od: self + +A shallow copy of ordered dict. +[clinic start generated code]*/ static PyObject * -odict_copy(PyObject *op, PyObject *Py_UNUSED(ignored)) +OrderedDict_copy_impl(PyObject *od) +/*[clinic end generated code: output=9cdbe7394aecc576 input=e329951ae617ed48]*/ { - register PyODictObject *od = _PyODictObject_CAST(op); _ODictNode *node; PyObject *od_copy; @@ -1285,6 +1310,7 @@ odict_reversed(PyObject *op, PyObject *Py_UNUSED(ignored)) /* move_to_end() */ /*[clinic input] +@critical_section OrderedDict.move_to_end key: object @@ -1297,7 +1323,7 @@ Raise KeyError if the element does not exist. static PyObject * OrderedDict_move_to_end_impl(PyODictObject *self, PyObject *key, int last) -/*[clinic end generated code: output=fafa4c5cc9b92f20 input=d6ceff7132a2fcd7]*/ +/*[clinic end generated code: output=fafa4c5cc9b92f20 input=09f8bc7053c0f6d4]*/ { _ODictNode *node; @@ -1338,10 +1364,8 @@ static PyMethodDef odict_methods[] = { /* overridden dict methods */ ORDEREDDICT_FROMKEYS_METHODDEF - {"__sizeof__", odict_sizeof, METH_NOARGS, - odict_sizeof__doc__}, - {"__reduce__", odict_reduce, METH_NOARGS, - odict_reduce__doc__}, + ORDEREDDICT___SIZEOF___METHODDEF + ORDEREDDICT___REDUCE___METHODDEF ORDEREDDICT_SETDEFAULT_METHODDEF ORDEREDDICT_POP_METHODDEF ORDEREDDICT_POPITEM_METHODDEF @@ -1353,11 +1377,8 @@ static PyMethodDef odict_methods[] = { odict_items__doc__}, {"update", _PyCFunction_CAST(odict_update), METH_VARARGS | METH_KEYWORDS, odict_update__doc__}, - {"clear", odict_clear, METH_NOARGS, - odict_clear__doc__}, - {"copy", odict_copy, METH_NOARGS, - odict_copy__doc__}, - + ORDEREDDICT_CLEAR_METHODDEF + ORDEREDDICT_COPY_METHODDEF /* new methods */ {"__reversed__", odict_reversed, METH_NOARGS, odict_reversed__doc__}, @@ -1465,7 +1486,7 @@ odict_tp_clear(PyObject *op) /* tp_richcompare */ static PyObject * -odict_richcompare(PyObject *v, PyObject *w, int op) +odict_richcompare_lock_held(PyObject *v, PyObject *w, int op) { if (!PyODict_Check(v) || !PyDict_Check(w)) { Py_RETURN_NOTIMPLEMENTED; @@ -1498,6 +1519,16 @@ odict_richcompare(PyObject *v, PyObject *w, int op) } } +static PyObject * +odict_richcompare(PyObject *v, PyObject *w, int op) +{ + PyObject *res; + Py_BEGIN_CRITICAL_SECTION2(v, w); + res = odict_richcompare_lock_held(v, w, op); + Py_END_CRITICAL_SECTION2(); + return res; +} + /* tp_iter */ static PyObject * @@ -1588,10 +1619,11 @@ PyODict_New(void) } static int -_PyODict_SetItem_KnownHash(PyObject *od, PyObject *key, PyObject *value, +_PyODict_SetItem_KnownHash_LockHeld(PyObject *od, PyObject *key, PyObject *value, Py_hash_t hash) { - int res = _PyDict_SetItem_KnownHash(od, key, value, hash); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); + int res = _PyDict_SetItem_KnownHash_LockHeld((PyDictObject *)(od), key, value, hash); if (res == 0) { res = _odict_add_new_node(_PyODictObject_CAST(od), key, hash); if (res < 0) { @@ -1604,18 +1636,43 @@ _PyODict_SetItem_KnownHash(PyObject *od, PyObject *key, PyObject *value, return res; } -int -PyODict_SetItem(PyObject *od, PyObject *key, PyObject *value) +static int +_PyODict_SetItem_KnownHash(PyObject *od, PyObject *key, PyObject *value, + Py_hash_t hash) { + int res; + Py_BEGIN_CRITICAL_SECTION(od); + res = _PyODict_SetItem_KnownHash_LockHeld(od, key, value, hash); + Py_END_CRITICAL_SECTION(); + return res; +} + +static int +PyODict_SetItem_LockHeld(PyObject *od, PyObject *key, PyObject *value) +{ + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); + int res; Py_hash_t hash = PyObject_Hash(key); if (hash == -1) return -1; - return _PyODict_SetItem_KnownHash(od, key, value, hash); + res = _PyODict_SetItem_KnownHash_LockHeld(od, key, value, hash); + return res; } int -PyODict_DelItem(PyObject *od, PyObject *key) +PyODict_SetItem(PyObject *od, PyObject *key, PyObject *value) { + int res; + Py_BEGIN_CRITICAL_SECTION(od); + res = PyODict_SetItem_LockHeld(od, key, value); + Py_END_CRITICAL_SECTION(); + return res; +} + +int +PyODict_DelItem_LockHeld(PyObject *od, PyObject *key) +{ + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); int res; Py_hash_t hash = PyObject_Hash(key); if (hash == -1) @@ -1623,9 +1680,18 @@ PyODict_DelItem(PyObject *od, PyObject *key) res = _odict_clear_node(_PyODictObject_CAST(od), NULL, key, hash); if (res < 0) return -1; - return _PyDict_DelItem_KnownHash(od, key, hash); + return _PyDict_DelItem_KnownHash_LockHeld(od, key, hash); } +int +PyODict_DelItem(PyObject *od, PyObject *key) +{ + int res; + Py_BEGIN_CRITICAL_SECTION(od); + res = PyODict_DelItem_LockHeld(od, key); + Py_END_CRITICAL_SECTION(); + return res; +} /* ------------------------------------------- * The OrderedDict views (keys/values/items) @@ -1667,8 +1733,9 @@ odictiter_traverse(PyObject *op, visitproc visit, void *arg) /* In order to protect against modifications during iteration, we track * the current key instead of the current node. */ static PyObject * -odictiter_nextkey(odictiterobject *di) +odictiter_nextkey_lock_held(odictiterobject *di) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(di->di_odict); PyObject *key = NULL; _ODictNode *node; int reversed = di->kind & _odict_ITER_REVERSED; @@ -1715,16 +1782,18 @@ odictiter_nextkey(odictiterobject *di) return key; done: +#ifndef Py_GIL_DISABLED Py_CLEAR(di->di_odict); +#endif return key; } static PyObject * -odictiter_iternext(PyObject *op) +odictiter_iternext_lock_held(PyObject *op) { odictiterobject *di = (odictiterobject*)op; PyObject *result, *value; - PyObject *key = odictiter_nextkey(di); /* new reference */ + PyObject *key = odictiter_nextkey_lock_held(di); /* new reference */ if (key == NULL) return NULL; @@ -1752,7 +1821,7 @@ odictiter_iternext(PyObject *op) /* Handle the items case. */ result = di->di_result; - if (Py_REFCNT(result) == 1) { + if (_PyObject_IsUniquelyReferenced(result) == 1) { /* not in use so we can reuse it * (the common case during iteration) */ Py_INCREF(result); @@ -1777,10 +1846,23 @@ odictiter_iternext(PyObject *op) done: Py_CLEAR(di->di_current); +#ifndef Py_GIL_DISABLED Py_CLEAR(di->di_odict); +#endif return NULL; } +static PyObject * +odictiter_iternext(PyObject *op) +{ + PyObject *res; + Py_BEGIN_CRITICAL_SECTION(((odictiterobject*)op)->di_odict); + res = odictiter_iternext_lock_held(op); + Py_END_CRITICAL_SECTION(); + return res; +} + + /* No need for tp_clear because odictiterobject is not mutable. */ PyDoc_STRVAR(reduce_doc, "Return state information for pickling"); From 5cbaeeb2f1aaa0c07f765651cc4b51d19c543566 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 10 May 2025 17:42:07 +0000 Subject: [PATCH 2/5] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2025-05-10-17-42-03.gh-issue-125996.vaQp0-.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2025-05-10-17-42-03.gh-issue-125996.vaQp0-.rst diff --git a/Misc/NEWS.d/next/Library/2025-05-10-17-42-03.gh-issue-125996.vaQp0-.rst b/Misc/NEWS.d/next/Library/2025-05-10-17-42-03.gh-issue-125996.vaQp0-.rst new file mode 100644 index 00000000000000..4bbfaa3a5e2d85 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-05-10-17-42-03.gh-issue-125996.vaQp0-.rst @@ -0,0 +1 @@ +Fix thread safety of :class:`collections.OrderedDict`. Patch by Kumar Aditya. From 5c8c90b42e3bda0d5639b2183c4e57054e795715 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Mon, 19 May 2025 18:18:31 +0530 Subject: [PATCH 3/5] fmt --- Include/internal/pycore_dict.h | 2 +- Objects/odictobject.c | 15 ++++++--------- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index c409468968528c..afd468735b4893 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -32,7 +32,7 @@ PyAPI_FUNC(int) _PyDict_DelItem_KnownHash(PyObject *mp, PyObject *key, Py_hash_t hash); extern int _PyDict_DelItem_KnownHash_LockHeld(PyObject *mp, PyObject *key, - Py_hash_t hash); + Py_hash_t hash); extern int _PyDict_Contains_KnownHash(PyObject *, PyObject *, Py_hash_t); diff --git a/Objects/odictobject.c b/Objects/odictobject.c index eb3a7ab7ed2f10..fb4f3c1cec0a4c 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1020,11 +1020,9 @@ OrderedDict___reduce___impl(PyODictObject *od) return result; } - /* setdefault(): Skips __missing__() calls. */ -static int PyODict_SetItem_LockHeld(PyObject *self, PyObject *key, - PyObject *value); +static int PyODict_SetItem_LockHeld(PyObject *self, PyObject *key, PyObject *value); /*[clinic input] @critical_section @@ -1620,10 +1618,10 @@ PyODict_New(void) static int _PyODict_SetItem_KnownHash_LockHeld(PyObject *od, PyObject *key, PyObject *value, - Py_hash_t hash) + Py_hash_t hash) { _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); - int res = _PyDict_SetItem_KnownHash_LockHeld((PyDictObject *)(od), key, value, hash); + int res = _PyDict_SetItem_KnownHash_LockHeld((PyDictObject *)od, key, value, hash); if (res == 0) { res = _odict_add_new_node(_PyODictObject_CAST(od), key, hash); if (res < 0) { @@ -1651,12 +1649,11 @@ static int PyODict_SetItem_LockHeld(PyObject *od, PyObject *key, PyObject *value) { _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(od); - int res; Py_hash_t hash = PyObject_Hash(key); - if (hash == -1) + if (hash == -1) { return -1; - res = _PyODict_SetItem_KnownHash_LockHeld(od, key, value, hash); - return res; + } + return _PyODict_SetItem_KnownHash_LockHeld(od, key, value, hash); } int From 0feaa1a2c34de5508e5e47a587159c8ae3ba9332 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Tue, 27 May 2025 17:44:43 +0530 Subject: [PATCH 4/5] use lock held clear of dict --- Objects/odictobject.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/Objects/odictobject.c b/Objects/odictobject.c index fb4f3c1cec0a4c..ae19241521f731 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1181,6 +1181,9 @@ OrderedDict_popitem_impl(PyODictObject *self, int last) PyDoc_STRVAR(odict_keys__doc__, ""); static PyObject * odictkeys_new(PyObject *od, PyObject *Py_UNUSED(ignored)); /* forward */ +static int +_PyODict_SetItem_KnownHash_LockHeld(PyObject *od, PyObject *key, PyObject *value, + Py_hash_t hash); /* forward */ /* values() */ @@ -1217,7 +1220,7 @@ static PyObject * OrderedDict_clear_impl(PyODictObject *self) /*[clinic end generated code: output=a1a76d1322f556c5 input=08b12322e74c535c]*/ { - PyDict_Clear((PyObject *)self); + _PyDict_Clear_LockHeld((PyDictObject *)self); _odict_clear_nodes(self); Py_RETURN_NONE; } @@ -1259,8 +1262,8 @@ OrderedDict_copy_impl(PyObject *od) PyErr_SetObject(PyExc_KeyError, key); goto fail; } - if (_PyODict_SetItem_KnownHash((PyObject *)od_copy, key, value, - _odictnode_HASH(node)) != 0) + if (_PyODict_SetItem_KnownHash_LockHeld((PyObject *)od_copy, key, value, + _odictnode_HASH(node)) != 0) goto fail; } } From 146a307eac446aec6c5f0a9488a70bce05ce5470 Mon Sep 17 00:00:00 2001 From: Kumar Aditya Date: Tue, 27 May 2025 18:00:06 +0530 Subject: [PATCH 5/5] remove unused function --- Objects/odictobject.c | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/Objects/odictobject.c b/Objects/odictobject.c index ae19241521f731..80fdb23524c734 100644 --- a/Objects/odictobject.c +++ b/Objects/odictobject.c @@ -1220,17 +1220,13 @@ static PyObject * OrderedDict_clear_impl(PyODictObject *self) /*[clinic end generated code: output=a1a76d1322f556c5 input=08b12322e74c535c]*/ { - _PyDict_Clear_LockHeld((PyDictObject *)self); + _PyDict_Clear_LockHeld((PyObject *)self); _odict_clear_nodes(self); Py_RETURN_NONE; } /* copy() */ -/* forward */ -static int _PyODict_SetItem_KnownHash(PyObject *, PyObject *, PyObject *, - Py_hash_t); - /*[clinic input] @critical_section OrderedDict.copy @@ -1637,16 +1633,6 @@ _PyODict_SetItem_KnownHash_LockHeld(PyObject *od, PyObject *key, PyObject *value return res; } -static int -_PyODict_SetItem_KnownHash(PyObject *od, PyObject *key, PyObject *value, - Py_hash_t hash) -{ - int res; - Py_BEGIN_CRITICAL_SECTION(od); - res = _PyODict_SetItem_KnownHash_LockHeld(od, key, value, hash); - Py_END_CRITICAL_SECTION(); - return res; -} static int PyODict_SetItem_LockHeld(PyObject *od, PyObject *key, PyObject *value)