Skip to content

Commit ff023ed

Browse files
eduardo-elizondoencukou
eduardo-elizondo
authored and
encukou
committed
bpo-37879: Suppress subtype_dealloc decref when base type is a C heap type (GH-15323)
The instance destructor for a type is responsible for preparing an instance for deallocation by decrementing the reference counts of its referents. If an instance belongs to a heap type, the type object of an instance has its reference count decremented while for static types, which are permanently allocated, the type object is unaffected by the instance destructor. Previously, the default instance destructor searched the class hierarchy for an inherited instance destructor and, if present, would invoke it. Then, if the instance type is a heap type, it would decrement the reference count of that heap type. However, this could result in the premature destruction of a type because the inherited instance destructor should have already decremented the reference count of the type object. This change avoids the premature destruction of the type object by suppressing the decrement of its reference count when an inherited, non-default instance destructor has been invoked. Finally, an assertion on the Py_SIZE of a type was deleted. Heap types have a non zero size, making this into an incorrect assertion. #15323
1 parent 7264e92 commit ff023ed

File tree

4 files changed

+318
-10
lines changed

4 files changed

+318
-10
lines changed

‎Lib/test/test_capi.py

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,92 @@ def __del__(self):
383383
del L
384384
self.assertEqual(PyList.num, 0)
385385

386+
def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self):
387+
class HeapGcCTypeSubclass(_testcapi.HeapGcCType):
388+
def __init__(self):
389+
self.value2 = 20
390+
super().__init__()
391+
392+
subclass_instance = HeapGcCTypeSubclass()
393+
type_refcnt = sys.getrefcount(HeapGcCTypeSubclass)
394+
395+
# Test that subclass instance was fully created
396+
self.assertEqual(subclass_instance.value, 10)
397+
self.assertEqual(subclass_instance.value2, 20)
398+
399+
# Test that the type reference count is only decremented once
400+
del subclass_instance
401+
self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass))
402+
403+
def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self):
404+
class A(_testcapi.HeapGcCType):
405+
def __init__(self):
406+
self.value2 = 20
407+
super().__init__()
408+
409+
class B(A):
410+
def __init__(self):
411+
super().__init__()
412+
413+
def __del__(self):
414+
self.__class__ = A
415+
A.refcnt_in_del = sys.getrefcount(A)
416+
B.refcnt_in_del = sys.getrefcount(B)
417+
418+
subclass_instance = B()
419+
type_refcnt = sys.getrefcount(B)
420+
new_type_refcnt = sys.getrefcount(A)
421+
422+
# Test that subclass instance was fully created
423+
self.assertEqual(subclass_instance.value, 10)
424+
self.assertEqual(subclass_instance.value2, 20)
425+
426+
del subclass_instance
427+
428+
# Test that setting __class__ modified the reference counts of the types
429+
self.assertEqual(type_refcnt - 1, B.refcnt_in_del)
430+
self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del)
431+
432+
# Test that the original type already has decreased its refcnt
433+
self.assertEqual(type_refcnt - 1, sys.getrefcount(B))
434+
435+
# Test that subtype_dealloc decref the newly assigned __class__ only once
436+
self.assertEqual(new_type_refcnt, sys.getrefcount(A))
437+
438+
def test_c_subclass_of_heap_ctype_with_tpdealloc_decrefs_once(self):
439+
subclass_instance = _testcapi.HeapCTypeSubclass()
440+
type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass)
441+
442+
# Test that subclass instance was fully created
443+
self.assertEqual(subclass_instance.value, 10)
444+
self.assertEqual(subclass_instance.value2, 20)
445+
446+
# Test that the type reference count is only decremented once
447+
del subclass_instance
448+
self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclass))
449+
450+
def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once(self):
451+
subclass_instance = _testcapi.HeapCTypeSubclassWithFinalizer()
452+
type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer)
453+
new_type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass)
454+
455+
# Test that subclass instance was fully created
456+
self.assertEqual(subclass_instance.value, 10)
457+
self.assertEqual(subclass_instance.value2, 20)
458+
459+
# The tp_finalize slot will set __class__ to HeapCTypeSubclass
460+
del subclass_instance
461+
462+
# Test that setting __class__ modified the reference counts of the types
463+
self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del)
464+
self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del)
465+
466+
# Test that the original type already has decreased its refcnt
467+
self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer))
468+
469+
# Test that subtype_dealloc decref the newly assigned __class__ only once
470+
self.assertEqual(new_type_refcnt, sys.getrefcount(_testcapi.HeapCTypeSubclass))
471+
386472

