From 8210e99b0cba4e88f582f4324102c9296512552f 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 200d9eeb8435995934088a4ec860b803e2c9b9e8 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 5f46ee4f14a62fc1cee35fd527f7729035e3d229 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 a644f75d53bb0a1c0d02e99b07803c1cf3510c82 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()