From defc6cd9068296e3aad6eb0eb6d06ee3b8e1a6f2 Mon Sep 17 00:00:00 2001 From: wmayer Date: Thu, 25 Apr 2024 14:34:02 +0200 Subject: [PATCH 1/4] Base: Fix memory leak in Quantity::parse If an exception is thrown then the allocated buffer won't be cleaned up. To make this exception-safe the class StringBufferCleaner is added using the RAII idiom --- src/Base/Quantity.cpp | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/Base/Quantity.cpp b/src/Base/Quantity.cpp index 7de1e7c49b..fe4c0e1e95 100644 --- a/src/Base/Quantity.cpp +++ b/src/Base/Quantity.cpp @@ -532,6 +532,28 @@ int QuantityLexer(); // NOLINTNEXTLINE #include "QuantityLexer.c" #endif // DOXYGEN_SHOULD_SKIP_THIS + +class StringBufferCleaner +{ +public: + explicit StringBufferCleaner(YY_BUFFER_STATE buffer) + : my_string_buffer {buffer} + {} + ~StringBufferCleaner() + { + // free the scan buffer + yy_delete_buffer(my_string_buffer); + } + + StringBufferCleaner(const StringBufferCleaner&) = delete; + StringBufferCleaner(StringBufferCleaner&&) = delete; + StringBufferCleaner& operator=(const StringBufferCleaner&) = delete; + StringBufferCleaner& operator=(StringBufferCleaner&&) = delete; + +private: + YY_BUFFER_STATE my_string_buffer; +}; + } // namespace QuantityParser #if defined(__clang__) @@ -545,12 +567,11 @@ Quantity Quantity::parse(const QString& string) // parse from buffer QuantityParser::YY_BUFFER_STATE my_string_buffer = QuantityParser::yy_scan_string(string.toUtf8().data()); + QuantityParser::StringBufferCleaner cleaner(my_string_buffer); // set the global return variables QuantResult = Quantity(DOUBLE_MIN); // run the parser QuantityParser::yyparse(); - // free the scan buffer - QuantityParser::yy_delete_buffer(my_string_buffer); return QuantResult; } From 29017e869a69aef77287ee327445beefc9ab2b80 Mon Sep 17 00:00:00 2001 From: wmayer Date: Thu, 25 Apr 2024 14:36:57 +0200 Subject: [PATCH 2/4] App: Add class CleanupProcess to allow to free resources when closing the application --- src/App/Application.cpp | 3 ++ src/App/CMakeLists.txt | 2 ++ src/App/CleanupProcess.cpp | 47 +++++++++++++++++++++++++++++++ src/App/CleanupProcess.h | 57 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+) create mode 100644 src/App/CleanupProcess.cpp create mode 100644 src/App/CleanupProcess.h diff --git a/src/App/Application.cpp b/src/App/Application.cpp index e092f1d5c4..dd34602d9b 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -84,6 +84,7 @@ #include "Annotation.h" #include "Application.h" +#include "CleanupProcess.h" #include "ComplexGeoData.h" #include "DocumentObjectFileIncluded.h" #include "DocumentObjectGroup.h" @@ -1699,6 +1700,8 @@ void Application::destruct() cleanupUnits(); #endif + CleanupProcess::callCleanup(); + // not initialized or double destruct! assert(_pcSingleton); delete _pcSingleton; diff --git a/src/App/CMakeLists.txt b/src/App/CMakeLists.txt index c809a0aeec..044825fdac 100644 --- a/src/App/CMakeLists.txt +++ b/src/App/CMakeLists.txt @@ -271,6 +271,7 @@ SET(FreeCADApp_CPP_SRCS ApplicationPy.cpp AutoTransaction.cpp Branding.cpp + CleanupProcess.cpp Color.cpp ColorModel.cpp ComplexGeoData.cpp @@ -296,6 +297,7 @@ SET(FreeCADApp_HPP_SRCS Application.h AutoTransaction.h Branding.h + CleanupProcess.h Color.h ColorModel.h ComplexGeoData.h diff --git a/src/App/CleanupProcess.cpp b/src/App/CleanupProcess.cpp new file mode 100644 index 0000000000..30fbd35ae2 --- /dev/null +++ b/src/App/CleanupProcess.cpp @@ -0,0 +1,47 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later + +/*************************************************************************** + * Copyright (c) 2024 Werner Mayer * + * * + * This file is part of FreeCAD. * + * * + * FreeCAD is free software: you can redistribute it and/or modify it * + * under the terms of the GNU Lesser General Public License as * + * published by the Free Software Foundation, either version 2.1 of the * + * License, or (at your option) any later version. * + * * + * FreeCAD 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 * + * Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public * + * License along with FreeCAD. If not, see * + * . * + * * + **************************************************************************/ + +#include "PreCompiled.h" +#include + +#include "CleanupProcess.h" + + +using namespace App; + +namespace +{ + static std::list> cleanup_funcs; // NOLINT +} + +void CleanupProcess::registerCleanup(const std::function& func) +{ + cleanup_funcs.push_back(func); +} + +void CleanupProcess::callCleanup() +{ + for (const auto& func : cleanup_funcs) { + func(); + } +} diff --git a/src/App/CleanupProcess.h b/src/App/CleanupProcess.h new file mode 100644 index 0000000000..a5d9281b4f --- /dev/null +++ b/src/App/CleanupProcess.h @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: LGPL-2.1-or-later + +/*************************************************************************** + * Copyright (c) 2024 Werner Mayer * + * * + * This file is part of FreeCAD. * + * * + * FreeCAD is free software: you can redistribute it and/or modify it * + * under the terms of the GNU Lesser General Public License as * + * published by the Free Software Foundation, either version 2.1 of the * + * License, or (at your option) any later version. * + * * + * FreeCAD 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 * + * Lesser General Public License for more details. * + * * + * You should have received a copy of the GNU Lesser General Public * + * License along with FreeCAD. If not, see * + * . * + * * + **************************************************************************/ + +#ifndef APP_CLEANUPPROCESS_H +#define APP_CLEANUPPROCESS_H + +#include +#include + +namespace App +{ + +/*! + * \brief The CleanupProcess class + */ +class AppExport CleanupProcess +{ +public: + /*! + * \brief registerCleanup + * \param func + * This adds a callback function that will be called when the application + * is about to be shut down. + * @note A callback function is only about to free resources. Accessing + * stuff of the application like parameter groups should be avoided. + */ + static void registerCleanup(const std::function& func); + /*! + * \brief callCleanup + * Calls the functions that are registered with \a registerCleanup. + */ + static void callCleanup(); +}; + +} + +#endif // APP_CLEANUPPROCESS_H From 70950692eb595df3d06d2ef91820566a5041fb35 Mon Sep 17 00:00:00 2001 From: wmayer Date: Thu, 25 Apr 2024 14:50:59 +0200 Subject: [PATCH 3/4] Material: Fix two direct memory leaks in ModelLoader::addToTree and ModelLoader::loadLibrary --- src/Mod/Material/App/ModelLoader.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/Mod/Material/App/ModelLoader.cpp b/src/Mod/Material/App/ModelLoader.cpp index 595c3f0d85..ef816e6b9c 100644 --- a/src/Mod/Material/App/ModelLoader.cpp +++ b/src/Mod/Material/App/ModelLoader.cpp @@ -235,7 +235,7 @@ void ModelLoader::addToTree(std::shared_ptr model, Model::ModelType type = (base == "Model") ? Model::ModelType_Physical : Model::ModelType_Appearance; - Model* finalModel = new Model(library, type, name, directory, uuid, description, url, doi); + Model finalModel(library, type, name, directory, uuid, description, url, doi); // Add inheritance list if (yamlModel[base]["Inherits"]) { @@ -243,7 +243,7 @@ void ModelLoader::addToTree(std::shared_ptr model, for (auto it = inherits.begin(); it != inherits.end(); it++) { QString nodeName = QString::fromStdString((*it)["UUID"].as()); - finalModel->addInheritance(nodeName); + finalModel.addInheritance(nodeName); } } @@ -299,11 +299,11 @@ void ModelLoader::addToTree(std::shared_ptr model, property.setInheritance((*inheritances)[key]); } - finalModel->addProperty(property); + finalModel.addProperty(property); } } - (*_modelMap)[uuid] = library->addModel(*finalModel, directory); + (*_modelMap)[uuid] = library->addModel(finalModel, directory); } void ModelLoader::loadLibrary(std::shared_ptr library) @@ -330,16 +330,14 @@ void ModelLoader::loadLibrary(std::shared_ptr library) } } - std::map, QString>* inheritances = - new std::map, QString>(); + std::map, QString> inheritances; for (auto it = _modelEntryMap->begin(); it != _modelEntryMap->end(); it++) { - dereference(it->second, inheritances); + dereference(it->second, &inheritances); } for (auto it = _modelEntryMap->begin(); it != _modelEntryMap->end(); it++) { - addToTree(it->second, inheritances); + addToTree(it->second, &inheritances); } - // delete inheritances; } void ModelLoader::loadLibraries() From 451c55b41dc9352ee90a157b7549abb8d672215a Mon Sep 17 00:00:00 2001 From: wmayer Date: Thu, 25 Apr 2024 15:01:21 +0200 Subject: [PATCH 4/4] Material: Fix several memory leaks caused by cyclic references The class MaterialLibrary has a shared pointer to its Material objects and the class Material has a shared pointer to the MaterialLibrary. The class MaterialManager owns all MaterialLibrary and Material objects in static containers. To resolve the cyclic references the method cleanup() has been added. The class ModelLibrary has a shared pointer to its Model objects and the class Model has a shared pointer to the ModelLibrary. The class ModelManager owns all ModelLibrary and Model objects in static containers. To resolve the cyclic references the method cleanup() has been added. The Materials module registers a function to App::CleanupProcess that calls the above cleanup() methods. NOTE: This registration is only done in debug mode mainly to satisfy memory checkers --- src/Mod/Material/App/AppMaterial.cpp | 9 +++++++++ src/Mod/Material/App/MaterialManager.cpp | 15 +++++++++++++++ src/Mod/Material/App/MaterialManager.h | 1 + src/Mod/Material/App/ModelManager.cpp | 15 +++++++++++++++ src/Mod/Material/App/ModelManager.h | 1 + 5 files changed, 41 insertions(+) diff --git a/src/Mod/Material/App/AppMaterial.cpp b/src/Mod/Material/App/AppMaterial.cpp index 6b4b137c9c..99ec73f60f 100644 --- a/src/Mod/Material/App/AppMaterial.cpp +++ b/src/Mod/Material/App/AppMaterial.cpp @@ -27,7 +27,10 @@ #include #include +#include + #include "MaterialFilterPy.h" +#include "MaterialLoader.h" #include "MaterialManagerPy.h" #include "MaterialPy.h" #include "ModelManagerPy.h" @@ -61,6 +64,12 @@ PyObject* initModule() PyMOD_INIT_FUNC(Materials) { +#ifdef FC_DEBUG + App::CleanupProcess::registerCleanup([](){ + Materials::MaterialManager::cleanup(); + Materials::ModelManager::cleanup(); + }); +#endif PyObject* module = Materials::initModule(); Base::Console().Log("Loading Material module... done\n"); diff --git a/src/Mod/Material/App/MaterialManager.cpp b/src/Mod/Material/App/MaterialManager.cpp index afb0c9024b..74ccb970ec 100644 --- a/src/Mod/Material/App/MaterialManager.cpp +++ b/src/Mod/Material/App/MaterialManager.cpp @@ -77,6 +77,21 @@ void MaterialManager::initLibraries() } } +void MaterialManager::cleanup() +{ + if (_libraryList) { + _libraryList->clear(); + } + + if (_materialMap) { + for (auto& it : *_materialMap) { + // This is needed to resolve cyclic dependencies + it.second->setLibrary(nullptr); + } + _materialMap->clear(); + } +} + void MaterialManager::saveMaterial(const std::shared_ptr& library, const std::shared_ptr& material, const QString& path, diff --git a/src/Mod/Material/App/MaterialManager.h b/src/Mod/Material/App/MaterialManager.h index e8c1c5b13f..34e762789d 100644 --- a/src/Mod/Material/App/MaterialManager.h +++ b/src/Mod/Material/App/MaterialManager.h @@ -54,6 +54,7 @@ public: MaterialManager(); ~MaterialManager() override = default; + static void cleanup(); static std::shared_ptr defaultMaterial(); static QString defaultMaterialUUID(); diff --git a/src/Mod/Material/App/ModelManager.cpp b/src/Mod/Material/App/ModelManager.cpp index 23be586253..dad296fb36 100644 --- a/src/Mod/Material/App/ModelManager.cpp +++ b/src/Mod/Material/App/ModelManager.cpp @@ -72,6 +72,21 @@ bool ModelManager::isModel(const QString& file) return false; } +void ModelManager::cleanup() +{ + if (_libraryList) { + _libraryList->clear(); + } + + if (_modelMap) { + for (auto& it : *_modelMap) { + // This is needed to resolve cyclic dependencies + it.second->setLibrary(nullptr); + } + _modelMap->clear(); + } +} + void ModelManager::refresh() { _modelMap->clear(); diff --git a/src/Mod/Material/App/ModelManager.h b/src/Mod/Material/App/ModelManager.h index 9ded3b3c78..15b433930d 100644 --- a/src/Mod/Material/App/ModelManager.h +++ b/src/Mod/Material/App/ModelManager.h @@ -44,6 +44,7 @@ public: ModelManager(); ~ModelManager() override = default; + static void cleanup(); void refresh(); std::shared_ptr>> getModelLibraries()