From 56d86df5bb20b83a86f1d326acd05674736f3084 Mon Sep 17 00:00:00 2001 From: wmayer Date: Sat, 11 Dec 2021 14:26:40 +0100 Subject: [PATCH] Base: fix memory leak when creating object with factory method --- src/Base/BindingManager.cpp | 99 +++++++++++++++++++++++++++++++++++++ src/Base/BindingManager.h | 57 +++++++++++++++++++++ src/Base/CMakeLists.txt | 2 + src/Base/TypePyImp.cpp | 52 +++++++++++++++++-- 4 files changed, 205 insertions(+), 5 deletions(-) create mode 100644 src/Base/BindingManager.cpp create mode 100644 src/Base/BindingManager.h diff --git a/src/Base/BindingManager.cpp b/src/Base/BindingManager.cpp new file mode 100644 index 0000000000..fa7cd4ed9c --- /dev/null +++ b/src/Base/BindingManager.cpp @@ -0,0 +1,99 @@ +/*************************************************************************** + * Copyright (c) 2021 Werner Mayer * + * * + * This file is part of the FreeCAD CAx development system. * + * * + * This library is free software; you can redistribute it and/or * + * modify it under the terms of the GNU Library General Public * + * License as published by the Free Software Foundation; either * + * version 2 of the License, or (at your option) any later version. * + * * + * This library is distributed in the hope that it will be useful, * + * but WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * + * GNU Library General Public License for more details. * + * * + * You should have received a copy of the GNU Library General Public * + * License along with this library; see the file COPYING.LIB. If not, * + * write to the Free Software Foundation, Inc., 59 Temple Place, * + * Suite 330, Boston, MA 02111-1307, USA * + * * + ***************************************************************************/ + + +#include "PreCompiled.h" +#ifndef _PreComp_ +#include +#endif + +#include "BindingManager.h" + +using namespace Base; + +struct BindingManager::BindingManagerPrivate { + std::unordered_map wrapperMapper; + + bool hasWrapper(const void *cptr) + { + auto it = wrapperMapper.find(cptr); + return it != wrapperMapper.end(); + } + + void registerWrapper(const void* cptr, PyObject* pyObj) + { + wrapperMapper[cptr] = pyObj; + } + + void releaseWrapper(const void* cptr, PyObject* pyObj) + { + auto it = wrapperMapper.find(cptr); + if (it != wrapperMapper.end() && (!pyObj || it->second == pyObj)) { + wrapperMapper.erase(it); + } + } + + PyObject* retrieveWrapper(const void* cptr) + { + auto it = wrapperMapper.find(cptr); + if (it != wrapperMapper.end()) { + return it->second; + } + + return nullptr; + } +}; + +BindingManager& BindingManager::instance() +{ + static BindingManager singleton; + return singleton; +} + +BindingManager::BindingManager() + : p(new BindingManagerPrivate) +{ +} + +BindingManager::~BindingManager() +{ +} + +bool BindingManager::hasWrapper(const void *cptr) +{ + return p->hasWrapper(cptr); +} + +void BindingManager::registerWrapper(const void* cptr, PyObject* pyObj) +{ + p->registerWrapper(cptr, pyObj); +} + +void BindingManager::releaseWrapper(const void* cptr, PyObject* pyObj) +{ + p->releaseWrapper(cptr, pyObj); +} + +PyObject* BindingManager::retrieveWrapper(const void* cptr) +{ + return p->retrieveWrapper(cptr); +} diff --git a/src/Base/BindingManager.h b/src/Base/BindingManager.h new file mode 100644 index 0000000000..361bb6e1fe --- /dev/null +++ b/src/Base/BindingManager.h @@ -0,0 +1,57 @@ +/*************************************************************************** + * Copyright (c) 2021 Werner Mayer * + * * + * This file is part of the FreeCAD CAx development system. * + * * + * This library is free software; you can redistribute it and/or * + * modify it under the terms of the GNU Library General Public * + * License as published by the Free Software Foundation; either * + * version 2 of the License, or (at your option) any later version. * + * * + * This library is distributed in the hope that it will be useful, * + * but WITHOUT ANY WARRANTY; without even the implied warranty of * + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * + * GNU Library General Public License for more details. * + * * + * You should have received a copy of the GNU Library General Public * + * License along with this library; see the file COPYING.LIB. If not, * + * write to the Free Software Foundation, Inc., 59 Temple Place, * + * Suite 330, Boston, MA 02111-1307, USA * + * * + ***************************************************************************/ + + +#ifndef BASE_BINDINGMANAGER_H +#define BASE_BINDINGMANAGER_H + +#include +#include + +typedef struct _object PyObject; + +namespace Base +{ + +class BaseExport BindingManager +{ +public: + static BindingManager& instance(); + + bool hasWrapper(const void *cptr); + + void registerWrapper(const void* cptr, PyObject* pyObj); + void releaseWrapper(const void* cptr, PyObject* pyObj); + + PyObject* retrieveWrapper(const void* cptr); + +private: + BindingManager(); + ~BindingManager(); + + struct BindingManagerPrivate; + std::unique_ptr p; +}; + +} + +#endif // BASE_BINDINGMANAGER_H diff --git a/src/Base/CMakeLists.txt b/src/Base/CMakeLists.txt index 7412ea22e6..e3933d0541 100644 --- a/src/Base/CMakeLists.txt +++ b/src/Base/CMakeLists.txt @@ -218,6 +218,7 @@ SET(FreeCADBase_CPP_SRCS Base64.cpp BaseClass.cpp BaseClassPyImp.cpp + BindingManager.cpp BoundBoxPyImp.cpp Builder3D.cpp Console.cpp @@ -278,6 +279,7 @@ SET(FreeCADBase_HPP_SRCS Axis.h Base64.h BaseClass.h + BindingManager.h BoundBox.h Builder3D.h Console.h diff --git a/src/Base/TypePyImp.cpp b/src/Base/TypePyImp.cpp index cf3d5fb975..f4ec19b70c 100644 --- a/src/Base/TypePyImp.cpp +++ b/src/Base/TypePyImp.cpp @@ -24,6 +24,8 @@ #include "PreCompiled.h" #include "Type.h" +#include "BaseClassPy.h" +#include "BindingManager.h" #include "TypePy.h" #include "TypePy.cpp" @@ -167,6 +169,48 @@ PyObject* TypePy::getAllDerived(PyObject *args) return Py::new_reference_to(res); } +namespace { +static void deallocPyObject(PyObject* py) +{ + Base::PyObjectBase* pybase = static_cast(py); + Base::BaseClass* base = static_cast(pybase->getTwinPointer()); + if (Base::BindingManager::instance().retrieveWrapper(base) == py) { + Base::BindingManager::instance().releaseWrapper(base, py); + delete base; + } + + Base::PyObjectBase::PyDestructor(py); +} + +static PyObject* createPyObject(Base::BaseClass* base) +{ + PyObject* py = base->getPyObject(); + + if (PyObject_TypeCheck(py, &Base::PyObjectBase::Type)) { + // if the Python wrapper is a sub-class of PyObjectBase then + // check if the C++ object must be added to the list of tracked objects + Base::PyObjectBase* pybase = static_cast(py); + if (base == pybase->getTwinPointer()) { + // steal a reference because at this point the counter is at 2 + Py_DECREF(py); + Py_TYPE(py)->tp_dealloc = deallocPyObject; + Base::BindingManager::instance().registerWrapper(base, py); + } + else { + // The Python wrapper creates its own copy of the C++ object + delete base; + } + } + else { + // if the Python wrapper is not a sub-class of PyObjectBase then + // immediately destroy the C++ object + delete base; + } + return py; +} + +} + PyObject* TypePy::createInstance (PyObject *args) { if (!PyArg_ParseTuple(args, "")) @@ -177,8 +221,7 @@ PyObject* TypePy::createInstance (PyObject *args) Py_Return; } - //TODO: At the moment "base" will never be destroyed and causes a memory leak - return base->getPyObject(); + return createPyObject(base); } PyObject* TypePy::createInstanceByName (PyObject *args) @@ -194,8 +237,7 @@ PyObject* TypePy::createInstanceByName (PyObject *args) Py_Return; } - //TODO: At the moment "base" will never be destroyed and causes a memory leak - return base->getPyObject(); + return createPyObject(base); } Py::String TypePy::getName(void) const @@ -223,7 +265,7 @@ Py::String TypePy::getModule(void) const PyObject *TypePy::getCustomAttributes(const char* /*attr*/) const { - return 0; + return nullptr; } int TypePy::setCustomAttributes(const char* /*attr*/, PyObject* /*obj*/)