From 3d3f264849580ab1e8f3a8e8e8ba402bfe2c6523 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Thu, 15 Dec 2016 17:21:23 +0100 Subject: Fix a memory leak in split-table dictionaries Issue #28147: Fix a memory leak in split-table dictionaries: setattr() must not convert combined table into split table. Patch written by INADA Naoki. --- Lib/test/test_dict.py | 30 ++++++++++++++++++++++++++++++ Misc/NEWS | 4 ++++ Modules/_testcapimodule.c | 14 ++++++++++++++ Objects/dictobject.c | 27 ++++++++++++++++++++++----- 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index cd077ff..832bb9c 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -933,6 +933,36 @@ class DictTest(unittest.TestCase): self.assertEqual(list(a), ['x', 'y']) self.assertEqual(list(b), ['x', 'y', 'z']) + @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 = 1 + self.assertTrue(_testcapi.dict_hassplittable(a.__dict__)) + + # dict.pop() convert it to combined table + a.__dict__.pop('a') + self.assertFalse(_testcapi.dict_hassplittable(a.__dict__)) + + # But C should not convert a.__dict__ to split table again. + a.a = 1 + self.assertFalse(_testcapi.dict_hassplittable(a.__dict__)) + + # Same for popitem() + a = C() + a.a = 2 + self.assertTrue(_testcapi.dict_hassplittable(a.__dict__)) + a.__dict__.popitem() + self.assertFalse(_testcapi.dict_hassplittable(a.__dict__)) + 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 9fd898c..165f9a0 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -10,6 +10,10 @@ What's New in Python 3.6.1 release candidate 1 Core and Builtins ----------------- +- Issue #28147: Fix a memory leak in split-table dictionaries: setattr() + must not convert combined table into split table. Patch written by INADA + Naoki. + - Issue #28739: f-string expressions no longer accepted as docstrings and by ast.literal_eval() even if they do not include expressions. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index ecfc085..f09205f 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -259,6 +259,19 @@ dict_getitem_knownhash(PyObject *self, PyObject *args) return result; } +static PyObject* +dict_hassplittable(PyObject *self, PyObject *arg) +{ + if (!PyDict_Check(arg)) { + PyErr_Format(PyExc_TypeError, + "dict_hassplittable() argument must be dict, not '%s'", + arg->ob_type->tp_name); + 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 */ @@ -4024,6 +4037,7 @@ static PyMethodDef TestMethods[] = { {"test_list_api", (PyCFunction)test_list_api, METH_NOARGS}, {"test_dict_iteration", (PyCFunction)test_dict_iteration,METH_NOARGS}, {"dict_getitem_knownhash", dict_getitem_knownhash, METH_VARARGS}, + {"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 97f0418..5fff34b 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1245,7 +1245,7 @@ After resizing a table is always combined, but can be resplit by make_keys_shared(). */ static int -dictresize(PyDictObject *mp, Py_ssize_t minused) +dictresize(PyDictObject *mp, Py_ssize_t minsize) { Py_ssize_t i, newsize; PyDictKeysObject *oldkeys; @@ -1254,7 +1254,7 @@ dictresize(PyDictObject *mp, Py_ssize_t minused) /* Find the smallest table size > minused. */ for (newsize = PyDict_MINSIZE; - newsize <= minused && newsize > 0; + newsize < minsize && newsize > 0; newsize <<= 1) ; if (newsize <= 0) { @@ -1269,6 +1269,8 @@ dictresize(PyDictObject *mp, Py_ssize_t minused) mp->ma_keys = oldkeys; return -1; } + // New table must be large enough. + assert(mp->ma_keys->dk_usable >= mp->ma_used); if (oldkeys->dk_lookup == lookdict) mp->ma_keys->dk_lookup = lookdict; mp->ma_values = NULL; @@ -4306,10 +4308,25 @@ _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 */ + if (was_shared && cached != ((PyDictObject *)dict)->ma_keys) { + /* 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. + * + * This is to allow using shared key in class like this: + * + * class C: + * def __init__(self): + * # one dict resize happens + * self.a, self.b, self.c = 1, 2, 3 + * self.d, self.e, self.f = 4, 5, 6 + * a = C() + */ if (cached->dk_refcnt == 1) { CACHED_KEYS(tp) = make_keys_shared(dict); } -- cgit v0.12