Skip to content

Commit 5c86e23

Browse files
jdemeyerjdemeyer
committed
bpo-35983: skip trashcan for subclasses
1 parent 1fc5bf2 commit 5c86e23

File tree

14 files changed

+173
-119
lines changed

14 files changed

+173
-119
lines changed

‎Include/object.h‎

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -651,26 +651,26 @@ times.
651651
652652
When deallocating a container object, it's possible to trigger an unbounded
653653
chain of deallocations, as each Py_DECREF in turn drops the refcount on "the
654-
next" object in the chain to 0. This can easily lead to stack faults, and
654+
next" object in the chain to 0. This can easily lead to stack overflows,
655655
especially in threads (which typically have less stack space to work with).
656656
657-
A container object that participates in cyclic gc can avoid this by
658-
bracketing the body of its tp_dealloc function with a pair of macros:
657+
A container object can avoid this by bracketing the body of its tp_dealloc
658+
function with a pair of macros:
659659
660660
static void
661661
mytype_dealloc(mytype *p)
662662
{
663663
... declarations go here ...
664664
665665
PyObject_GC_UnTrack(p); // must untrack first
666-
Py_TRASHCAN_SAFE_BEGIN(p)
666+
Py_TRASHCAN_BEGIN(p, mytype_dealloc)
667667
... The body of the deallocator goes here, including all calls ...
668668
... to Py_DECREF on contained objects. ...
669-
Py_TRASHCAN_SAFE_END(p)
669+
Py_TRASHCAN_END
670670
}
671671
672672
CAUTION: Never return from the middle of the body! If the body needs to
673-
"get out early", put a label immediately before the Py_TRASHCAN_SAFE_END
673+
"get out early", put a label immediately before the Py_TRASHCAN_END
674674
call, and goto it. Else the call-depth counter (see below) will stay
675675
above 0 forever, and the trashcan will never get emptied.
676676
@@ -686,6 +686,12 @@ notices this, and calls another routine to deallocate all the objects that
686686
may have been added to the list of deferred deallocations. In effect, a
687687
chain of N deallocations is broken into (N-1)/(PyTrash_UNWIND_LEVEL-1) pieces,
688688
with the call stack never exceeding a depth of PyTrash_UNWIND_LEVEL.
689+
690+
Since the tp_dealloc of a subclass typically calls the tp_dealloc of the base
691+
class, we need to ensure that the trashcan is only triggered on the tp_dealloc
692+
of the actual class being deallocated. Otherwise we might end up with a
693+
partially-deallocated object. To check this, the tp_dealloc function must be
694+
passed as second argument to Py_TRASHCAN_BEGIN().
689695
*/
690696

691697
/* The new thread-safe private API, invoked by the macros below. */
@@ -694,21 +700,38 @@ PyAPI_FUNC(void) _PyTrash_thread_destroy_chain(void);
694700

695701
#define PyTrash_UNWIND_LEVEL 50
696702

697-
#define Py_TRASHCAN_SAFE_BEGIN(op) \
703+
#define Py_TRASHCAN_BEGIN_CONDITION(op, cond) \
698704
do { \
699-
PyThreadState *_tstate = PyThreadState_GET(); \
700-
if (_tstate->trash_delete_nesting < PyTrash_UNWIND_LEVEL) { \
701-
++_tstate->trash_delete_nesting;
702-
/* The body of the deallocator is here. */
703-
#define Py_TRASHCAN_SAFE_END(op) \
705+
PyThreadState *_tstate = NULL; \
706+
/* If "cond" is false, then _tstate remains NULL and the deallocator \
707+
* is run normally without involving the trashcan */ \
708+
if (cond) { \
709+
_tstate = PyThreadState_GET(); \
710+
if (_tstate->trash_delete_nesting >= PyTrash_UNWIND_LEVEL) { \
711+
/* Store the object (to be deallocated later) and jump past \
712+
* Py_TRASHCAN_END, skipping the body of the deallocator */ \
713+
_PyTrash_thread_deposit_object(_PyObject_CAST(op)); \
714+
break; \
715+
} \
716+
++_tstate->trash_delete_nesting; \
717+
}
718+
/* The body of the deallocator is here. */
719+
#define Py_TRASHCAN_END \
720+
if (_tstate) { \
704721
--_tstate->trash_delete_nesting; \
705722
if (_tstate->trash_delete_later && _tstate->trash_delete_nesting <= 0) \
706723
_PyTrash_thread_destroy_chain(); \
707724
} \
708-
else \
709-
_PyTrash_thread_deposit_object(_PyObject_CAST(op)); \
710725
} while (0);
711726

727+
#define Py_TRASHCAN_BEGIN(op, dealloc) Py_TRASHCAN_BEGIN_CONDITION(op, \
728+
Py_TYPE(op)->tp_dealloc == (destructor)(dealloc))
729+
730+
/* For backwards compatibility, these macros enable the trashcan
731+
* unconditionally */
732+
#define Py_TRASHCAN_SAFE_BEGIN(op) Py_TRASHCAN_BEGIN_CONDITION(op, 1)
733+
#define Py_TRASHCAN_SAFE_END(op) Py_TRASHCAN_END
734+
712735

