diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index 6d68e761c64..b85d333dcbe 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -836,6 +836,24 @@ class DictTest(unittest.TestCase): pass self._tracked(MyDict()) + @support.cpython_only + def test_splittable_setattr_after_pop(self): + """setattr must not convert combined table into split table""" + # Issue 28147 + import _testcapi + + class C: + pass + a = C() + a.a = 2 + self.assertTrue(_testcapi.dict_hassplittable(a.__dict__)) + # dict.popitem() convert it to combined table + a.__dict__.popitem() + self.assertFalse(_testcapi.dict_hassplittable(a.__dict__)) + # But C should not convert a.__dict__ to split table again. + a.a = 3 + self.assertFalse(_testcapi.dict_hassplittable(a.__dict__)) + def test_iterator_pickling(self): for proto in range(pickle.HIGHEST_PROTOCOL + 1): data = {1:"a", 2:"b", 3:"c"} diff --git a/Misc/NEWS b/Misc/NEWS index 33b777b0f2e..f09f0ac5376 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,9 @@ Release date: TBA Core and Builtins ----------------- +- Issue #28147: Fix a memory leak in split-table dictionaries: setattr() + must not convert combined table into split table. + - Issue #25677: Correct the positioning of the syntax error caret for indented blocks. Based on patch by Michael Layzell. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 29a28a7a34e..060a92da4b0 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -249,6 +249,15 @@ test_dict_iteration(PyObject* self) } +static PyObject* +dict_hassplittable(PyObject *self, PyObject *arg) +{ + if (!PyArg_Parse(arg, "O!:dict_hassplittable", &PyDict_Type, &arg)) { + return NULL; + } + return PyBool_FromLong(_PyDict_HasSplitTable((PyDictObject*)arg)); +} + /* Issue #4701: Check that PyObject_Hash implicitly calls * PyType_Ready if it hasn't already been called */ @@ -3858,6 +3867,7 @@ static PyMethodDef TestMethods[] = { {"test_datetime_capi", test_datetime_capi, METH_NOARGS}, {"test_list_api", (PyCFunction)test_list_api, METH_NOARGS}, {"test_dict_iteration", (PyCFunction)test_dict_iteration,METH_NOARGS}, + {"dict_hassplittable", dict_hassplittable, METH_O}, {"test_lazy_hash_inheritance", (PyCFunction)test_lazy_hash_inheritance,METH_NOARGS}, {"test_long_api", (PyCFunction)test_long_api, METH_NOARGS}, {"test_xincref_doesnt_leak",(PyCFunction)test_xincref_doesnt_leak, METH_NOARGS}, diff --git a/Objects/dictobject.c b/Objects/dictobject.c index e79ab36eb2c..795c1f6de15 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -985,8 +985,10 @@ make_keys_shared(PyObject *op) return NULL; } else if (mp->ma_keys->dk_lookup == lookdict_unicode) { - /* Remove dummy keys */ - if (dictresize(mp, DK_SIZE(mp->ma_keys))) + /* Remove dummy keys + * -1 is required since dictresize() uses key size > minused + */ + if (dictresize(mp, DK_SIZE(mp->ma_keys) - 1)) return NULL; } assert(mp->ma_keys->dk_lookup == lookdict_unicode_nodummy); @@ -2473,7 +2475,8 @@ dict_popitem(PyDictObject *mp) } /* Convert split table to combined table */ if (mp->ma_keys->dk_lookup == lookdict_split) { - if (dictresize(mp, DK_SIZE(mp->ma_keys))) { + /* -1 is required since dictresize() uses key size > minused */ + if (dictresize(mp, DK_SIZE(mp->ma_keys) - 1)) { Py_DECREF(res); return NULL; } @@ -3848,10 +3851,16 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr, CACHED_KEYS(tp) = NULL; DK_DECREF(cached); } - } else { + } + else { + int was_shared = cached == ((PyDictObject *)dict)->ma_keys; res = PyDict_SetItem(dict, key, value); - if (cached != ((PyDictObject *)dict)->ma_keys) { - /* Either update tp->ht_cached_keys or delete it */ + /* PyDict_SetItem() may call dictresize() and convert split table + * into combined table. In such case, convert it to split + * table again and update type's shared key only when this is + * the only dict sharing key with the type. + */ + if (was_shared && cached != ((PyDictObject *)dict)->ma_keys) { if (cached->dk_refcnt == 1) { CACHED_KEYS(tp) = make_keys_shared(dict); } else {