From 459b4d2c36be41eb79606a318a16f52575fab3e6 Mon Sep 17 00:00:00 2001 From: wmayer Date: Mon, 6 May 2024 11:57:23 +0200 Subject: [PATCH 1/2] Core: Fix possible race conditions when reading or writing config file --- src/App/Application.cpp | 24 ++++++++++++++------- src/Base/Parameter.cpp | 43 ++++++++++++++++++++++++++++++++------ src/Base/Parameter.h | 3 +++ src/Gui/StartupProcess.cpp | 14 +++++++++++++ src/Gui/StartupProcess.h | 1 + 5 files changed, 72 insertions(+), 13 deletions(-) diff --git a/src/App/Application.cpp b/src/App/Application.cpp index 0e86aa9bdb..6c30378fce 100644 --- a/src/App/Application.cpp +++ b/src/App/Application.cpp @@ -1674,19 +1674,29 @@ void Application::cleanupUnits() void Application::destruct() { // saving system parameter - Base::Console().Log("Saving system parameter...\n"); - _pcSysParamMngr->SaveDocument(); + if (_pcSysParamMngr->IgnoreSave()) { + Base::Console().Warning("Discard system parameter\n"); + } + else { + Base::Console().Log("Saving system parameter...\n"); + _pcSysParamMngr->SaveDocument(); + Base::Console().Log("Saving system parameter...done\n"); + } // saving the User parameter - Base::Console().Log("Saving system parameter...done\n"); - Base::Console().Log("Saving user parameter...\n"); - _pcUserParamMngr->SaveDocument(); - Base::Console().Log("Saving user parameter...done\n"); + if (_pcUserParamMngr->IgnoreSave()) { + Base::Console().Warning("Discard user parameter\n"); + } + else { + Base::Console().Log("Saving user parameter...\n"); + _pcUserParamMngr->SaveDocument(); + Base::Console().Log("Saving user parameter...done\n"); + } // now save all other parameter files auto& paramMgr = _pcSingleton->mpcPramManager; for (auto it : paramMgr) { if ((it.second != _pcSysParamMngr) && (it.second != _pcUserParamMngr)) { - if (it.second->HasSerializer()) { + if (it.second->HasSerializer() && !it.second->IgnoreSave()) { Base::Console().Log("Saving %s...\n", it.first.c_str()); it.second->SaveDocument(); Base::Console().Log("Saving %s...done\n", it.first.c_str()); diff --git a/src/Base/Parameter.cpp b/src/Base/Parameter.cpp index 69e4d32248..17c7d91eeb 100644 --- a/src/Base/Parameter.cpp +++ b/src/Base/Parameter.cpp @@ -1617,6 +1617,7 @@ ParameterManager::ParameterManager() // --------------------------------------------------------------------------- // NOLINTBEGIN + gIgnoreSave = false; gDoNamespaces = false; gDoSchema = false; gSchemaFullChecking = false; @@ -1723,14 +1724,34 @@ void ParameterManager::SaveDocument() const } } +void ParameterManager::SetIgnoreSave(bool value) +{ + gIgnoreSave = value; +} + +bool ParameterManager::IgnoreSave() const +{ + return gIgnoreSave; +} + namespace { -void waitForFileAccess(const Base::FileInfo& file) +QString getLockFile(const Base::FileInfo& file) { QFileInfo fi(QDir::tempPath(), QString::fromStdString(file.fileName() + ".lock")); - QLockFile lock(fi.absoluteFilePath()); - const int waitOneSecond = 1000; - lock.tryLock(waitOneSecond); + return fi.absoluteFilePath(); +} + +int getTimeout() +{ + const int timeout = 5000; + return timeout; +} + +bool waitForReadAccess(const Base::FileInfo& file) +{ + QLockFile lock(getLockFile(file)); + return lock.tryLock(getTimeout()); } } // namespace @@ -1753,7 +1774,13 @@ int ParameterManager::LoadDocument(const char* sFileName) { try { Base::FileInfo file(sFileName); - waitForFileAccess(file); + if (!waitForReadAccess(file)) { + // Continue with empty config + CreateDocument(); + SetIgnoreSave(true); + std::cerr << "Failed to access file for reading: " << sFileName << std::endl; + return 1; + } #if defined(FC_OS_WIN32) std::wstring name = file.toStdWString(); // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast) @@ -1845,7 +1872,11 @@ void ParameterManager::SaveDocument(const char* sFileName) const { try { Base::FileInfo file(sFileName); - waitForFileAccess(file); + QLockFile lock(getLockFile(file)); + if (!lock.tryLock(getTimeout())) { + std::cerr << "Failed to access file for writing: " << sFileName << std::endl; + return; + } // // Plug in a format target to receive the resultant // XML stream from the serializer. diff --git a/src/Base/Parameter.h b/src/Base/Parameter.h index a3bc1f3848..5f967ba692 100644 --- a/src/Base/Parameter.h +++ b/src/Base/Parameter.h @@ -434,12 +434,15 @@ public: bool LoadOrCreateDocument(); /// Saves an XML document by calling the serializer's save method. void SaveDocument() const; + void SetIgnoreSave(bool value); + bool IgnoreSave() const; //@} private: XERCES_CPP_NAMESPACE_QUALIFIER DOMDocument* _pDocument {nullptr}; ParameterSerializer* paramSerializer {nullptr}; + bool gIgnoreSave; bool gDoNamespaces; bool gDoSchema; bool gSchemaFullChecking; diff --git a/src/Gui/StartupProcess.cpp b/src/Gui/StartupProcess.cpp index 9c5e7e571b..a539af372d 100644 --- a/src/Gui/StartupProcess.cpp +++ b/src/Gui/StartupProcess.cpp @@ -43,6 +43,7 @@ #include "MainWindow.h" #include "Language/Translator.h" #include +#include using namespace Gui; @@ -233,6 +234,7 @@ void StartupPostProcess::execute() setBranding(); showMainWindow(); activateWorkbench(); + checkParameters(); } void StartupPostProcess::setWindowTitle() @@ -545,3 +547,15 @@ void StartupPostProcess::autoloadModules(const QStringList& wb) } } } + +void StartupPostProcess::checkParameters() +{ + if (App::GetApplication().GetSystemParameter().IgnoreSave()) { + Base::Console().Warning("System parameter file couldn't be opened.\n" + "Continue with an empty configuration that won't be saved.\n"); + } + if (App::GetApplication().GetUserParameter().IgnoreSave()) { + Base::Console().Warning("User parameter file couldn't be opened.\n" + "Continue with an empty configuration that won't be saved.\n"); + } +} diff --git a/src/Gui/StartupProcess.h b/src/Gui/StartupProcess.h index a6e169aeff..838ac7ec5c 100644 --- a/src/Gui/StartupProcess.h +++ b/src/Gui/StartupProcess.h @@ -74,6 +74,7 @@ private: bool hiddenMainWindow() const; void showMainWindow(); void activateWorkbench(); + void checkParameters(); private: bool loadFromPythonModule = false; From 1f3e7bc0daf45b1e71a947c90845edf09c8ea334 Mon Sep 17 00:00:00 2001 From: Pieter Hijma Date: Fri, 17 May 2024 11:29:49 +0200 Subject: [PATCH 2/2] Core: Maintain the lock while reading config file --- src/Base/Parameter.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/Base/Parameter.cpp b/src/Base/Parameter.cpp index 17c7d91eeb..66f5068cd9 100644 --- a/src/Base/Parameter.cpp +++ b/src/Base/Parameter.cpp @@ -1747,12 +1747,6 @@ int getTimeout() const int timeout = 5000; return timeout; } - -bool waitForReadAccess(const Base::FileInfo& file) -{ - QLockFile lock(getLockFile(file)); - return lock.tryLock(getTimeout()); -} } // namespace //************************************************************************** @@ -1774,7 +1768,8 @@ int ParameterManager::LoadDocument(const char* sFileName) { try { Base::FileInfo file(sFileName); - if (!waitForReadAccess(file)) { + QLockFile lock(getLockFile(file)); + if (!lock.tryLock(getTimeout())) { // Continue with empty config CreateDocument(); SetIgnoreSave(true);