713736
#ifndef Py_LIMITED_API
714737
# define Py_CPYTHON_OBJECT_H

‎Lib/test/test_capi.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,39 @@ def test_negative_refcount(self):
333333
br'_Py_NegativeRefcount: Assertion failed: '
334334
br'object has negative ref count')
335335

336+
def test_trashcan_subclass(self):
337+
# bpo-35983: Check that the trashcan mechanism for "list" is NOT
338+
# activated when its tp_dealloc is being called by a subclass
339+
from _testcapi import MyList
340+
L = None
341+
for i in range(1000):
342+
L = MyList((L,))
343+
344+
def test_trashcan_python_class(self):
345+
# Check that the trashcan mechanism works properly for a Python
346+
# subclass of a class using the trashcan (list in this test)
347+
class PyList(list):
348+
# Count the number of PyList instances to verify that there is
349+
# no memory leak
350+
num = 0
351+
def __init__(self, *args):
352+
__class__.num += 1
353+
super().__init__(*args)
354+
def __del__(self):
355+
__class__.num -= 1
356+
357+
for parity in (0, 1):
358+
L = None
359+
for i in range(2**20):
360+
L = PyList((L,))
361+
L.attr = i
362+
if parity:
363+
# Add one additional nesting layer
364+
L = (L,)
365+
self.assertGreater(PyList.num, 0)
366+
del L
367+
self.assertEqual(PyList.num, 0)
368+
336369

337370
class TestPendingCalls(unittest.TestCase):
338371

‎Misc/NEWS.d/next/Core and Builtins/2019-02-13-16-47-19.bpo-35983.bNxsXv.rst‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Added new trashcan macros to deal with a double deallocation that could occur
2+
when the `tp_dealloc` of a subclass calls the `tp_dealloc` of a base class
3+
and that base class uses the trashcan mechanism. Patch by Jeroen Demeyer.

‎Modules/_elementtree.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -663,7 +663,7 @@ element_dealloc(ElementObject* self)
663663
{
664664
/* bpo-31095: UnTrack is needed before calling any callbacks */
665665
PyObject_GC_UnTrack(self);
666-
Py_TRASHCAN_SAFE_BEGIN(self)
666+
Py_TRASHCAN_BEGIN(self, element_dealloc)
667667

668668
if (self->weakreflist != NULL)
669669
PyObject_ClearWeakRefs((PyObject *) self);
@@ -674,7 +674,7 @@ element_dealloc(ElementObject* self)
674674

675675
RELEASE(sizeof(ElementObject), "destroy element");
676676
Py_TYPE(self)->tp_free((PyObject *)self);
677-
Py_TRASHCAN_SAFE_END(self)
677+
Py_TRASHCAN_END
678678
}
679679

680680
/* -------------------------------------------------------------------- */

‎Modules/_testcapimodule.c‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5330,6 +5330,76 @@ recurse_infinitely_error_init(PyObject *self, PyObject *args, PyObject *kwds)
53305330
}
53315331

53325332

