bpo-35983: skip trashcan for subclasses (GH-11841)
Add 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/Objects/descrobject.c b/Objects/descrobject.c
index 0fe9a44..8f1a823 100644
--- a/Objects/descrobject.c
+++ b/Objects/descrobject.c
@@ -1021,11 +1021,11 @@
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 PyObject *
diff --git a/Objects/dictobject.c b/Objects/dictobject.c
index c8c88d2..88ac1a9 100644
--- a/Objects/dictobject.c
+++ b/Objects/dictobject.c
@@ -1978,7 +1978,7 @@
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(mp);
- Py_TRASHCAN_SAFE_BEGIN(mp)
+ Py_TRASHCAN_BEGIN(mp, dict_dealloc)
if (values != NULL) {
if (values != empty_values) {
for (i = 0, n = mp->ma_keys->dk_nentries; i < n; i++) {
@@ -1996,7 +1996,7 @@
free_list[numfree++] = mp;
else
Py_TYPE(mp)->tp_free((PyObject *)mp);
- Py_TRASHCAN_SAFE_END(mp)
+ Py_TRASHCAN_END
}
diff --git a/Objects/listobject.c b/Objects/listobject.c
index c299542..08b3e89 100644
--- a/Objects/listobject.c
+++ b/Objects/listobject.c
@@ -361,7 +361,7 @@
{
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
@@ -377,7 +377,7 @@
free_list[numfree++] = op;
else
Py_TYPE(op)->tp_free((PyObject *)op);
- Py_TRASHCAN_SAFE_END(op)
+ Py_TRASHCAN_END
}
static PyObject *
diff --git a/Objects/odictobject.c b/Objects/odictobject.c
index 6c75a42..773827d 100644
--- a/Objects/odictobject.c
+++ b/Objects/odictobject.c
@@ -1356,28 +1356,17 @@
static void
odict_dealloc(PyODictObject *self)
{
- PyThreadState *tstate = _PyThreadState_GET();
-
PyObject_GC_UnTrack(self);
- Py_TRASHCAN_SAFE_BEGIN(self)
+ Py_TRASHCAN_BEGIN(self, odict_dealloc)
Py_XDECREF(self->od_inst_dict);
if (self->od_weakreflist != NULL)
PyObject_ClearWeakRefs((PyObject *)self);
_odict_clear_nodes(self);
-
- /* Call the base tp_dealloc(). Since it too uses the trashcan mechanism,
- * temporarily decrement trash_delete_nesting to prevent triggering it
- * and putting the partially deallocated object on the trashcan's
- * to-be-deleted-later list.
- */
- --tstate->trash_delete_nesting;
- assert(_tstate->trash_delete_nesting < PyTrash_UNWIND_LEVEL);
PyDict_Type.tp_dealloc((PyObject *)self);
- ++tstate->trash_delete_nesting;
- Py_TRASHCAN_SAFE_END(self)
+ Py_TRASHCAN_END
}
/* tp_repr */
diff --git a/Objects/setobject.c b/Objects/setobject.c
index a43ecd5..82e9639 100644
--- a/Objects/setobject.c
+++ b/Objects/setobject.c
@@ -559,7 +559,7 @@
/* 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);
@@ -572,7 +572,7 @@
if (so->table != so->smalltable)
PyMem_DEL(so->table);
Py_TYPE(so)->tp_free(so);
- Py_TRASHCAN_SAFE_END(so)
+ Py_TRASHCAN_END
}
static PyObject *
diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c
index 75d2bf9..9f0fc1c 100644
--- a/Objects/tupleobject.c
+++ b/Objects/tupleobject.c
@@ -240,7 +240,7 @@
Py_ssize_t i;
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)
@@ -259,7 +259,7 @@
}
Py_TYPE(op)->tp_free((PyObject *)op);
done:
- Py_TRASHCAN_SAFE_END(op)
+ Py_TRASHCAN_END
}
static PyObject *
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index b28f494..bfbd320 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -1120,7 +1120,6 @@
{
PyTypeObject *type, *base;
destructor basedealloc;
- PyThreadState *tstate = _PyThreadState_GET();
int has_finalizer;
/* Extract the type; we expect it to be a heap type */
@@ -1174,11 +1173,7 @@
/* UnTrack and re-Track around the trashcan macro, alas */
/* See explanation at end of function for full disclosure */
PyObject_GC_UnTrack(self);
- ++_PyRuntime.gc.trash_delete_nesting;
- ++ tstate->trash_delete_nesting;
- Py_TRASHCAN_SAFE_BEGIN(self);
- --_PyRuntime.gc.trash_delete_nesting;
- -- tstate->trash_delete_nesting;
+ Py_TRASHCAN_BEGIN(self, subtype_dealloc);
/* Find the nearest base with a different tp_dealloc */
base = type;
@@ -1271,11 +1266,7 @@
Py_DECREF(type);
endlabel:
- ++_PyRuntime.gc.trash_delete_nesting;
- ++ tstate->trash_delete_nesting;
- Py_TRASHCAN_SAFE_END(self);
- --_PyRuntime.gc.trash_delete_nesting;
- -- tstate->trash_delete_nesting;
+ Py_TRASHCAN_END
/* Explanation of the weirdness around the trashcan macros:
@@ -1312,67 +1303,6 @@
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
- _PyRuntime.trash_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.
*/
}