From f1776aa7f33ed6383f07a7fccfdc680bd697a0d1 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 17 Apr 2024 22:58:04 +0900 Subject: [PATCH 01/11] gh-112069: Make _PySet_NextEntry to be thread-safe. --- Include/internal/pycore_setobject.h | 7 ++++++ Modules/_abc.c | 4 ++-- Objects/codeobject.c | 2 +- Objects/dictobject.c | 10 +++++++- Objects/listobject.c | 9 +++++++- Objects/setobject.c | 36 ++++++++++++++++++++++------- Python/compile.c | 2 +- Python/pylifecycle.c | 3 ++- 8 files changed, 58 insertions(+), 15 deletions(-) diff --git a/Include/internal/pycore_setobject.h b/Include/internal/pycore_setobject.h index c4ec3ceb17eba6..5ab1f7bdf990a3 100644 --- a/Include/internal/pycore_setobject.h +++ b/Include/internal/pycore_setobject.h @@ -15,6 +15,13 @@ PyAPI_FUNC(int) _PySet_NextEntry( PyObject **key, Py_hash_t *hash); +// Export for 'Python/compile.c' +PyAPI_FUNC(int) _PyFrozenSet_NextEntry( + PyObject *set, + Py_ssize_t *pos, + PyObject **key, + Py_hash_t *hash); + // Export for '_pickle' shared extension PyAPI_FUNC(int) _PySet_Update(PyObject *set, PyObject *iterable); diff --git a/Modules/_abc.c b/Modules/_abc.c index ad28035843fd32..d68022cb873114 100644 --- a/Modules/_abc.c +++ b/Modules/_abc.c @@ -862,7 +862,7 @@ subclasscheck_check_registry(_abc_data *impl, PyObject *subclass, // Make a local copy of the registry to protect against concurrent // modifications of _abc_registry. - PyObject *registry = PySet_New(registry_shared); + PyObject *registry = PyFrozenSet_New(registry_shared); if (registry == NULL) { return -1; } @@ -870,7 +870,7 @@ subclasscheck_check_registry(_abc_data *impl, PyObject *subclass, Py_ssize_t pos = 0; Py_hash_t hash; - while (_PySet_NextEntry(registry, &pos, &key, &hash)) { + while (_PyFrozenSet_NextEntry(registry, &pos, &key, &hash)) { PyObject *rkey; if (PyWeakref_GetRef(key, &rkey) < 0) { // Someone inject non-weakref type in the registry. diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 014632962bfcf3..735b1dd7a50021 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2347,7 +2347,7 @@ _PyCode_ConstantKey(PyObject *op) return NULL; i = 0; - while (_PySet_NextEntry(op, &pos, &item, &hash)) { + while (_PyFrozenSet_NextEntry(op, &pos, &item, &hash)) { PyObject *item_key; item_key = _PyCode_ConstantKey(item); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 003a03fd741702..2151c7ea509dda 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2979,7 +2979,15 @@ dict_set_fromkeys(PyInterpreterState *interp, PyDictObject *mp, return NULL; } - while (_PySet_NextEntry(iterable, &pos, &key, &hash)) { + int (*next_entry_ptr)(PyObject *, Py_ssize_t *, PyObject **, Py_hash_t *); + if (PyFrozenSet_CheckExact(iterable)) { + next_entry_ptr = &_PyFrozenSet_NextEntry; + } + else { + next_entry_ptr = &_PySet_NextEntry; + } + + while ((*next_entry_ptr)(iterable, &pos, &key, &hash)) { if (insertdict(interp, mp, Py_NewRef(key), hash, Py_NewRef(value))) { Py_DECREF(mp); return NULL; diff --git a/Objects/listobject.c b/Objects/listobject.c index 472c471d9968a4..43705658c7d230 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -1282,12 +1282,19 @@ list_extend_set(PyListObject *self, PySetObject *other) if (list_resize(self, m + n) < 0) { return -1; } + int (*next_entry_ptr)(PyObject *, Py_ssize_t *, PyObject **, Py_hash_t *); + if (PyFrozenSet_CheckExact(other)) { + next_entry_ptr = &_PyFrozenSet_NextEntry; + } + else { + next_entry_ptr = &_PySet_NextEntry; + } /* populate the end of self with iterable's items */ Py_ssize_t setpos = 0; Py_hash_t hash; PyObject *key; PyObject **dest = self->ob_item + m; - while (_PySet_NextEntry((PyObject *)other, &setpos, &key, &hash)) { + while ((*next_entry_ptr)((PyObject *)other, &setpos, &key, &hash)) { Py_INCREF(key); FT_ATOMIC_STORE_PTR_RELEASE(*dest, key); dest++; diff --git a/Objects/setobject.c b/Objects/setobject.c index 66ca80e8fc25f9..8f9e17d57f9e57 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -473,7 +473,7 @@ set_clear_internal(PySetObject *so) * while (set_next(yourset, &pos, &entry)) { * Refer to borrowed reference in entry->key. * } - * + *f * CAUTION: In general, it isn't safe to use set_next in a loop that * mutates the table. */ @@ -2661,21 +2661,41 @@ PySet_Add(PyObject *anyset, PyObject *key) return rv; } -// TODO: Make thread-safe in free-threaded builds int -_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) +_PySet_NextEntry_lock_held(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) { setentry *entry; + int ret = set_next((PySetObject *)set, pos, &entry); + if (ret == 0) { + return 0; + } + *key = entry->key; + *hash = entry->hash; + return 1; +} +int +_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) +{ if (!PyAnySet_Check(set)) { PyErr_BadInternalCall(); return -1; } - if (set_next((PySetObject *)set, pos, &entry) == 0) - return 0; - *key = entry->key; - *hash = entry->hash; - return 1; + int ret; + Py_BEGIN_CRITICAL_SECTION(set); + ret = _PySet_NextEntry_lock_held(set, pos, key, hash); + Py_END_CRITICAL_SECTION(); + return ret; +} + +int +_PyFrozenSet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) +{ + if (!PyFrozenSet_CheckExact(set)) { + PyErr_BadInternalCall(); + return -1; + } + return _PySet_NextEntry_lock_held(set, pos, key, hash); } PyObject * diff --git a/Python/compile.c b/Python/compile.c index 1e8f97e72cdff6..00dc7d7dfa23a2 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -920,7 +920,7 @@ merge_consts_recursive(PyObject *const_cache, PyObject *o) Py_ssize_t i = 0, pos = 0; PyObject *item; Py_hash_t hash; - while (_PySet_NextEntry(o, &pos, &item, &hash)) { + while (_PyFrozenSet_NextEntry(o, &pos, &item, &hash)) { PyObject *k = merge_consts_recursive(const_cache, item); if (k == NULL) { Py_DECREF(tuple); diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index efb25878312d85..f148bdca9c135d 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2910,7 +2910,8 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) Py_ssize_t i = 0; PyObject *item; Py_hash_t hash; - while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) { + // if stdlib_module_names is not NULL, it is always a frozenset. + while (_PyFrozenSet_NextEntry(stdlib_module_names, &i, &item, &hash)) { if (PyUnicode_Check(item) && PyUnicode_Compare(key, item) == 0) { From 4170a351ae6a1eca1c9a2de48bf6c8580f939891 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Wed, 17 Apr 2024 23:31:26 +0900 Subject: [PATCH 02/11] nit --- Objects/setobject.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Objects/setobject.c b/Objects/setobject.c index 8f9e17d57f9e57..d59ca4ce6fcd42 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -473,7 +473,7 @@ set_clear_internal(PySetObject *so) * while (set_next(yourset, &pos, &entry)) { * Refer to borrowed reference in entry->key. * } - *f + * * CAUTION: In general, it isn't safe to use set_next in a loop that * mutates the table. */ From 60e8d539ccb1be383737635a64e3acfef7dccae4 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 01:36:49 +0900 Subject: [PATCH 03/11] WIP --- Modules/_pickle.c | 28 ++++++++++++++++++---------- Modules/_testinternalcapi/set.c | 4 +++- Objects/dictobject.c | 14 ++++---------- Objects/listobject.c | 11 +++-------- Objects/setobject.c | 7 ++----- Python/marshal.c | 25 +++++++++++++++---------- 6 files changed, 45 insertions(+), 44 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 0d83261168185d..3e3794891520d6 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -9,15 +9,16 @@ #endif #include "Python.h" -#include "pycore_bytesobject.h" // _PyBytesWriter -#include "pycore_ceval.h" // _Py_EnterRecursiveCall() -#include "pycore_long.h" // _PyLong_AsByteArray() -#include "pycore_moduleobject.h" // _PyModule_GetState() -#include "pycore_object.h" // _PyNone_Type -#include "pycore_pystate.h" // _PyThreadState_GET() -#include "pycore_runtime.h" // _Py_ID() -#include "pycore_setobject.h" // _PySet_NextEntry() -#include "pycore_sysmodule.h" // _PySys_GetAttr() +#include "pycore_bytesobject.h" // _PyBytesWriter +#include "pycore_ceval.h" // _Py_EnterRecursiveCall() +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() +#include "pycore_long.h" // _PyLong_AsByteArray() +#include "pycore_moduleobject.h" // _PyModule_GetState() +#include "pycore_object.h" // _PyNone_Type +#include "pycore_pystate.h" // _PyThreadState_GET() +#include "pycore_runtime.h" // _Py_ID() +#include "pycore_setobject.h" // _PySet_NextEntry() +#include "pycore_sysmodule.h" // _PySys_GetAttr() #include // strtol() @@ -3413,15 +3414,22 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj) i = 0; if (_Pickler_Write(self, &mark_op, 1) < 0) return -1; + + int err; + Py_BEGIN_CRITICAL_SECTION(obj); while (_PySet_NextEntry(obj, &ppos, &item, &hash)) { Py_INCREF(item); int err = save(state, self, item, 0); Py_CLEAR(item); if (err < 0) - return -1; + break; if (++i == BATCHSIZE) break; } + Py_END_CRITICAL_SECTION(); + if (err < 0) { + return -1; + } if (_Pickler_Write(self, &additems_op, 1) < 0) return -1; if (PySet_GET_SIZE(obj) != set_size) { diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c index 0305a7885d217c..ca61528dbd226d 100644 --- a/Modules/_testinternalcapi/set.c +++ b/Modules/_testinternalcapi/set.c @@ -1,6 +1,7 @@ #include "parts.h" #include "../_testcapi/util.h" // NULLABLE, RETURN_INT +#include "pycore_critical_section.h" #include "pycore_setobject.h" @@ -27,8 +28,9 @@ set_next_entry(PyObject *self, PyObject *args) return NULL; } NULLABLE(set); - + Py_BEGIN_CRITICAL_SECTION(set); rc = _PySet_NextEntry(set, &pos, &item, &hash); + Py_END_CRITICAL_SECTION(); if (rc == 1) { return Py_BuildValue("innO", rc, pos, hash, item); } diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 2151c7ea509dda..a91c5abd31b262 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2979,18 +2979,12 @@ dict_set_fromkeys(PyInterpreterState *interp, PyDictObject *mp, return NULL; } - int (*next_entry_ptr)(PyObject *, Py_ssize_t *, PyObject **, Py_hash_t *); - if (PyFrozenSet_CheckExact(iterable)) { - next_entry_ptr = &_PyFrozenSet_NextEntry; - } - else { - next_entry_ptr = &_PySet_NextEntry; - } - - while ((*next_entry_ptr)(iterable, &pos, &key, &hash)) { + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(iterable); + while (_PySet_NextEntry(iterable, &pos, &key, &hash)) { if (insertdict(interp, mp, Py_NewRef(key), hash, Py_NewRef(value))) { Py_DECREF(mp); - return NULL; + mp = NULL; + break; } } return mp; diff --git a/Objects/listobject.c b/Objects/listobject.c index 43705658c7d230..8deecd5658e103 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -1282,23 +1282,18 @@ list_extend_set(PyListObject *self, PySetObject *other) if (list_resize(self, m + n) < 0) { return -1; } - int (*next_entry_ptr)(PyObject *, Py_ssize_t *, PyObject **, Py_hash_t *); - if (PyFrozenSet_CheckExact(other)) { - next_entry_ptr = &_PyFrozenSet_NextEntry; - } - else { - next_entry_ptr = &_PySet_NextEntry; - } /* populate the end of self with iterable's items */ Py_ssize_t setpos = 0; Py_hash_t hash; PyObject *key; PyObject **dest = self->ob_item + m; - while ((*next_entry_ptr)((PyObject *)other, &setpos, &key, &hash)) { + Py_BEGIN_CRITICAL_SECTION(other); + while (_PySet_NextEntry((PyObject *)other, &setpos, &key, &hash)) { Py_INCREF(key); FT_ATOMIC_STORE_PTR_RELEASE(*dest, key); dest++; } + Py_END_CRITICAL_SECTION(); Py_SET_SIZE(self, m + n); return 0; } diff --git a/Objects/setobject.c b/Objects/setobject.c index d59ca4ce6fcd42..dafd856649760b 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -2681,11 +2681,8 @@ _PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash PyErr_BadInternalCall(); return -1; } - int ret; - Py_BEGIN_CRITICAL_SECTION(set); - ret = _PySet_NextEntry_lock_held(set, pos, key, hash); - Py_END_CRITICAL_SECTION(); - return ret; + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(set); + return _PySet_NextEntry_lock_held(set, pos, key, hash); } int diff --git a/Python/marshal.c b/Python/marshal.c index 21d242bbb9757e..1979863bdca493 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -7,12 +7,13 @@ and sharing. */ #include "Python.h" -#include "pycore_call.h" // _PyObject_CallNoArgs() -#include "pycore_code.h" // _PyCode_New() -#include "pycore_hashtable.h" // _Py_hashtable_t -#include "pycore_long.h" // _PyLong_DigitCount -#include "pycore_setobject.h" // _PySet_NextEntry() -#include "marshal.h" // Py_MARSHAL_VERSION +#include "pycore_call.h" // _PyObject_CallNoArgs() +#include "pycore_code.h" // _PyCode_New() +#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION() +#include "pycore_hashtable.h" // _Py_hashtable_t +#include "pycore_long.h" // _PyLong_DigitCount +#include "pycore_setobject.h" // _PySet_NextEntry() +#include "marshal.h" // Py_MARSHAL_VERSION #ifdef __APPLE__ # include "TargetConditionals.h" @@ -531,23 +532,27 @@ w_complex_object(PyObject *v, char flag, WFILE *p) return; } Py_ssize_t i = 0; + Py_BEGIN_CRITICAL_SECTION(v); while (_PySet_NextEntry(v, &pos, &value, &hash)) { PyObject *dump = _PyMarshal_WriteObjectToString(value, p->version, p->allow_code); if (dump == NULL) { p->error = WFERR_UNMARSHALLABLE; - Py_DECREF(pairs); - return; + break; } PyObject *pair = PyTuple_Pack(2, dump, value); Py_DECREF(dump); if (pair == NULL) { p->error = WFERR_NOMEMORY; - Py_DECREF(pairs); - return; + break; } PyList_SET_ITEM(pairs, i++, pair); } + Py_END_CRITICAL_SECTION(); + if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY) { + Py_DECREF(pairs); + return; + } assert(i == n); if (PyList_Sort(pairs)) { p->error = WFERR_NOMEMORY; From c8b531dc07184f80c12fc26e8c937428dba18d15 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 01:46:25 +0900 Subject: [PATCH 04/11] nit --- Modules/_pickle.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 3e3794891520d6..ad919bd6337226 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -3415,11 +3415,11 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj) if (_Pickler_Write(self, &mark_op, 1) < 0) return -1; - int err; + int err = 0; Py_BEGIN_CRITICAL_SECTION(obj); while (_PySet_NextEntry(obj, &ppos, &item, &hash)) { Py_INCREF(item); - int err = save(state, self, item, 0); + err = save(state, self, item, 0); Py_CLEAR(item); if (err < 0) break; From c81dd70f14b2619c1359e63eacf58d8dd1d71157 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 02:09:08 +0900 Subject: [PATCH 05/11] Update --- Modules/_abc.c | 3 +++ Modules/_pickle.c | 1 - Objects/codeobject.c | 2 ++ Objects/dictobject.c | 2 +- Objects/listobject.c | 1 - Objects/setobject.c | 2 +- Python/compile.c | 1 + Python/marshal.c | 1 + Python/pylifecycle.c | 1 + 9 files changed, 10 insertions(+), 4 deletions(-) diff --git a/Modules/_abc.c b/Modules/_abc.c index d68022cb873114..3cfb6c7835a89e 100644 --- a/Modules/_abc.c +++ b/Modules/_abc.c @@ -875,14 +875,17 @@ subclasscheck_check_registry(_abc_data *impl, PyObject *subclass, if (PyWeakref_GetRef(key, &rkey) < 0) { // Someone inject non-weakref type in the registry. ret = -1; + Py_DECREF(key); break; } if (rkey == NULL) { + Py_DECREF(key); continue; } int r = PyObject_IsSubclass(subclass, rkey); Py_DECREF(rkey); + Py_DECREF(key); if (r < 0) { ret = -1; break; diff --git a/Modules/_pickle.c b/Modules/_pickle.c index ad919bd6337226..845cf6b960084b 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -3418,7 +3418,6 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj) int err = 0; Py_BEGIN_CRITICAL_SECTION(obj); while (_PySet_NextEntry(obj, &ppos, &item, &hash)) { - Py_INCREF(item); err = save(state, self, item, 0); Py_CLEAR(item); if (err < 0) diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 735b1dd7a50021..5f89c09205f9b6 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2352,12 +2352,14 @@ _PyCode_ConstantKey(PyObject *op) item_key = _PyCode_ConstantKey(item); if (item_key == NULL) { + Py_DECREF(item); Py_DECREF(tuple); return NULL; } assert(i < len); PyTuple_SET_ITEM(tuple, i, item_key); + Py_DECREF(item); i++; } set = PyFrozenSet_New(tuple); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index a91c5abd31b262..4d2fbac67cbaa1 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2981,7 +2981,7 @@ dict_set_fromkeys(PyInterpreterState *interp, PyDictObject *mp, _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(iterable); while (_PySet_NextEntry(iterable, &pos, &key, &hash)) { - if (insertdict(interp, mp, Py_NewRef(key), hash, Py_NewRef(value))) { + if (insertdict(interp, mp, key, hash, Py_NewRef(value))) { Py_DECREF(mp); mp = NULL; break; diff --git a/Objects/listobject.c b/Objects/listobject.c index 8deecd5658e103..bf0ba0e89cde15 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -1289,7 +1289,6 @@ list_extend_set(PyListObject *self, PySetObject *other) PyObject **dest = self->ob_item + m; Py_BEGIN_CRITICAL_SECTION(other); while (_PySet_NextEntry((PyObject *)other, &setpos, &key, &hash)) { - Py_INCREF(key); FT_ATOMIC_STORE_PTR_RELEASE(*dest, key); dest++; } diff --git a/Objects/setobject.c b/Objects/setobject.c index dafd856649760b..53f870286f3836 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -2669,7 +2669,7 @@ _PySet_NextEntry_lock_held(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_ha if (ret == 0) { return 0; } - *key = entry->key; + *key = Py_NewRef(entry->key); *hash = entry->hash; return 1; } diff --git a/Python/compile.c b/Python/compile.c index 00dc7d7dfa23a2..7ceb3456489ee7 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -922,6 +922,7 @@ merge_consts_recursive(PyObject *const_cache, PyObject *o) Py_hash_t hash; while (_PyFrozenSet_NextEntry(o, &pos, &item, &hash)) { PyObject *k = merge_consts_recursive(const_cache, item); + Py_DECREF(item); if (k == NULL) { Py_DECREF(tuple); Py_DECREF(key); diff --git a/Python/marshal.c b/Python/marshal.c index 1979863bdca493..2cd2e17e5a6d76 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -542,6 +542,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p) } PyObject *pair = PyTuple_Pack(2, dump, value); Py_DECREF(dump); + Py_DECREF(value); if (pair == NULL) { p->error = WFERR_NOMEMORY; break; diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index f148bdca9c135d..023a3d47774a26 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2916,6 +2916,7 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) && PyUnicode_Compare(key, item) == 0) { is_stdlib_ext = 1; + Py_DECREF(item); break; } } From 0e32d1aa536619fe9031422573dbc385d3f91829 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 02:24:30 +0900 Subject: [PATCH 06/11] nit --- Modules/_pickle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 845cf6b960084b..dbee4cc508d962 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -3415,7 +3415,7 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj) if (_Pickler_Write(self, &mark_op, 1) < 0) return -1; - int err = 0; + int err = -1; Py_BEGIN_CRITICAL_SECTION(obj); while (_PySet_NextEntry(obj, &ppos, &item, &hash)) { err = save(state, self, item, 0); From fbb996478e3d6d2febc4e9ff16bdf4cbe4bbd816 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 02:38:56 +0900 Subject: [PATCH 07/11] Remove uncessary locking --- Objects/listobject.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index bf0ba0e89cde15..0510d4204d2931 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -1287,12 +1287,10 @@ list_extend_set(PyListObject *self, PySetObject *other) Py_hash_t hash; PyObject *key; PyObject **dest = self->ob_item + m; - Py_BEGIN_CRITICAL_SECTION(other); while (_PySet_NextEntry((PyObject *)other, &setpos, &key, &hash)) { FT_ATOMIC_STORE_PTR_RELEASE(*dest, key); dest++; } - Py_END_CRITICAL_SECTION(); Py_SET_SIZE(self, m + n); return 0; } From 5d57a56a313aeed01b771bae44ccd8f7e8e4ec04 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 02:54:00 +0900 Subject: [PATCH 08/11] fix --- Modules/_pickle.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_pickle.c b/Modules/_pickle.c index dbee4cc508d962..845cf6b960084b 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -3415,7 +3415,7 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj) if (_Pickler_Write(self, &mark_op, 1) < 0) return -1; - int err = -1; + int err = 0; Py_BEGIN_CRITICAL_SECTION(obj); while (_PySet_NextEntry(obj, &ppos, &item, &hash)) { err = save(state, self, item, 0); From ae8f217fb1095e03c0b2b71966fbc7e1ee1e2753 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 09:52:16 +0900 Subject: [PATCH 09/11] Address code review --- Include/internal/pycore_setobject.h | 6 +++--- Modules/_abc.c | 5 +---- Modules/_pickle.c | 2 +- Modules/_testinternalcapi/set.c | 2 +- Objects/codeobject.c | 4 +--- Objects/dictobject.c | 2 +- Objects/listobject.c | 2 +- Objects/setobject.c | 33 ++++++++++++++--------------- Python/compile.c | 3 +-- Python/marshal.c | 2 +- Python/pylifecycle.c | 3 +-- 11 files changed, 28 insertions(+), 36 deletions(-) diff --git a/Include/internal/pycore_setobject.h b/Include/internal/pycore_setobject.h index 5ab1f7bdf990a3..3a8461d573a73c 100644 --- a/Include/internal/pycore_setobject.h +++ b/Include/internal/pycore_setobject.h @@ -8,15 +8,15 @@ extern "C" { # error "this header requires Py_BUILD_CORE define" #endif -// Export for '_pickle' shared extension +// Export for '_abc' shared extension PyAPI_FUNC(int) _PySet_NextEntry( PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash); -// Export for 'Python/compile.c' -PyAPI_FUNC(int) _PyFrozenSet_NextEntry( +// Export for '_pickle' +PyAPI_FUNC(int) _PySet_NextEntryRef( PyObject *set, Py_ssize_t *pos, PyObject **key, diff --git a/Modules/_abc.c b/Modules/_abc.c index 3cfb6c7835a89e..f2a523e6f2fc27 100644 --- a/Modules/_abc.c +++ b/Modules/_abc.c @@ -870,22 +870,19 @@ subclasscheck_check_registry(_abc_data *impl, PyObject *subclass, Py_ssize_t pos = 0; Py_hash_t hash; - while (_PyFrozenSet_NextEntry(registry, &pos, &key, &hash)) { + while (_PySet_NextEntry(registry, &pos, &key, &hash)) { PyObject *rkey; if (PyWeakref_GetRef(key, &rkey) < 0) { // Someone inject non-weakref type in the registry. ret = -1; - Py_DECREF(key); break; } if (rkey == NULL) { - Py_DECREF(key); continue; } int r = PyObject_IsSubclass(subclass, rkey); Py_DECREF(rkey); - Py_DECREF(key); if (r < 0) { ret = -1; break; diff --git a/Modules/_pickle.c b/Modules/_pickle.c index 845cf6b960084b..d7ffb04c28c2ac 100644 --- a/Modules/_pickle.c +++ b/Modules/_pickle.c @@ -3417,7 +3417,7 @@ save_set(PickleState *state, PicklerObject *self, PyObject *obj) int err = 0; Py_BEGIN_CRITICAL_SECTION(obj); - while (_PySet_NextEntry(obj, &ppos, &item, &hash)) { + while (_PySet_NextEntryRef(obj, &ppos, &item, &hash)) { err = save(state, self, item, 0); Py_CLEAR(item); if (err < 0) diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c index ca61528dbd226d..6cd69a739925b2 100644 --- a/Modules/_testinternalcapi/set.c +++ b/Modules/_testinternalcapi/set.c @@ -29,7 +29,7 @@ set_next_entry(PyObject *self, PyObject *args) } NULLABLE(set); Py_BEGIN_CRITICAL_SECTION(set); - rc = _PySet_NextEntry(set, &pos, &item, &hash); + rc = _PySet_NextEntryRef(set, &pos, &item, &hash); Py_END_CRITICAL_SECTION(); if (rc == 1) { return Py_BuildValue("innO", rc, pos, hash, item); diff --git a/Objects/codeobject.c b/Objects/codeobject.c index 5f89c09205f9b6..014632962bfcf3 100644 --- a/Objects/codeobject.c +++ b/Objects/codeobject.c @@ -2347,19 +2347,17 @@ _PyCode_ConstantKey(PyObject *op) return NULL; i = 0; - while (_PyFrozenSet_NextEntry(op, &pos, &item, &hash)) { + while (_PySet_NextEntry(op, &pos, &item, &hash)) { PyObject *item_key; item_key = _PyCode_ConstantKey(item); if (item_key == NULL) { - Py_DECREF(item); Py_DECREF(tuple); return NULL; } assert(i < len); PyTuple_SET_ITEM(tuple, i, item_key); - Py_DECREF(item); i++; } set = PyFrozenSet_New(tuple); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 4d2fbac67cbaa1..f005029d7e53eb 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2980,7 +2980,7 @@ dict_set_fromkeys(PyInterpreterState *interp, PyDictObject *mp, } _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(iterable); - while (_PySet_NextEntry(iterable, &pos, &key, &hash)) { + while (_PySet_NextEntryRef(iterable, &pos, &key, &hash)) { if (insertdict(interp, mp, key, hash, Py_NewRef(value))) { Py_DECREF(mp); mp = NULL; diff --git a/Objects/listobject.c b/Objects/listobject.c index 0510d4204d2931..4eaf20033fa262 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -1287,7 +1287,7 @@ list_extend_set(PyListObject *self, PySetObject *other) Py_hash_t hash; PyObject *key; PyObject **dest = self->ob_item + m; - while (_PySet_NextEntry((PyObject *)other, &setpos, &key, &hash)) { + while (_PySet_NextEntryRef((PyObject *)other, &setpos, &key, &hash)) { FT_ATOMIC_STORE_PTR_RELEASE(*dest, key); dest++; } diff --git a/Objects/setobject.c b/Objects/setobject.c index 53f870286f3836..7af0ae166f9da3 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -2662,37 +2662,36 @@ PySet_Add(PyObject *anyset, PyObject *key) } int -_PySet_NextEntry_lock_held(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) +_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) { setentry *entry; - int ret = set_next((PySetObject *)set, pos, &entry); - if (ret == 0) { - return 0; - } - *key = Py_NewRef(entry->key); - *hash = entry->hash; - return 1; -} -int -_PySet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) -{ if (!PyAnySet_Check(set)) { PyErr_BadInternalCall(); return -1; } - _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(set); - return _PySet_NextEntry_lock_held(set, pos, key, hash); + if (set_next((PySetObject *)set, pos, &entry) == 0) + return 0; + *key = entry->key; + *hash = entry->hash; + return 1; } int -_PyFrozenSet_NextEntry(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) +_PySet_NextEntryRef(PyObject *set, Py_ssize_t *pos, PyObject **key, Py_hash_t *hash) { - if (!PyFrozenSet_CheckExact(set)) { + setentry *entry; + + if (!PyAnySet_Check(set)) { PyErr_BadInternalCall(); return -1; } - return _PySet_NextEntry_lock_held(set, pos, key, hash); + _Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(set); + if (set_next((PySetObject *)set, pos, &entry) == 0) + return 0; + *key = Py_NewRef(entry->key); + *hash = entry->hash; + return 1; } PyObject * diff --git a/Python/compile.c b/Python/compile.c index 7ceb3456489ee7..1e8f97e72cdff6 100644 --- a/Python/compile.c +++ b/Python/compile.c @@ -920,9 +920,8 @@ merge_consts_recursive(PyObject *const_cache, PyObject *o) Py_ssize_t i = 0, pos = 0; PyObject *item; Py_hash_t hash; - while (_PyFrozenSet_NextEntry(o, &pos, &item, &hash)) { + while (_PySet_NextEntry(o, &pos, &item, &hash)) { PyObject *k = merge_consts_recursive(const_cache, item); - Py_DECREF(item); if (k == NULL) { Py_DECREF(tuple); Py_DECREF(key); diff --git a/Python/marshal.c b/Python/marshal.c index 2cd2e17e5a6d76..2e6267abc0eceb 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -533,7 +533,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p) } Py_ssize_t i = 0; Py_BEGIN_CRITICAL_SECTION(v); - while (_PySet_NextEntry(v, &pos, &value, &hash)) { + while (_PySet_NextEntryRef(v, &pos, &value, &hash)) { PyObject *dump = _PyMarshal_WriteObjectToString(value, p->version, p->allow_code); if (dump == NULL) { diff --git a/Python/pylifecycle.c b/Python/pylifecycle.c index 023a3d47774a26..cc1824634e7a7f 100644 --- a/Python/pylifecycle.c +++ b/Python/pylifecycle.c @@ -2911,12 +2911,11 @@ _Py_DumpExtensionModules(int fd, PyInterpreterState *interp) PyObject *item; Py_hash_t hash; // if stdlib_module_names is not NULL, it is always a frozenset. - while (_PyFrozenSet_NextEntry(stdlib_module_names, &i, &item, &hash)) { + while (_PySet_NextEntry(stdlib_module_names, &i, &item, &hash)) { if (PyUnicode_Check(item) && PyUnicode_Compare(key, item) == 0) { is_stdlib_ext = 1; - Py_DECREF(item); break; } } From 3bd4edeafeffd1869a4d9fdce38b1fa5775d26e1 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 10:18:25 +0900 Subject: [PATCH 10/11] Fix refleaks --- Modules/_testinternalcapi/set.c | 4 +++- Objects/dictobject.c | 3 +-- Python/marshal.c | 1 + 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Modules/_testinternalcapi/set.c b/Modules/_testinternalcapi/set.c index 6cd69a739925b2..01aab03cc109ed 100644 --- a/Modules/_testinternalcapi/set.c +++ b/Modules/_testinternalcapi/set.c @@ -32,7 +32,9 @@ set_next_entry(PyObject *self, PyObject *args) rc = _PySet_NextEntryRef(set, &pos, &item, &hash); Py_END_CRITICAL_SECTION(); if (rc == 1) { - return Py_BuildValue("innO", rc, pos, hash, item); + PyObject *ret = Py_BuildValue("innO", rc, pos, hash, item); + Py_DECREF(item); + return ret; } assert(item == UNINITIALIZED_PTR); assert(hash == (Py_hash_t)UNINITIALIZED_SIZE); diff --git a/Objects/dictobject.c b/Objects/dictobject.c index f005029d7e53eb..58f34c32a87ea9 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -2983,8 +2983,7 @@ dict_set_fromkeys(PyInterpreterState *interp, PyDictObject *mp, while (_PySet_NextEntryRef(iterable, &pos, &key, &hash)) { if (insertdict(interp, mp, key, hash, Py_NewRef(value))) { Py_DECREF(mp); - mp = NULL; - break; + return NULL; } } return mp; diff --git a/Python/marshal.c b/Python/marshal.c index 2e6267abc0eceb..4274f90206b240 100644 --- a/Python/marshal.c +++ b/Python/marshal.c @@ -538,6 +538,7 @@ w_complex_object(PyObject *v, char flag, WFILE *p) p->version, p->allow_code); if (dump == NULL) { p->error = WFERR_UNMARSHALLABLE; + Py_DECREF(value); break; } PyObject *pair = PyTuple_Pack(2, dump, value); From 99fb7e67c39cf56919ab0858d6ef239f6b131802 Mon Sep 17 00:00:00 2001 From: Donghee Na Date: Thu, 18 Apr 2024 10:22:17 +0900 Subject: [PATCH 11/11] nit --- Include/internal/pycore_setobject.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Include/internal/pycore_setobject.h b/Include/internal/pycore_setobject.h index 3a8461d573a73c..41b351ead25dd1 100644 --- a/Include/internal/pycore_setobject.h +++ b/Include/internal/pycore_setobject.h @@ -15,7 +15,7 @@ PyAPI_FUNC(int) _PySet_NextEntry( PyObject **key, Py_hash_t *hash); -// Export for '_pickle' +// Export for '_pickle' shared extension PyAPI_FUNC(int) _PySet_NextEntryRef( PyObject *set, Py_ssize_t *pos,