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.

diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py
index cd077ff0e0cc7f4128cc558f3323aa314511d190..832bb9c8e2dbcc73173bf0838c6e44bf894ccbec 100644 (file)
--- 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 9fd898c5a27578edb999e5f591e5a8d057ace004..165f9a019809d235bc74831dfb9f0f4f85c5f095 100644 (file)
--- 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 ecfc0858c14933c85cb3e0737ccc951a608b8b4a..f09205f63cc5abca192e3e57bfeba3f57ab260a0 100644 (file)
--- 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 97f04186748f1fee617286f7a0e223d0576a23a7..5fff34b11946463398ded981d25f5709fb244166 100644 (file)
--- 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);
                 }