From 6978325323208c395d135f19847a8ad0b13f93f9 Mon Sep 17 00:00:00 2001 From: Christian Tismer Date: Fri, 26 Oct 2018 16:58:09 +0200 Subject: Fix Memory Leak Caused By Wrong Limited API Default When a type has nullptr as tp_dealloc, there apply different defaults. Static types had object_dealloc as default, while new heaptypes created with type_new have subtype_dealloc as default. A problem was now that PyType_FromSpec also has subtype_dealloc as default. But that is wrong, because a type that was written with the static type approach is already written with object_dealloc in mind and takes somehow care about further issues with that type. When we now convert this type and suddenly use subtype_dealloc instead of object_dealloc, things get pretty wrong. Finding that out was pretty hard and took quite long to understand. The fix was then very easy and is the best proof: Replacing our former (wrong) solution of supplying an SbkDummyDealloc with a function object_dealloc works perfectly, and the leakage completely vanished. The documentation now is also corrected. Task-number: PYSIDE-832 Change-Id: Ifc20c28172eb5663cd5e60dac52e0a43acfb626c Reviewed-by: Friedemann Kleint --- sources/shiboken2/generator/shiboken2/cppgenerator.cpp | 2 +- sources/shiboken2/libshiboken/basewrapper.cpp | 6 ++++-- sources/shiboken2/libshiboken/basewrapper.h | 12 +++++------- sources/shiboken2/libshiboken/pep384impl_doc.rst | 13 +++++++++---- sources/shiboken2/libshiboken/sbkenum.cpp | 2 +- sources/shiboken2/libshiboken/voidptr.cpp | 2 +- 6 files changed, 21 insertions(+), 16 deletions(-) (limited to 'sources/shiboken2') diff --git a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp index b0d778231..9e1d6926e 100644 --- a/sources/shiboken2/generator/shiboken2/cppgenerator.cpp +++ b/sources/shiboken2/generator/shiboken2/cppgenerator.cpp @@ -3704,7 +3704,7 @@ void CppGenerator::writeClassDefinition(QTextStream &s, if (metaClass->isNamespace() || metaClass->hasPrivateDestructor()) { tp_dealloc = metaClass->hasPrivateDestructor() ? QLatin1String("SbkDeallocWrapperWithPrivateDtor") : - QLatin1String("SbkDummyDealloc /* PYSIDE-595: Prevent replacement of \"0\" with subtype_dealloc. */"); + QLatin1String("object_dealloc /* PYSIDE-832: Prevent replacement of \"0\" with subtype_dealloc. */"); tp_init = QLatin1String("0"); } else { QString deallocClassName; diff --git a/sources/shiboken2/libshiboken/basewrapper.cpp b/sources/shiboken2/libshiboken/basewrapper.cpp index 0aba26679..5ca4172c8 100644 --- a/sources/shiboken2/libshiboken/basewrapper.cpp +++ b/sources/shiboken2/libshiboken/basewrapper.cpp @@ -416,8 +416,10 @@ PyObject* SbkQAppTpNew(PyTypeObject* subtype, PyObject *, PyObject *) } void -SbkDummyDealloc(PyObject *) -{} +object_dealloc(PyObject *self) +{ + Py_TYPE(self)->tp_free(self); +} PyObject * SbkDummyNew(PyTypeObject *type, PyObject*, PyObject*) diff --git a/sources/shiboken2/libshiboken/basewrapper.h b/sources/shiboken2/libshiboken/basewrapper.h index 06b17a151..134a3bc51 100644 --- a/sources/shiboken2/libshiboken/basewrapper.h +++ b/sources/shiboken2/libshiboken/basewrapper.h @@ -109,17 +109,15 @@ LIBSHIBOKEN_API PyObject* SbkObjectTpNew(PyTypeObject* subtype, PyObject*, PyObj LIBSHIBOKEN_API PyObject* SbkQAppTpNew(PyTypeObject *subtype, PyObject *args, PyObject *kwds); /** - * PYSIDE-595: Use a null deallocator instead of nullptr. + * PYSIDE-832: Use object_dealloc instead of nullptr. * * When moving to heaptypes, we were struck by a special default behavior of * PyType_FromSpecWithBases that inserts subtype_dealloc when tp_dealloc is - * nullptr. To prevent inserting this, we use a null deallocator that is there - * as a placeholder. - * - * The same holds for a null tp_new. We use one that raises the right error. + * nullptr. But the default before conversion to heaptypes was to assign + * object_dealloc. This seems to be a bug in the Limited API. */ -LIBSHIBOKEN_API void SbkDummyDealloc(PyObject*); -LIBSHIBOKEN_API PyObject *SbkDummyNew(PyTypeObject *type, PyObject*, PyObject*); +LIBSHIBOKEN_API void object_dealloc(PyObject *); +LIBSHIBOKEN_API PyObject *SbkDummyNew(PyTypeObject *type, PyObject *, PyObject *); } // extern "C" diff --git a/sources/shiboken2/libshiboken/pep384impl_doc.rst b/sources/shiboken2/libshiboken/pep384impl_doc.rst index 9974f737b..2844249ad 100644 --- a/sources/shiboken2/libshiboken/pep384impl_doc.rst +++ b/sources/shiboken2/libshiboken/pep384impl_doc.rst @@ -426,11 +426,16 @@ many headaches:: type->tp_dealloc = subtype_dealloc; } -So, if you think you have no ``tp_dealloc`` field set, you will unwantedly -get ``subtype_dealloc``, which in the case of PySide always was wrong! +In fact, before the move to the new API, the ``PyType_Ready`` function +filled empty ``tp_dealloc`` fields with ``object_dealloc``. And the code +that has been written with that in mind now becomes pretty wrong if suddenly +``subtype_dealloc`` is used. + +The way out was to explicitly provide an ``object_dealloc`` function. +This would then again impose a problem, because ``object_dealloc`` is not +public. Writing our own version is easy, but it again needs access to +type objects. But fortunately, we have broken this rule, already... -The way out was to use a dummy function that has no effect other than -being something not NULL. * The new types are only partially allocated diff --git a/sources/shiboken2/libshiboken/sbkenum.cpp b/sources/shiboken2/libshiboken/sbkenum.cpp index bd007f079..d129e6380 100644 --- a/sources/shiboken2/libshiboken/sbkenum.cpp +++ b/sources/shiboken2/libshiboken/sbkenum.cpp @@ -507,7 +507,7 @@ static PyType_Slot SbkNewType_slots[] = { {Py_nb_index, (void *)enum_int}, {Py_tp_richcompare, (void *)enum_richcompare}, {Py_tp_hash, (void *)enum_hash}, - {Py_tp_dealloc, (void *)SbkDummyDealloc}, + {Py_tp_dealloc, (void *)object_dealloc}, {0, 0} }; static PyType_Spec SbkNewType_spec = { diff --git a/sources/shiboken2/libshiboken/voidptr.cpp b/sources/shiboken2/libshiboken/voidptr.cpp index 3a0dbb434..0d7b6b9cd 100644 --- a/sources/shiboken2/libshiboken/voidptr.cpp +++ b/sources/shiboken2/libshiboken/voidptr.cpp @@ -224,7 +224,7 @@ static PyType_Slot SbkVoidPtrType_slots[] = { {Py_tp_richcompare, (void *)SbkVoidPtrObject_richcmp}, {Py_tp_init, (void *)SbkVoidPtrObject_init}, {Py_tp_new, (void *)SbkVoidPtrObject_new}, - {Py_tp_dealloc, (void *)SbkDummyDealloc}, + {Py_tp_dealloc, (void *)object_dealloc}, {0, 0} }; static PyType_Spec SbkVoidPtrType_spec = { -- cgit v1.2.3