387473
class TestPendingCalls(unittest.TestCase):
388474

‎Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Fix subtype_dealloc to suppress the type decref when the base type is a C
2+
heap type

‎Modules/_testcapimodule.c

Lines changed: 220 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
# error "_testcapi must test the public Python C API, not CPython internal C API"
3333
#endif
3434

35+
static struct PyModuleDef _testcapimodule;
36+
3537
static PyObject *TestError; /* set to exception object in init */
3638

3739
/* Raise TestError with test_name + ": " + msg, and return NULL. */
@@ -6169,6 +6171,189 @@ static PyTypeObject MethodDescriptor2_Type = {
61696171
.tp_flags = Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | _Py_TPFLAGS_HAVE_VECTORCALL,
61706172
};
61716173

6174+
PyDoc_STRVAR(heapgctype__doc__,
6175+
"A heap type with GC, and with overridden dealloc.\n\n"
6176+
"The 'value' attribute is set to 10 in __init__.");
6177+
6178+
typedef struct {
6179+
PyObject_HEAD
6180+
int value;
6181+
} HeapCTypeObject;
6182+
6183+
static struct PyMemberDef heapctype_members[] = {
6184+
{"value", T_INT, offsetof(HeapCTypeObject, value)},
6185+
{NULL} /* Sentinel */
6186+
};
6187+
6188+
static int
6189+
heapctype_init(PyObject *self, PyObject *args, PyObject *kwargs)
6190+
{
6191+
((HeapCTypeObject *)self)->value = 10;
6192+
return 0;
6193+
}
6194+
6195+
static void
6196+
heapgcctype_dealloc(HeapCTypeObject *self)
6197+
{
6198+
PyTypeObject *tp = Py_TYPE(self);
6199+
PyObject_GC_UnTrack(self);
6200+
PyObject_GC_Del(self);
6201+
Py_DECREF(tp);
6202+
}
6203+
6204+
static PyType_Slot HeapGcCType_slots[] = {
6205+
{Py_tp_init, heapctype_init},
6206+
{Py_tp_members, heapctype_members},
6207+
{Py_tp_dealloc, heapgcctype_dealloc},
6208+
{Py_tp_doc, heapgctype__doc__},
6209+
{0, 0},
6210+
};
6211+
6212+
static PyType_Spec HeapGcCType_spec = {
6213+
"_testcapi.HeapGcCType",
6214+
sizeof(HeapCTypeObject),
6215+
0,
6216+
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC,
6217+
HeapGcCType_slots
6218+
};
6219+
6220+
PyDoc_STRVAR(heapctype__doc__,
6221+
"A heap type without GC, but with overridden dealloc.\n\n"
6222+
"The 'value' attribute is set to 10 in __init__.");
6223+
6224+
static void
6225+
heapctype_dealloc(HeapCTypeObject *self)
6226+
{
6227+
PyTypeObject *tp = Py_TYPE(self);
6228+
PyObject_Del(self);
6229+
Py_DECREF(tp);
6230+
}
6231+
6232+
static PyType_Slot HeapCType_slots[] = {
6233+
{Py_tp_init, heapctype_init},
6234+
{Py_tp_members, heapctype_members},
6235+
{Py_tp_dealloc, heapctype_dealloc},
6236+
{Py_tp_doc, heapctype__doc__},
6237+
{0, 0},
6238+
};
6239+
6240+
static PyType_Spec HeapCType_spec = {
6241+
"_testcapi.HeapCType",
6242+
sizeof(HeapCTypeObject),
6243+
0,
6244+
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
6245+
HeapCType_slots
6246+
};
6247+
6248+
PyDoc_STRVAR(heapctypesubclass__doc__,
6249+
"Subclass of HeapCType, without GC.\n\n"
6250+
"__init__ sets the 'value' attribute to 10 and 'value2' to 20.");
6251+
6252+
typedef struct {
6253+
HeapCTypeObject base;
6254+
int value2;
6255+
} HeapCTypeSubclassObject;
6256+
6257+
static int
6258+
heapctypesubclass_init(PyObject *self, PyObject *args, PyObject *kwargs)
6259+
{
6260+
/* Call __init__ of the superclass */
6261+
if (heapctype_init(self, args, kwargs) < 0) {
6262+
return -1;
6263+
}
6264+
/* Initialize additional element */
6265+
((HeapCTypeSubclassObject *)self)->value2 = 20;
6266+
return 0;
6267+
}
6268+
6269+
static struct PyMemberDef heapctypesubclass_members[] = {
6270+
{"value2", T_INT, offsetof(HeapCTypeSubclassObject, value2)},
6271+
{NULL} /* Sentinel */
6272+
};
6273+
6274+
static PyType_Slot HeapCTypeSubclass_slots[] = {
6275+
{Py_tp_init, heapctypesubclass_init},
6276+
{Py_tp_members, heapctypesubclass_members},
6277+
{Py_tp_doc, heapctypesubclass__doc__},
6278+
{0, 0},
6279+
};
6280+
6281+
static PyType_Spec HeapCTypeSubclass_spec = {
6282+
"_testcapi.HeapCTypeSubclass",
6283+
sizeof(HeapCTypeSubclassObject),
6284+
0,
6285+
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
6286+
HeapCTypeSubclass_slots
6287+
};
6288+
6289+
PyDoc_STRVAR(heapctypesubclasswithfinalizer__doc__,
6290+
"Subclass of HeapCType with a finalizer that reassigns __class__.\n\n"
6291+
"__class__ is set to plain HeapCTypeSubclass during finalization.\n"
6292+
"__init__ sets the 'value' attribute to 10 and 'value2' to 20.");
6293+
6294+
static int
6295+
heapctypesubclasswithfinalizer_init(PyObject *self, PyObject *args, PyObject *kwargs)
6296+
{
6297+
PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(Py_TYPE(self), Py_tp_base);
6298+
initproc base_init = PyType_GetSlot(base, Py_tp_init);
6299+
base_init(self, args, kwargs);
6300+
return 0;
6301+
}
6302+
6303+
static void
6304+
heapctypesubclasswithfinalizer_finalize(PyObject *self)
6305+
{
6306+
PyObject *error_type, *error_value, *error_traceback, *m, *oldtype, *newtype;
6307+
6308+
/* Save the current exception, if any. */
6309+
PyErr_Fetch(&error_type, &error_value, &error_traceback);
6310+
6311+
m = PyState_FindModule(&_testcapimodule);
6312+
if (m == NULL) {
6313+
goto cleanup_finalize;
6314+
}
6315+
oldtype = PyObject_GetAttrString(m, "HeapCTypeSubclassWithFinalizer");
6316+
newtype = PyObject_GetAttrString(m, "HeapCTypeSubclass");
6317+
if (oldtype == NULL || newtype == NULL) {
6318+
goto cleanup_finalize;
6319+
}
6320+
6321+
if (PyObject_SetAttrString(self, "__class__", newtype) < 0) {
6322+
goto cleanup_finalize;
6323+
}
6324+
if (PyObject_SetAttrString(
6325+
oldtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(oldtype))) < 0) {
6326+
goto cleanup_finalize;
6327+
}
6328+
if (PyObject_SetAttrString(
6329+
newtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(newtype))) < 0) {
6330+
goto cleanup_finalize;
6331+
}
6332+
6333+
cleanup_finalize:
6334+
Py_XDECREF(oldtype);
6335+
Py_XDECREF(newtype);
6336+
6337+
/* Restore the saved exception. */
6338+
PyErr_Restore(error_type, error_value, error_traceback);
6339+
}
6340+
6341+
static PyType_Slot HeapCTypeSubclassWithFinalizer_slots[] = {
6342+
{Py_tp_init, heapctypesubclasswithfinalizer_init},
6343+
{Py_tp_members, heapctypesubclass_members},
6344+
{Py_tp_finalize, heapctypesubclasswithfinalizer_finalize},
6345+
{Py_tp_doc, heapctypesubclasswithfinalizer__doc__},
6346+
{0, 0},
6347+
};
6348+
6349+
static PyType_Spec HeapCTypeSubclassWithFinalizer_spec = {
6350+
"_testcapi.HeapCTypeSubclassWithFinalizer",
6351+
sizeof(HeapCTypeSubclassObject),
6352+
0,
6353+
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_FINALIZE,
6354+
HeapCTypeSubclassWithFinalizer_slots
6355+
};
6356+
61726357
static PyMethodDef meth_instance_methods[] = {
61736358
{"meth_varargs", meth_varargs, METH_VARARGS},
61746359
{"meth_varargs_keywords", (PyCFunction)(void(*)(void))meth_varargs_keywords, METH_VARARGS|METH_KEYWORDS},
@@ -6380,5 +6565,40 @@ PyInit__testcapi(void)
63806565
TestError = PyErr_NewException("_testcapi.error", NULL, NULL);
63816566
Py_INCREF(TestError);
63826567
PyModule_AddObject(m, "error", TestError);
6568+
6569+
PyObject *HeapGcCType = PyType_FromSpec(&HeapGcCType_spec);
6570+
if (HeapGcCType == NULL) {
6571+
return NULL;
6572+
}
6573+
PyModule_AddObject(m, "HeapGcCType", HeapGcCType);
6574+
6575+
PyObject *HeapCType = PyType_FromSpec(&HeapCType_spec);
6576+
if (HeapCType == NULL) {
6577+
return NULL;
6578+
}
6579+
PyObject *subclass_bases = PyTuple_Pack(1, HeapCType);
6580+
if (subclass_bases == NULL) {
6581+
return NULL;
6582+
}
6583+
PyObject *HeapCTypeSubclass = PyType_FromSpecWithBases(&HeapCTypeSubclass_spec, subclass_bases);
6584+
if (HeapCTypeSubclass == NULL) {
6585+
return NULL;
6586+
}
6587+
Py_DECREF(subclass_bases);
6588+
PyModule_AddObject(m, "HeapCTypeSubclass", HeapCTypeSubclass);
6589+
6590+
PyObject *subclass_with_finalizer_bases = PyTuple_Pack(1, HeapCTypeSubclass);
6591+
if (subclass_with_finalizer_bases == NULL) {
6592+
return NULL;
6593+
}
6594+
PyObject *HeapCTypeSubclassWithFinalizer = PyType_FromSpecWithBases(
6595+
&HeapCTypeSubclassWithFinalizer_spec, subclass_with_finalizer_bases);
6596+
if (HeapCTypeSubclassWithFinalizer == NULL) {
6597+
return NULL;
6598+
}
6599+
Py_DECREF(subclass_with_finalizer_bases);
6600+
PyModule_AddObject(m, "HeapCTypeSubclassWithFinalizer", HeapCTypeSubclassWithFinalizer);
6601+
6602+
PyState_AddModule(m, &_testcapimodule);
63836603
return m;
63846604
}