5333+
/* Test bpo-35983: create a subclass of "list" which checks that instances
5334+
* are not deallocated twice */
5335+
5336+
typedef struct {
5337+
PyListObject list;
5338+
int deallocated;
5339+
} MyListObject;
5340+
5341+
static PyObject *
5342+
MyList_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
5343+
{
5344+
PyObject* op = PyList_Type.tp_new(type, args, kwds);
5345+
((MyListObject*)op)->deallocated = 0;
5346+
return op;
5347+
}
5348+
5349+
void
5350+
MyList_dealloc(MyListObject* op)
5351+
{
5352+
if (op->deallocated) {
5353+
/* We cannot raise exceptions here but we still want the testsuite
5354+
* to fail when we hit this */
5355+
abort();
5356+
}
5357+
op->deallocated = 1;
5358+
PyList_Type.tp_dealloc((PyObject *)op);
5359+
}
5360+
5361+
static PyTypeObject MyList_Type = {
5362+
PyVarObject_HEAD_INIT(NULL, 0)
5363+
"MyList",
5364+
sizeof(MyListObject),
5365+
0,
5366+
(destructor)MyList_dealloc, /* tp_dealloc */
5367+
0, /* tp_print */
5368+
0, /* tp_getattr */
5369+
0, /* tp_setattr */
5370+
0, /* tp_reserved */
5371+
0, /* tp_repr */
5372+
0, /* tp_as_number */
5373+
0, /* tp_as_sequence */
5374+
0, /* tp_as_mapping */
5375+
0, /* tp_hash */
5376+
0, /* tp_call */
5377+
0, /* tp_str */
5378+
0, /* tp_getattro */
5379+
0, /* tp_setattro */
5380+
0, /* tp_as_buffer */
5381+
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
5382+
0, /* tp_doc */
5383+
0, /* tp_traverse */
5384+
0, /* tp_clear */
5385+
0, /* tp_richcompare */
5386+
0, /* tp_weaklistoffset */
5387+
0, /* tp_iter */
5388+
0, /* tp_iternext */
5389+
0, /* tp_methods */
5390+
0, /* tp_members */
5391+
0, /* tp_getset */
5392+
0, /* &PyList_Type */ /* tp_base */
5393+
0, /* tp_dict */
5394+
0, /* tp_descr_get */
5395+
0, /* tp_descr_set */
5396+
0, /* tp_dictoffset */
5397+
0, /* tp_init */
5398+
0, /* tp_alloc */
5399+
MyList_new, /* tp_new */
5400+
};
5401+
5402+
53335403
/* Test PEP 560 */
53345404

