From 6f8de0b75e0ca987cf5e734cf9c4bb91762efcf3 Mon Sep 17 00:00:00 2001 From: Jeroen Demeyer Date: Mon, 8 Apr 2019 14:08:37 +0200 Subject: [PATCH 1/2] bpo-36556: trashcan should not cause duplicated __del__ --- Lib/test/test_gc.py | 18 ++++++++++++++++++ .../2019-04-08-14-32-28.bpo-36556.lp-8oV.rst | 2 ++ Objects/typeobject.c | 8 ++++++++ 3 files changed, 28 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2019-04-08-14-32-28.bpo-36556.lp-8oV.rst diff --git a/Lib/test/test_gc.py b/Lib/test/test_gc.py index 7e47b2d3a27bae..3e4d0f95172833 100644 --- a/Lib/test/test_gc.py +++ b/Lib/test/test_gc.py @@ -307,6 +307,24 @@ def __del__(self): v = {1: v, 2: Ouch()} gc.disable() + def test_no_double_del(self): + # bpo-36556: instances of heap types should be deallocated once, + # even if the trashcan and __del__ are involved + class ObjectCounter(object): + count = 0 + def __init__(self): + type(self).count += 1 + def __del__(self): + # create temporary involving self, whose deallocation + # uses the trashcan + L = [self] + type(self).count -= 1 + L = None + for i in range(10000): + L = (L, ObjectCounter()) + del L + self.assertEqual(ObjectCounter.count, 0) + @unittest.skipUnless(threading, "test meaningless on builds without threads") def test_trashcan_threads(self): # Issue #13992: trashcan mechanism should be thread-safe diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-04-08-14-32-28.bpo-36556.lp-8oV.rst b/Misc/NEWS.d/next/Core and Builtins/2019-04-08-14-32-28.bpo-36556.lp-8oV.rst new file mode 100644 index 00000000000000..496bdc560c6479 --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2019-04-08-14-32-28.bpo-36556.lp-8oV.rst @@ -0,0 +1,2 @@ +When deleting highly nested objects (where the trashcan mechanism is +involved), it is less likely that ``__del__`` is called multiple times. diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 844fb00749206e..43b35b1159a9b6 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -917,6 +917,14 @@ subtype_clear(PyObject *self) return 0; } +/* bpo-36556: lower the trashcan recursion limit for heap types: this gives + * __del__ 10 additional stack frames to work with. This makes it less likely + * that the trashcan is used in __del__. Otherwise, an object might seemingly + * be resurrected by __del__ when it's still referenced by an object in the + * trashcan. */ +#undef PyTrash_UNWIND_LEVEL +#define PyTrash_UNWIND_LEVEL 40 + static void subtype_dealloc(PyObject *self) { From 0c914c2f7bc75a5e15c83023e8502f7570d52bd6 Mon Sep 17 00:00:00 2001 From: Jeroen Demeyer Date: Wed, 13 Feb 2019 15:12:48 +0100 Subject: [PATCH 2/2] [2.7] bpo-35983: skip trashcan for subclasses (GH-11841) --- Include/object.h | 60 +++++++++----- Lib/test/test_capi.py | 35 ++++++++ .../2019-02-13-16-47-19.bpo-35983.bNxsXv.rst | 3 + Modules/_testcapimodule.c | 76 ++++++++++++++++++ Objects/descrobject.c | 4 +- Objects/dictobject.c | 4 +- Objects/listobject.c | 4 +- Objects/setobject.c | 4 +- Objects/tupleobject.c | 4 +- Objects/typeobject.c | 79 +------------------ Python/traceback.c | 4 +- 11 files changed, 167 insertions(+), 110 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2019-02-13-16-47-19.bpo-35983.bNxsXv.rst diff --git a/Include/object.h b/Include/object.h index 807b24188a75b2..cc2f3c5375f17c 100644 --- a/Include/object.h +++ b/Include/object.h @@ -969,11 +969,11 @@ times. When deallocating a container object, it's possible to trigger an unbounded chain of deallocations, as each Py_DECREF in turn drops the refcount on "the -next" object in the chain to 0. This can easily lead to stack faults, and +next" object in the chain to 0. This can easily lead to stack overflows, especially in threads (which typically have less stack space to work with). -A container object that participates in cyclic gc can avoid this by -bracketing the body of its tp_dealloc function with a pair of macros: +A container object can avoid this by bracketing the body of its tp_dealloc +function with a pair of macros: static void mytype_dealloc(mytype *p) @@ -981,14 +981,14 @@ mytype_dealloc(mytype *p) ... declarations go here ... PyObject_GC_UnTrack(p); // must untrack first - Py_TRASHCAN_SAFE_BEGIN(p) + Py_TRASHCAN_BEGIN(p, mytype_dealloc) ... The body of the deallocator goes here, including all calls ... ... to Py_DECREF on contained objects. ... - Py_TRASHCAN_SAFE_END(p) + Py_TRASHCAN_END // there should be no code after this } CAUTION: Never return from the middle of the body! If the body needs to -"get out early", put a label immediately before the Py_TRASHCAN_SAFE_END +"get out early", put a label immediately before the Py_TRASHCAN_END call, and goto it. Else the call-depth counter (see below) will stay above 0 forever, and the trashcan will never get emptied. @@ -1004,6 +1004,12 @@ notices this, and calls another routine to deallocate all the objects that may have been added to the list of deferred deallocations. In effect, a chain of N deallocations is broken into N / PyTrash_UNWIND_LEVEL pieces, with the call stack never exceeding a depth of PyTrash_UNWIND_LEVEL. + +Since the tp_dealloc of a subclass typically calls the tp_dealloc of the base +class, we need to ensure that the trashcan is only triggered on the tp_dealloc +of the actual class being deallocated. Otherwise we might end up with a +partially-deallocated object. To check this, the tp_dealloc function must be +passed as second argument to Py_TRASHCAN_BEGIN(). */ /* This is the old private API, invoked by the macros before 2.7.4. @@ -1020,26 +1026,38 @@ PyAPI_FUNC(void) _PyTrash_thread_destroy_chain(void); #define PyTrash_UNWIND_LEVEL 50 /* Note the workaround for when the thread state is NULL (issue #17703) */ -#define Py_TRASHCAN_SAFE_BEGIN(op) \ +#define Py_TRASHCAN_BEGIN_CONDITION(op, cond) \ do { \ - PyThreadState *_tstate = PyThreadState_GET(); \ - if (!_tstate || \ - _tstate->trash_delete_nesting < PyTrash_UNWIND_LEVEL) { \ - if (_tstate) \ - ++_tstate->trash_delete_nesting; - /* The body of the deallocator is here. */ -#define Py_TRASHCAN_SAFE_END(op) \ - if (_tstate) { \ - --_tstate->trash_delete_nesting; \ - if (_tstate->trash_delete_later \ - && _tstate->trash_delete_nesting <= 0) \ - _PyTrash_thread_destroy_chain(); \ + PyThreadState *_tstate = NULL; \ + /* If "cond" is false, then _tstate remains NULL and the deallocator \ + * is run normally without involving the trashcan */ \ + if (cond && (_tstate = PyThreadState_GET()) != NULL) { \ + if (_tstate->trash_delete_nesting >= PyTrash_UNWIND_LEVEL) { \ + /* Store the object (to be deallocated later) and jump past \ + * Py_TRASHCAN_END, skipping the body of the deallocator */ \ + _PyTrash_thread_deposit_object((PyObject*)op); \ + break; \ } \ + ++_tstate->trash_delete_nesting; \ + } + /* The body of the deallocator is here. */ +#define Py_TRASHCAN_END \ + if (_tstate) { \ + --_tstate->trash_delete_nesting; \ + if (_tstate->trash_delete_later && _tstate->trash_delete_nesting <= 0) \ + _PyTrash_thread_destroy_chain(); \ } \ - else \ - _PyTrash_thread_deposit_object((PyObject*)op); \ } while (0); +#define Py_TRASHCAN_BEGIN(op, dealloc) Py_TRASHCAN_BEGIN_CONDITION(op, \ + Py_TYPE(op)->tp_dealloc == (destructor)(dealloc)) + +/* For backwards compatibility, these macros enable the trashcan + * unconditionally */ +#define Py_TRASHCAN_SAFE_BEGIN(op) Py_TRASHCAN_BEGIN_CONDITION(op, 1) +#define Py_TRASHCAN_SAFE_END(op) Py_TRASHCAN_END + + #ifdef __cplusplus } #endif diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 5eb3f7d4c75565..1431e96690f748 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -22,6 +22,41 @@ class CAPITest(unittest.TestCase): def test_buildvalue_N(self): _testcapi.test_buildvalue_N() + def test_trashcan_subclass(self): + # bpo-35983: Check that the trashcan mechanism for "list" is NOT + # activated when its tp_dealloc is being called by a subclass + from _testcapi import MyList + L = None + for i in range(1000): + L = MyList((L,)) + + def test_trashcan_python_class(self): + # Check that the trashcan mechanism works properly for a Python + # subclass of a class using the trashcan (list in this test) + class PyList(list): + # Count the number of PyList instances to verify that there is + # no memory leak + num = 0 + def __init__(self, *args): + PyList.num += 1 + list.__init__(self, *args) + def __del__(self): + PyList.num -= 1 + + for parity in (0, 1): + L = None + # We need in the order of 2**20 iterations here such that a + # typical 8MB stack would overflow without the trashcan. + for i in range(2**20): + L = PyList((L,)) + L.attr = i + if parity: + # Add one additional nesting layer + L = (L,) + self.assertGreater(PyList.num, 0) + del L + self.assertEqual(PyList.num, 0) + @unittest.skipUnless(threading, 'Threading required for this test.') class TestPendingCalls(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-02-13-16-47-19.bpo-35983.bNxsXv.rst b/Misc/NEWS.d/next/Core and Builtins/2019-02-13-16-47-19.bpo-35983.bNxsXv.rst new file mode 100644 index 00000000000000..1138df635dbbdc --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2019-02-13-16-47-19.bpo-35983.bNxsXv.rst @@ -0,0 +1,3 @@ +Added new trashcan macros to deal with a double deallocation that could occur +when the `tp_dealloc` of a subclass calls the `tp_dealloc` of a base class +and that base class uses the trashcan mechanism. Patch by Jeroen Demeyer. diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 67488e70e45ff1..fb866fa133b3ed 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -2944,6 +2944,76 @@ static PyTypeObject test_structmembersType = { }; +/* Test bpo-35983: create a subclass of "list" which checks that instances + * are not deallocated twice */ + +typedef struct { + PyListObject list; + int deallocated; +} MyListObject; + +static PyObject * +MyList_new(PyTypeObject *type, PyObject *args, PyObject *kwds) +{ + PyObject* op = PyList_Type.tp_new(type, args, kwds); + ((MyListObject*)op)->deallocated = 0; + return op; +} + +void +MyList_dealloc(MyListObject* op) +{ + if (op->deallocated) { + /* We cannot raise exceptions here but we still want the testsuite + * to fail when we hit this */ + Py_FatalError("MyList instance deallocated twice"); + } + op->deallocated = 1; + PyList_Type.tp_dealloc((PyObject *)op); +} + +static PyTypeObject MyList_Type = { + PyVarObject_HEAD_INIT(NULL, 0) + "MyList", + sizeof(MyListObject), + 0, + (destructor)MyList_dealloc, /* tp_dealloc */ + 0, /* tp_print */ + 0, /* tp_getattr */ + 0, /* tp_setattr */ + 0, /* tp_reserved */ + 0, /* tp_repr */ + 0, /* tp_as_number */ + 0, /* tp_as_sequence */ + 0, /* tp_as_mapping */ + 0, /* tp_hash */ + 0, /* tp_call */ + 0, /* tp_str */ + 0, /* tp_getattro */ + 0, /* tp_setattro */ + 0, /* tp_as_buffer */ + Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */ + 0, /* tp_doc */ + 0, /* tp_traverse */ + 0, /* tp_clear */ + 0, /* tp_richcompare */ + 0, /* tp_weaklistoffset */ + 0, /* tp_iter */ + 0, /* tp_iternext */ + 0, /* tp_methods */ + 0, /* tp_members */ + 0, /* tp_getset */ + 0, /* &PyList_Type */ /* tp_base */ + 0, /* tp_dict */ + 0, /* tp_descr_get */ + 0, /* tp_descr_set */ + 0, /* tp_dictoffset */ + 0, /* tp_init */ + 0, /* tp_alloc */ + MyList_new, /* tp_new */ +}; + + PyMODINIT_FUNC init_testcapi(void) { @@ -2961,6 +3031,12 @@ init_testcapi(void) test_capi to automatically call this */ PyModule_AddObject(m, "_test_structmembersType", (PyObject *)&test_structmembersType); + MyList_Type.tp_base = &PyList_Type; + if (PyType_Ready(&MyList_Type) < 0) + return NULL; + Py_INCREF(&MyList_Type); + PyModule_AddObject(m, "MyList", (PyObject *)&MyList_Type); + PyModule_AddObject(m, "CHAR_MAX", PyInt_FromLong(CHAR_MAX)); PyModule_AddObject(m, "CHAR_MIN", PyInt_FromLong(CHAR_MIN)); PyModule_AddObject(m, "UCHAR_MAX", PyInt_FromLong(UCHAR_MAX)); diff --git a/Objects/descrobject.c b/Objects/descrobject.c index 8d6e6e3a246717..38f11e918b51d3 100644 --- a/Objects/descrobject.c +++ b/Objects/descrobject.c @@ -940,11 +940,11 @@ static void wrapper_dealloc(wrapperobject *wp) { PyObject_GC_UnTrack(wp); - Py_TRASHCAN_SAFE_BEGIN(wp) + Py_TRASHCAN_BEGIN(wp, wrapper_dealloc) Py_XDECREF(wp->descr); Py_XDECREF(wp->self); PyObject_GC_Del(wp); - Py_TRASHCAN_SAFE_END(wp) + Py_TRASHCAN_END } static int diff --git a/Objects/dictobject.c b/Objects/dictobject.c index c544ecd8c2d254..fa2b4b7c53e92c 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -1078,7 +1078,7 @@ dict_dealloc(register PyDictObject *mp) Py_ssize_t fill = mp->ma_fill; /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(mp); - Py_TRASHCAN_SAFE_BEGIN(mp) + Py_TRASHCAN_BEGIN(mp, dict_dealloc) for (ep = mp->ma_table; fill > 0; ep++) { if (ep->me_key) { --fill; @@ -1092,7 +1092,7 @@ dict_dealloc(register PyDictObject *mp) free_list[numfree++] = mp; else Py_TYPE(mp)->tp_free((PyObject *)mp); - Py_TRASHCAN_SAFE_END(mp) + Py_TRASHCAN_END } static int diff --git a/Objects/listobject.c b/Objects/listobject.c index 24eff769c64e70..dc57270eb43252 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -298,7 +298,7 @@ list_dealloc(PyListObject *op) { Py_ssize_t i; PyObject_GC_UnTrack(op); - Py_TRASHCAN_SAFE_BEGIN(op) + Py_TRASHCAN_BEGIN(op, list_dealloc) if (op->ob_item != NULL) { /* Do it backwards, for Christian Tismer. There's a simple test case where somehow this reduces @@ -314,7 +314,7 @@ list_dealloc(PyListObject *op) free_list[numfree++] = op; else Py_TYPE(op)->tp_free((PyObject *)op); - Py_TRASHCAN_SAFE_END(op) + Py_TRASHCAN_END } static int diff --git a/Objects/setobject.c b/Objects/setobject.c index 31da3dbfecb174..3dbbcf378096fc 100644 --- a/Objects/setobject.c +++ b/Objects/setobject.c @@ -551,7 +551,7 @@ set_dealloc(PySetObject *so) Py_ssize_t fill = so->fill; /* bpo-31095: UnTrack is needed before calling any callbacks */ PyObject_GC_UnTrack(so); - Py_TRASHCAN_SAFE_BEGIN(so) + Py_TRASHCAN_BEGIN(so, set_dealloc) if (so->weakreflist != NULL) PyObject_ClearWeakRefs((PyObject *) so); @@ -567,7 +567,7 @@ set_dealloc(PySetObject *so) free_list[numfree++] = so; else Py_TYPE(so)->tp_free(so); - Py_TRASHCAN_SAFE_END(so) + Py_TRASHCAN_END } static int diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c index 6f4b18cc5c65a8..ae8216377525a1 100644 --- a/Objects/tupleobject.c +++ b/Objects/tupleobject.c @@ -215,7 +215,7 @@ tupledealloc(register PyTupleObject *op) register Py_ssize_t i; register Py_ssize_t len = Py_SIZE(op); PyObject_GC_UnTrack(op); - Py_TRASHCAN_SAFE_BEGIN(op) + Py_TRASHCAN_BEGIN(op, tupledealloc) if (len > 0) { i = len; while (--i >= 0) @@ -234,7 +234,7 @@ tupledealloc(register PyTupleObject *op) } Py_TYPE(op)->tp_free((PyObject *)op); done: - Py_TRASHCAN_SAFE_END(op) + Py_TRASHCAN_END } static int diff --git a/Objects/typeobject.c b/Objects/typeobject.c index 43b35b1159a9b6..3bb93863013667 100644 --- a/Objects/typeobject.c +++ b/Objects/typeobject.c @@ -930,7 +930,6 @@ subtype_dealloc(PyObject *self) { PyTypeObject *type, *base; destructor basedealloc; - PyThreadState *tstate = PyThreadState_GET(); /* Extract the type; we expect it to be a heap type */ type = Py_TYPE(self); @@ -979,16 +978,7 @@ subtype_dealloc(PyObject *self) /* UnTrack and re-Track around the trashcan macro, alas */ /* See explanation at end of function for full disclosure */ PyObject_GC_UnTrack(self); - ++_PyTrash_delete_nesting; - ++ tstate->trash_delete_nesting; - Py_TRASHCAN_SAFE_BEGIN(self); - --_PyTrash_delete_nesting; - -- tstate->trash_delete_nesting; - /* DO NOT restore GC tracking at this point. weakref callbacks - * (if any, and whether directly here or indirectly in something we - * call) may trigger GC, and if self is tracked at that point, it - * will look like trash to GC and GC will try to delete self again. - */ + Py_TRASHCAN_BEGIN(self, subtype_dealloc); /* Find the nearest base with a different tp_dealloc */ base = type; @@ -1061,11 +1051,7 @@ subtype_dealloc(PyObject *self) Py_DECREF(type); endlabel: - ++_PyTrash_delete_nesting; - ++ tstate->trash_delete_nesting; - Py_TRASHCAN_SAFE_END(self); - --_PyTrash_delete_nesting; - -- tstate->trash_delete_nesting; + Py_TRASHCAN_END /* Explanation of the weirdness around the trashcan macros: @@ -1102,67 +1088,6 @@ subtype_dealloc(PyObject *self) looks like trash to gc too, and gc also tries to delete self then. But we're already deleting self. Double deallocation is a subtle disaster. - - Q. Why the bizarre (net-zero) manipulation of - _PyTrash_delete_nesting around the trashcan macros? - - A. Some base classes (e.g. list) also use the trashcan mechanism. - The following scenario used to be possible: - - - suppose the trashcan level is one below the trashcan limit - - - subtype_dealloc() is called - - - the trashcan limit is not yet reached, so the trashcan level - is incremented and the code between trashcan begin and end is - executed - - - this destroys much of the object's contents, including its - slots and __dict__ - - - basedealloc() is called; this is really list_dealloc(), or - some other type which also uses the trashcan macros - - - the trashcan limit is now reached, so the object is put on the - trashcan's to-be-deleted-later list - - - basedealloc() returns - - - subtype_dealloc() decrefs the object's type - - - subtype_dealloc() returns - - - later, the trashcan code starts deleting the objects from its - to-be-deleted-later list - - - subtype_dealloc() is called *AGAIN* for the same object - - - at the very least (if the destroyed slots and __dict__ don't - cause problems) the object's type gets decref'ed a second - time, which is *BAD*!!! - - The remedy is to make sure that if the code between trashcan - begin and end in subtype_dealloc() is called, the code between - trashcan begin and end in basedealloc() will also be called. - This is done by decrementing the level after passing into the - trashcan block, and incrementing it just before leaving the - block. - - But now it's possible that a chain of objects consisting solely - of objects whose deallocator is subtype_dealloc() will defeat - the trashcan mechanism completely: the decremented level means - that the effective level never reaches the limit. Therefore, we - *increment* the level *before* entering the trashcan block, and - matchingly decrement it after leaving. This means the trashcan - code will trigger a little early, but that's no big deal. - - Q. Are there any live examples of code in need of all this - complexity? - - A. Yes. See SF bug 668433 for code that crashed (when Python was - compiled in debug mode) before the trashcan level manipulations - were added. For more discussion, see SF patches 581742, 575073 - and bug 574207. */ } diff --git a/Python/traceback.c b/Python/traceback.c index fd5309ae3f7ebf..38327b6f173323 100644 --- a/Python/traceback.c +++ b/Python/traceback.c @@ -23,11 +23,11 @@ static void tb_dealloc(PyTracebackObject *tb) { PyObject_GC_UnTrack(tb); - Py_TRASHCAN_SAFE_BEGIN(tb) + Py_TRASHCAN_BEGIN(tb, tb_dealloc) Py_XDECREF(tb->tb_next); Py_XDECREF(tb->tb_frame); PyObject_GC_Del(tb); - Py_TRASHCAN_SAFE_END(tb) + Py_TRASHCAN_END } static int