‎Objects/typeobject.c

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,11 +1157,9 @@ subtype_dealloc(PyObject *self)
11571157
/* Test whether the type has GC exactly once */
11581158

11591159
if (!PyType_IS_GC(type)) {
1160-
/* It's really rare to find a dynamic type that doesn't have
1161-
GC; it can only happen when deriving from 'object' and not
1162-
adding any slots or instance variables. This allows
1163-
certain simplifications: there's no need to call
1164-
clear_slots(), or DECREF the dict, or clear weakrefs. */
1160+
/* A non GC dynamic type allows certain simplifications:
1161+
there's no need to call clear_slots(), or DECREF the dict,
1162+
or clear weakrefs. */
11651163

11661164
/* Maybe call finalizer; exit early if resurrected */
11671165
if (type->tp_finalize) {
@@ -1177,7 +1175,6 @@ subtype_dealloc(PyObject *self)
11771175
/* Find the nearest base with a different tp_dealloc */
11781176
base = type;
11791177
while ((basedealloc = base->tp_dealloc) == subtype_dealloc) {
1180-
assert(Py_SIZE(base) == 0);
11811178
base = base->tp_base;
11821179
assert(base);
11831180
}
@@ -1189,8 +1186,10 @@ subtype_dealloc(PyObject *self)
11891186
assert(basedealloc);
11901187
basedealloc(self);
11911188

1192-
/* Can't reference self beyond this point */
1193-
Py_DECREF(type);
1189+
/* Only decref if the base type is not already a heap allocated type.
1190+
Otherwise, basedealloc should have decref'd it already */
1191+
if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE))
1192+
Py_DECREF(type);
11941193

11951194
/* Done */
11961195
return;
@@ -1289,8 +1288,9 @@ subtype_dealloc(PyObject *self)
12891288

12901289
/* Can't reference self beyond this point. It's possible tp_del switched
12911290
our type from a HEAPTYPE to a non-HEAPTYPE, so be careful about
1292-
reference counting. */
1293-
if (type->tp_flags & Py_TPFLAGS_HEAPTYPE)
1291+
reference counting. Only decref if the base type is not already a heap
1292+
allocated type. Otherwise, basedealloc should have decref'd it already */
1293+
if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE))
12941294
Py_DECREF(type);
12951295

12961296
endlabel:

0 commit comments

Comments
 (0)