53355405
typedef struct {
@@ -5443,6 +5513,12 @@ PyInit__testcapi(void)
54435513
Py_INCREF(&awaitType);
54445514
PyModule_AddObject(m, "awaitType", (PyObject *)&awaitType);
54455515

5516+
MyList_Type.tp_base = &PyList_Type;
5517+
if (PyType_Ready(&MyList_Type) < 0)
5518+
return NULL;
5519+
Py_INCREF(&MyList_Type);
5520+
PyModule_AddObject(m, "MyList", (PyObject *)&MyList_Type);
5521+
54465522
if (PyType_Ready(&GenericAlias_Type) < 0)
54475523
return NULL;
54485524
Py_INCREF(&GenericAlias_Type);

‎Objects/descrobject.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,11 +1023,11 @@ static void
10231023
wrapper_dealloc(wrapperobject *wp)
10241024
{
10251025
PyObject_GC_UnTrack(wp);
1026-
Py_TRASHCAN_SAFE_BEGIN(wp)
1026+
Py_TRASHCAN_BEGIN(wp, wrapper_dealloc)
10271027
Py_XDECREF(wp->descr);
10281028
Py_XDECREF(wp->self);
10291029
PyObject_GC_Del(wp);
1030-
Py_TRASHCAN_SAFE_END(wp)
1030+
Py_TRASHCAN_END
10311031
}
10321032

10331033
static PyObject *

‎Objects/dictobject.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1977,7 +1977,7 @@ dict_dealloc(PyDictObject *mp)
19771977

19781978
/* bpo-31095: UnTrack is needed before calling any callbacks */
19791979
PyObject_GC_UnTrack(mp);
1980-
Py_TRASHCAN_SAFE_BEGIN(mp)
1980+
Py_TRASHCAN_BEGIN(mp, dict_dealloc)
19811981
if (values != NULL) {
19821982
if (values != empty_values) {
19831983
for (i = 0, n = mp->ma_keys->dk_nentries; i < n; i++) {
@@ -1995,7 +1995,7 @@ dict_dealloc(PyDictObject *mp)
19951995
free_list[numfree++] = mp;
19961996
else
19971997
Py_TYPE(mp)->tp_free((PyObject *)mp);
1998-
Py_TRASHCAN_SAFE_END(mp)
1998+
Py_TRASHCAN_END
19991999
}
20002000

20012001

‎Objects/listobject.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -361,7 +361,7 @@ list_dealloc(PyListObject *op)
361361
{
362362
Py_ssize_t i;
363363
PyObject_GC_UnTrack(op);
364-
Py_TRASHCAN_SAFE_BEGIN(op)
364+
Py_TRASHCAN_BEGIN(op, list_dealloc)
365365
if (op->ob_item != NULL) {
366366
/* Do it backwards, for Christian Tismer.
367367
There's a simple test case where somehow this reduces
@@ -377,7 +377,7 @@ list_dealloc(PyListObject *op)
377377
free_list[numfree++] = op;
378378
else
379379
Py_TYPE(op)->tp_free((PyObject *)op);
380-
Py_TRASHCAN_SAFE_END(op)
380+
Py_TRASHCAN_END
381381
}
382382

383383
static PyObject *

‎Objects/odictobject.c‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1356,28 +1356,17 @@ static PyGetSetDef odict_getset[] = {
13561356
static void
13571357
odict_dealloc(PyODictObject *self)
13581358
{
1359-
PyThreadState *tstate = _PyThreadState_GET();
1360-
13611359
PyObject_GC_UnTrack(self);
1362-
Py_TRASHCAN_SAFE_BEGIN(self)
1360+
Py_TRASHCAN_BEGIN(self, odict_dealloc)
13631361

13641362
Py_XDECREF(self->od_inst_dict);
13651363
if (self->od_weakreflist != NULL)
13661364
PyObject_ClearWeakRefs((PyObject *)self);
13671365

13681366
_odict_clear_nodes(self);
1369-
1370-
/* Call the base tp_dealloc(). Since it too uses the trashcan mechanism,
1371-
* temporarily decrement trash_delete_nesting to prevent triggering it
1372-
* and putting the partially deallocated object on the trashcan's
1373-
* to-be-deleted-later list.
1374-
*/
1375-
--tstate->trash_delete_nesting;
1376-
assert(_tstate->trash_delete_nesting < PyTrash_UNWIND_LEVEL);
13771367
PyDict_Type.tp_dealloc((PyObject *)self);
1378-
++tstate->trash_delete_nesting;
13791368

1380-
Py_TRASHCAN_SAFE_END(self)
1369+
Py_TRASHCAN_END
13811370
}
13821371

13831372
/* tp_repr */

‎Objects/setobject.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -559,7 +559,7 @@ set_dealloc(PySetObject *so)
559559

560560
/* bpo-31095: UnTrack is needed before calling any callbacks */
561561
PyObject_GC_UnTrack(so);
562-
Py_TRASHCAN_SAFE_BEGIN(so)
562+
Py_TRASHCAN_BEGIN(so, set_dealloc)
563563
if (so->weakreflist != NULL)
564564
PyObject_ClearWeakRefs((PyObject *) so);
565565

@@ -572,7 +572,7 @@ set_dealloc(PySetObject *so)
572572
if (so->table != so->smalltable)
573573
PyMem_DEL(so->table);
574574
Py_TYPE(so)->tp_free(so);
575-
Py_TRASHCAN_SAFE_END(so)
575+
Py_TRASHCAN_END
576576
}
577577

578578
static PyObject *

0 commit comments

Comments
 (0)