From f0c1764adca7e3bd01ebd5417e2f1f75804439e3 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Mon, 1 Aug 2022 17:07:00 +0200 Subject: [PATCH 1/5] Introduce AtomicFile::Write() --- lib/base/atomic-file.cpp | 7 +++++++ lib/base/atomic-file.hpp | 2 ++ 2 files changed, 9 insertions(+) diff --git a/lib/base/atomic-file.cpp b/lib/base/atomic-file.cpp index 361a3d24c..762f38465 100644 --- a/lib/base/atomic-file.cpp +++ b/lib/base/atomic-file.cpp @@ -15,6 +15,13 @@ using namespace icinga; +void AtomicFile::Write(String path, int mode, const String& content) +{ + AtomicFile af (path, mode); + af << content; + af.Commit(); +} + AtomicFile::AtomicFile(String path, int mode) : m_Path(std::move(path)) { m_TempFilename = m_Path + ".tmp.XXXXXX"; diff --git a/lib/base/atomic-file.hpp b/lib/base/atomic-file.hpp index 5ad79d914..3a95c092b 100644 --- a/lib/base/atomic-file.hpp +++ b/lib/base/atomic-file.hpp @@ -18,6 +18,8 @@ namespace icinga class AtomicFile : public boost::iostreams::stream { public: + static void Write(String path, int mode, const String& content); + AtomicFile(String path, int mode); ~AtomicFile(); From 34844c146ddcd2fb1602f3946432f6e7e16b8b93 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Mon, 1 Aug 2022 17:44:05 +0200 Subject: [PATCH 2/5] Deduplicate and stabilize fragile filesystem transactions by using AtomicFile so they ensure all or nothing of a file gets replaced. --- lib/base/scriptglobal.cpp | 13 +++---------- lib/base/utility.cpp | 12 ++---------- lib/cli/apisetuputility.cpp | 8 +++----- lib/cli/nodesetupcommand.cpp | 27 ++++++++------------------- lib/cli/nodeutility.cpp | 22 +++++++--------------- lib/cli/nodewizardcommand.cpp | 27 ++++++++------------------- lib/compat/statusdatawriter.cpp | 15 +++++---------- lib/config/configcompilercontext.cpp | 25 ++++--------------------- lib/config/configcompilercontext.hpp | 6 +++--- lib/icinga/icingaapplication.cpp | 11 ++++------- lib/remote/apilistener.cpp | 10 ++-------- lib/remote/jsonrpcconnection-pki.cpp | 15 +++------------ 12 files changed, 52 insertions(+), 139 deletions(-) diff --git a/lib/base/scriptglobal.cpp b/lib/base/scriptglobal.cpp index 545c09706..2cb5f40e2 100644 --- a/lib/base/scriptglobal.cpp +++ b/lib/base/scriptglobal.cpp @@ -1,5 +1,6 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ +#include "base/atomic-file.hpp" #include "base/scriptglobal.hpp" #include "base/singleton.hpp" #include "base/logger.hpp" @@ -83,12 +84,7 @@ void ScriptGlobal::WriteToFile(const String& filename) Log(LogInformation, "ScriptGlobal") << "Dumping variables to file '" << filename << "'"; - std::fstream fp; - String tempFilename = Utility::CreateTempFile(filename + ".XXXXXX", 0600, fp); - - if (!fp) - BOOST_THROW_EXCEPTION(std::runtime_error("Could not open '" + tempFilename + "' file")); - + AtomicFile fp (filename, 0600); StdioStream::Ptr sfp = new StdioStream(&fp, false); ObjectLock olock(m_Globals); @@ -109,9 +105,6 @@ void ScriptGlobal::WriteToFile(const String& filename) } sfp->Close(); - - fp.close(); - - Utility::RenameFile(tempFilename, filename); + fp.Commit(); } diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index a2930a0f8..a78cef1d8 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -1,5 +1,6 @@ /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */ +#include "base/atomic-file.hpp" #include "base/utility.hpp" #include "base/convert.hpp" #include "base/application.hpp" @@ -1443,16 +1444,7 @@ Value Utility::LoadJsonFile(const String& path) void Utility::SaveJsonFile(const String& path, int mode, const Value& value) { - namespace fs = boost::filesystem; - - std::fstream fp; - String tempFilename = Utility::CreateTempFile(path + ".XXXXXX", mode, fp); - - fp.exceptions(std::ofstream::failbit | std::ofstream::badbit); - fp << JsonEncode(value); - fp.close(); - - RenameFile(tempFilename, path); + AtomicFile::Write(path, mode, JsonEncode(value)); } static void HexEncode(char ch, std::ostream& os) diff --git a/lib/cli/apisetuputility.cpp b/lib/cli/apisetuputility.cpp index 21583af0a..8bdd76796 100644 --- a/lib/cli/apisetuputility.cpp +++ b/lib/cli/apisetuputility.cpp @@ -5,6 +5,7 @@ #include "cli/featureutility.hpp" #include "remote/apilistener.hpp" #include "remote/pkiutility.hpp" +#include "base/atomic-file.hpp" #include "base/logger.hpp" #include "base/console.hpp" #include "base/application.hpp" @@ -160,8 +161,7 @@ bool ApiSetupUtility::SetupMasterApiUser() NodeUtility::CreateBackupFile(apiUsersPath); - std::fstream fp; - String tempFilename = Utility::CreateTempFile(apiUsersPath + ".XXXXXX", 0644, fp); + AtomicFile fp (apiUsersPath, 0644); fp << "/**\n" << " * The ApiUser objects are used for authentication against the API.\n" @@ -173,9 +173,7 @@ bool ApiSetupUtility::SetupMasterApiUser() << " permissions = [ \"*\" ]\n" << "}\n"; - fp.close(); - - Utility::RenameFile(tempFilename, apiUsersPath); + fp.Commit(); return true; } diff --git a/lib/cli/nodesetupcommand.cpp b/lib/cli/nodesetupcommand.cpp index 688ff9775..da63d003c 100644 --- a/lib/cli/nodesetupcommand.cpp +++ b/lib/cli/nodesetupcommand.cpp @@ -6,6 +6,7 @@ #include "cli/apisetuputility.hpp" #include "remote/apilistener.hpp" #include "remote/pkiutility.hpp" +#include "base/atomic-file.hpp" #include "base/logger.hpp" #include "base/console.hpp" #include "base/application.hpp" @@ -172,8 +173,7 @@ int NodeSetupCommand::SetupMaster(const boost::program_options::variables_map& v String apipath = FeatureUtility::GetFeaturesAvailablePath() + "/api.conf"; NodeUtility::CreateBackupFile(apipath); - std::fstream fp; - String tempApiPath = Utility::CreateTempFile(apipath + ".XXXXXX", 0644, fp); + AtomicFile fp (apipath, 0644); fp << "/**\n" << " * The API listener is used for distributed monitoring setups.\n" @@ -205,9 +205,7 @@ int NodeSetupCommand::SetupMaster(const boost::program_options::variables_map& v << " ticket_salt = TicketSalt\n" << "}\n"; - fp.close(); - - Utility::RenameFile(tempApiPath, apipath); + fp.Commit(); /* update constants.conf with NodeName = CN + TicketSalt = random value */ if (endpointName != Utility::GetFQDN()) { @@ -447,8 +445,7 @@ int NodeSetupCommand::SetupNode(const boost::program_options::variables_map& vm, String apipath = FeatureUtility::GetFeaturesAvailablePath() + "/api.conf"; NodeUtility::CreateBackupFile(apipath); - std::fstream fp; - String tempApiPath = Utility::CreateTempFile(apipath + ".XXXXXX", 0644, fp); + AtomicFile fp (apipath, 0644); fp << "/**\n" << " * The API listener is used for distributed monitoring setups.\n" @@ -479,9 +476,7 @@ int NodeSetupCommand::SetupNode(const boost::program_options::variables_map& vm, fp << "\n" << "}\n"; - fp.close(); - - Utility::RenameFile(tempApiPath, apipath); + fp.Commit(); /* Generate zones configuration. */ Log(LogInformation, "cli", "Generating zone and object configuration."); @@ -530,20 +525,14 @@ int NodeSetupCommand::SetupNode(const boost::program_options::variables_map& vm, if (!ticket.IsEmpty()) { String ticketPath = ApiListener::GetCertsDir() + "/ticket"; - String tempTicketPath = Utility::CreateTempFile(ticketPath + ".XXXXXX", 0600, fp); + AtomicFile::Write(ticketPath, 0600, ticket); - if (!Utility::SetFileOwnership(tempTicketPath, user, group)) { + if (!Utility::SetFileOwnership(ticketPath, user, group)) { Log(LogWarning, "cli") << "Cannot set ownership for user '" << user << "' group '" << group - << "' on file '" << tempTicketPath << "'. Verify it yourself!"; + << "' on file '" << ticketPath << "'. Verify it yourself!"; } - - fp << ticket; - - fp.close(); - - Utility::RenameFile(tempTicketPath, ticketPath); } /* If no parent connection was made, the user must supply the ca.crt before restarting Icinga 2.*/ diff --git a/lib/cli/nodeutility.cpp b/lib/cli/nodeutility.cpp index bd343ab4c..9fae63fd4 100644 --- a/lib/cli/nodeutility.cpp +++ b/lib/cli/nodeutility.cpp @@ -3,6 +3,7 @@ #include "cli/nodeutility.hpp" #include "cli/clicommand.hpp" #include "cli/variableutility.hpp" +#include "base/atomic-file.hpp" #include "base/logger.hpp" #include "base/application.hpp" #include "base/tlsutility.hpp" @@ -165,8 +166,7 @@ bool NodeUtility::WriteNodeConfigObjects(const String& filename, const Array::Pt << "Cannot set ownership for user '" << user << "' group '" << group << "' on path '" << path << "'. Verify it yourself!"; } - std::fstream fp; - String tempFilename = Utility::CreateTempFile(filename + ".XXXXXX", 0644, fp); + AtomicFile fp (filename, 0644); fp << "/*\n"; fp << " * Generated by Icinga 2 node setup commands\n"; @@ -179,9 +179,7 @@ bool NodeUtility::WriteNodeConfigObjects(const String& filename, const Array::Pt } fp << std::endl; - fp.close(); - - Utility::RenameFile(tempFilename, filename); + fp.Commit(); return true; } @@ -299,8 +297,7 @@ bool NodeUtility::UpdateConfiguration(const String& value, bool include, bool re NodeUtility::CreateBackupFile(configurationFile); std::ifstream ifp(configurationFile.CStr()); - std::fstream ofp; - String tempFile = Utility::CreateTempFile(configurationFile + ".XXXXXX", 0644, ofp); + AtomicFile ofp (configurationFile, 0644); String affectedInclude = value; @@ -349,9 +346,7 @@ bool NodeUtility::UpdateConfiguration(const String& value, bool include, bool re } ifp.close(); - ofp.close(); - - Utility::RenameFile(tempFile, configurationFile); + ofp.Commit(); return (found || include); } @@ -366,8 +361,7 @@ void NodeUtility::UpdateConstant(const String& name, const String& value) NodeUtility::CreateBackupFile(constantsConfPath); std::ifstream ifp(constantsConfPath.CStr()); - std::fstream ofp; - String tempFile = Utility::CreateTempFile(constantsConfPath + ".XXXXXX", 0644, ofp); + AtomicFile ofp (constantsConfPath, 0644); bool found = false; @@ -384,7 +378,5 @@ void NodeUtility::UpdateConstant(const String& name, const String& value) ofp << "const " + name + " = \"" + value + "\"\n"; ifp.close(); - ofp.close(); - - Utility::RenameFile(tempFile, constantsConfPath); + ofp.Commit(); } diff --git a/lib/cli/nodewizardcommand.cpp b/lib/cli/nodewizardcommand.cpp index abf87d87e..1f7d33578 100644 --- a/lib/cli/nodewizardcommand.cpp +++ b/lib/cli/nodewizardcommand.cpp @@ -6,6 +6,7 @@ #include "cli/apisetuputility.hpp" #include "remote/apilistener.hpp" #include "remote/pkiutility.hpp" +#include "base/atomic-file.hpp" #include "base/logger.hpp" #include "base/console.hpp" #include "base/application.hpp" @@ -451,8 +452,7 @@ wizard_ticket: String apiConfPath = FeatureUtility::GetFeaturesAvailablePath() + "/api.conf"; NodeUtility::CreateBackupFile(apiConfPath); - std::fstream fp; - String tempApiConfPath = Utility::CreateTempFile(apiConfPath + ".XXXXXX", 0644, fp); + AtomicFile fp (apiConfPath, 0644); fp << "/**\n" << " * The API listener is used for distributed monitoring setups.\n" @@ -468,9 +468,7 @@ wizard_ticket: fp << "}\n"; - fp.close(); - - Utility::RenameFile(tempApiConfPath, apiConfPath); + fp.Commit(); /* Zones configuration. */ Log(LogInformation, "cli", "Generating local zones.conf."); @@ -556,20 +554,14 @@ wizard_global_zone_loop_start: if (!ticket.IsEmpty()) { String ticketPath = ApiListener::GetCertsDir() + "/ticket"; - String tempTicketPath = Utility::CreateTempFile(ticketPath + ".XXXXXX", 0600, fp); + AtomicFile::Write(ticketPath, 0600, ticket); - if (!Utility::SetFileOwnership(tempTicketPath, user, group)) { + if (!Utility::SetFileOwnership(ticketPath, user, group)) { Log(LogWarning, "cli") << "Cannot set ownership for user '" << user << "' group '" << group - << "' on file '" << tempTicketPath << "'. Verify it yourself!"; + << "' on file '" << ticketPath << "'. Verify it yourself!"; } - - fp << ticket; - - fp.close(); - - Utility::RenameFile(tempTicketPath, ticketPath); } /* If no parent connection was made, the user must supply the ca.crt before restarting Icinga 2.*/ @@ -745,8 +737,7 @@ wizard_global_zone_loop_start: String apiConfPath = FeatureUtility::GetFeaturesAvailablePath() + "/api.conf"; NodeUtility::CreateBackupFile(apiConfPath); - std::fstream fp; - String tempApiConfPath = Utility::CreateTempFile(apiConfPath + ".XXXXXX", 0644, fp); + AtomicFile fp (apiConfPath, 0644); fp << "/**\n" << " * The API listener is used for distributed monitoring setups.\n" @@ -762,9 +753,7 @@ wizard_global_zone_loop_start: << " ticket_salt = TicketSalt\n" << "}\n"; - fp.close(); - - Utility::RenameFile(tempApiConfPath, apiConfPath); + fp.Commit(); /* update constants.conf with NodeName = CN + TicketSalt = random value */ if (cn != Utility::GetFQDN()) { diff --git a/lib/compat/statusdatawriter.cpp b/lib/compat/statusdatawriter.cpp index 2c6a666f1..72eb55efb 100644 --- a/lib/compat/statusdatawriter.cpp +++ b/lib/compat/statusdatawriter.cpp @@ -13,6 +13,7 @@ #include "icinga/compatutility.hpp" #include "icinga/pluginutility.hpp" #include "icinga/dependency.hpp" +#include "base/atomic-file.hpp" #include "base/configtype.hpp" #include "base/objectlock.hpp" #include "base/json.hpp" @@ -544,8 +545,7 @@ void StatusDataWriter::UpdateObjectsCache() /* Use the compat path here from the .ti generated class. */ String objectsPath = GetObjectsPath(); - std::fstream objectfp; - String tempObjectsPath = Utility::CreateTempFile(objectsPath + ".XXXXXX", 0644, objectfp); + AtomicFile objectfp (objectsPath, 0644); objectfp << std::fixed; @@ -760,9 +760,7 @@ void StatusDataWriter::UpdateObjectsCache() } } - objectfp.close(); - - Utility::RenameFile(tempObjectsPath, objectsPath); + objectfp.Commit(); } /** @@ -779,8 +777,7 @@ void StatusDataWriter::StatusTimerHandler() String statusPath = GetStatusPath(); - std::fstream statusfp; - String tempStatusPath = Utility::CreateTempFile(statusPath + ".XXXXXX", 0644, statusfp); + AtomicFile statusfp (statusPath, 0644); statusfp << std::fixed; @@ -833,9 +830,7 @@ void StatusDataWriter::StatusTimerHandler() } } - statusfp.close(); - - Utility::RenameFile(tempStatusPath, statusPath); + statusfp.Commit(); Log(LogNotice, "StatusDataWriter") << "Writing status.dat file took " << Utility::FormatDuration(Utility::GetTime() - start); diff --git a/lib/config/configcompilercontext.cpp b/lib/config/configcompilercontext.cpp index 666fe560e..9a8a7a671 100644 --- a/lib/config/configcompilercontext.cpp +++ b/lib/config/configcompilercontext.cpp @@ -17,20 +17,12 @@ ConfigCompilerContext *ConfigCompilerContext::GetInstance() void ConfigCompilerContext::OpenObjectsFile(const String& filename) { - m_ObjectsPath = filename; - - auto *fp = new std::fstream(); try { - m_ObjectsTempFile = Utility::CreateTempFile(filename + ".XXXXXX", 0600, *fp); + m_ObjectsFP.reset(new AtomicFile(filename, 0600)); } catch (const std::exception& ex) { Log(LogCritical, "cli", "Could not create temporary objects file: " + DiagnosticInformation(ex, false)); Application::Exit(1); } - - if (!*fp) - BOOST_THROW_EXCEPTION(std::runtime_error("Could not open '" + m_ObjectsTempFile + "' file")); - - m_ObjectsFP = fp; } void ConfigCompilerContext::WriteObject(const Dictionary::Ptr& object) @@ -48,21 +40,12 @@ void ConfigCompilerContext::WriteObject(const Dictionary::Ptr& object) void ConfigCompilerContext::CancelObjectsFile() { - delete m_ObjectsFP; - m_ObjectsFP = nullptr; - -#ifdef _WIN32 - _unlink(m_ObjectsTempFile.CStr()); -#else /* _WIN32 */ - unlink(m_ObjectsTempFile.CStr()); -#endif /* _WIN32 */ + m_ObjectsFP.reset(nullptr); } void ConfigCompilerContext::FinishObjectsFile() { - delete m_ObjectsFP; - m_ObjectsFP = nullptr; - - Utility::RenameFile(m_ObjectsTempFile, m_ObjectsPath); + m_ObjectsFP->Commit(); + m_ObjectsFP.reset(nullptr); } diff --git a/lib/config/configcompilercontext.hpp b/lib/config/configcompilercontext.hpp index f02e26869..e9023b4b9 100644 --- a/lib/config/configcompilercontext.hpp +++ b/lib/config/configcompilercontext.hpp @@ -4,8 +4,10 @@ #define CONFIGCOMPILERCONTEXT_H #include "config/i2-config.hpp" +#include "base/atomic-file.hpp" #include "base/dictionary.hpp" #include +#include #include namespace icinga @@ -25,9 +27,7 @@ public: static ConfigCompilerContext *GetInstance(); private: - String m_ObjectsPath; - String m_ObjectsTempFile; - std::fstream *m_ObjectsFP{nullptr}; + std::unique_ptr m_ObjectsFP; mutable std::mutex m_Mutex; }; diff --git a/lib/icinga/icingaapplication.cpp b/lib/icinga/icingaapplication.cpp index 1ff303dcb..d455572d5 100644 --- a/lib/icinga/icingaapplication.cpp +++ b/lib/icinga/icingaapplication.cpp @@ -5,6 +5,7 @@ #include "icinga/cib.hpp" #include "icinga/macroprocessor.hpp" #include "config/configcompiler.hpp" +#include "base/atomic-file.hpp" #include "base/configwriter.hpp" #include "base/configtype.hpp" #include "base/exception.hpp" @@ -121,7 +122,7 @@ void IcingaApplication::OnShutdown() DumpProgramState(); } -static void PersistModAttrHelper(std::fstream& fp, ConfigObject::Ptr& previousObject, const ConfigObject::Ptr& object, const String& attr, const Value& value) +static void PersistModAttrHelper(AtomicFile& fp, ConfigObject::Ptr& previousObject, const ConfigObject::Ptr& object, const String& attr, const Value& value) { if (object != previousObject) { if (previousObject) { @@ -170,9 +171,7 @@ void IcingaApplication::DumpModifiedAttributes() Log(LogWarning, "IcingaApplication") << DiagnosticInformation(ex); } - std::fstream fp; - String tempFilename = Utility::CreateTempFile(path + ".tmp.XXXXXX", 0644, fp); - fp.exceptions(std::ofstream::failbit | std::ofstream::badbit); + AtomicFile fp (path, 0644); ConfigObject::Ptr previousObject; ConfigObject::DumpModifiedAttributes([&fp, &previousObject](const ConfigObject::Ptr& object, const String& attr, const Value& value) { @@ -185,9 +184,7 @@ void IcingaApplication::DumpModifiedAttributes() ConfigWriter::EmitRaw(fp, "\n}\n"); } - fp.close(); - - Utility::RenameFile(tempFilename, path); + fp.Commit(); } IcingaApplication::Ptr IcingaApplication::GetInstance() diff --git a/lib/remote/apilistener.cpp b/lib/remote/apilistener.cpp index 472bd9021..c40933de8 100644 --- a/lib/remote/apilistener.cpp +++ b/lib/remote/apilistener.cpp @@ -8,6 +8,7 @@ #include "remote/apifunction.hpp" #include "remote/configpackageutility.hpp" #include "remote/configobjectutility.hpp" +#include "base/atomic-file.hpp" #include "base/convert.hpp" #include "base/defer.hpp" #include "base/io-engine.hpp" @@ -324,14 +325,7 @@ void ApiListener::RenewOwnCert() return; } - std::fstream certfp; - auto tempCertPath (Utility::CreateTempFile(certPath + ".XXXXXX", 0644, certfp)); - - certfp.exceptions(std::ofstream::failbit | std::ofstream::badbit); - certfp << CertificateToString(cert); - certfp.close(); - - Utility::RenameFile(tempCertPath, certPath); + AtomicFile::Write(certPath, 0644, CertificateToString(cert)); UpdateSSLContext(); } diff --git a/lib/remote/jsonrpcconnection-pki.cpp b/lib/remote/jsonrpcconnection-pki.cpp index d661dcf90..611e03833 100644 --- a/lib/remote/jsonrpcconnection-pki.cpp +++ b/lib/remote/jsonrpcconnection-pki.cpp @@ -4,6 +4,7 @@ #include "remote/apilistener.hpp" #include "remote/apifunction.hpp" #include "remote/jsonrpc.hpp" +#include "base/atomic-file.hpp" #include "base/configtype.hpp" #include "base/objectlock.hpp" #include "base/utility.hpp" @@ -367,12 +368,7 @@ Value UpdateCertificateHandler(const MessageOrigin::Ptr& origin, const Dictionar Log(LogInformation, "JsonRpcConnection") << "Updating CA certificate in '" << caPath << "'."; - std::fstream cafp; - String tempCaPath = Utility::CreateTempFile(caPath + ".XXXXXX", 0644, cafp); - cafp << ca; - cafp.close(); - - Utility::RenameFile(tempCaPath, caPath); + AtomicFile::Write(caPath, 0644, ca); /* Update signed certificate. */ String certPath = listener->GetDefaultCertPath(); @@ -380,12 +376,7 @@ Value UpdateCertificateHandler(const MessageOrigin::Ptr& origin, const Dictionar Log(LogInformation, "JsonRpcConnection") << "Updating client certificate for CN '" << cn << "' in '" << certPath << "'."; - std::fstream certfp; - String tempCertPath = Utility::CreateTempFile(certPath + ".XXXXXX", 0644, certfp); - certfp << cert; - certfp.close(); - - Utility::RenameFile(tempCertPath, certPath); + AtomicFile::Write(certPath, 0644, cert); /* Remove ticket for successful signing request. */ String ticketPath = ApiListener::GetCertsDir() + "/ticket"; From 69b3c81ea1c7e7ec150156817b553261029db5ad Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Mon, 1 Aug 2022 17:50:17 +0200 Subject: [PATCH 3/5] Remove unused Utility::CreateTempFile() --- lib/base/utility.cpp | 40 ---------------------------------------- lib/base/utility.hpp | 2 -- 2 files changed, 42 deletions(-) diff --git a/lib/base/utility.cpp b/lib/base/utility.cpp index a78cef1d8..6ff84ae65 100644 --- a/lib/base/utility.cpp +++ b/lib/base/utility.cpp @@ -1783,46 +1783,6 @@ String Utility::ValidateUTF8(const String& input) return String(std::move(output)); } -String Utility::CreateTempFile(const String& path, int mode, std::fstream& fp) -{ - std::vector targetPath(path.Begin(), path.End()); - targetPath.push_back('\0'); - - int fd; -#ifndef _WIN32 - fd = mkstemp(&targetPath[0]); -#else /* _WIN32 */ - fd = MksTemp(&targetPath[0]); -#endif /*_WIN32*/ - - if (fd < 0) { - BOOST_THROW_EXCEPTION(posix_error() - << boost::errinfo_api_function("mkstemp") - << boost::errinfo_errno(errno) - << boost::errinfo_file_name(path)); - } - - try { - fp.open(&targetPath[0], std::ios_base::trunc | std::ios_base::out); - } catch (const std::fstream::failure&) { - close(fd); - throw; - } - - close(fd); - - String resultPath = String(targetPath.begin(), targetPath.end() - 1); - - if (chmod(resultPath.CStr(), mode) < 0) { - BOOST_THROW_EXCEPTION(posix_error() - << boost::errinfo_api_function("chmod") - << boost::errinfo_errno(errno) - << boost::errinfo_file_name(resultPath)); - } - - return resultPath; -} - #ifdef _WIN32 /* mkstemp extracted from libc/sysdeps/posix/tempname.c. Copyright * (C) 1991-1999, 2000, 2001, 2006 Free Software Foundation, Inc. diff --git a/lib/base/utility.hpp b/lib/base/utility.hpp index 6f9d03d44..47b68d251 100644 --- a/lib/base/utility.hpp +++ b/lib/base/utility.hpp @@ -132,8 +132,6 @@ public: static String ValidateUTF8(const String& input); - static String CreateTempFile(const String& path, int mode, std::fstream& fp); - #ifdef _WIN32 static int MksTemp(char *tmpl); #endif /* _WIN32 */ From d2e3a094c146f5d16ebd7098edc13285b5f13f51 Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Tue, 2 Aug 2022 15:47:22 +0200 Subject: [PATCH 4/5] Introduce AtomicFile#GetTempFilename() --- lib/base/atomic-file.hpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/base/atomic-file.hpp b/lib/base/atomic-file.hpp index 3a95c092b..c5c7897ca 100644 --- a/lib/base/atomic-file.hpp +++ b/lib/base/atomic-file.hpp @@ -23,6 +23,11 @@ public: AtomicFile(String path, int mode); ~AtomicFile(); + inline const String& GetTempFilename() const noexcept + { + return m_TempFilename; + } + void Commit(); private: From 9ea9b1001405543dc69897fb67e56147002b243e Mon Sep 17 00:00:00 2001 From: "Alexander A. Klimov" Date: Fri, 27 Jan 2023 11:51:11 +0100 Subject: [PATCH 5/5] Include Utility::SetFileOwnership() inside FS transactions to make them even more atomic. --- lib/cli/nodesetupcommand.cpp | 8 +++++--- lib/cli/nodewizardcommand.cpp | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/cli/nodesetupcommand.cpp b/lib/cli/nodesetupcommand.cpp index da63d003c..2a685b503 100644 --- a/lib/cli/nodesetupcommand.cpp +++ b/lib/cli/nodesetupcommand.cpp @@ -524,15 +524,17 @@ int NodeSetupCommand::SetupNode(const boost::program_options::variables_map& vm, if (!ticket.IsEmpty()) { String ticketPath = ApiListener::GetCertsDir() + "/ticket"; + AtomicFile af (ticketPath, 0600); - AtomicFile::Write(ticketPath, 0600, ticket); - - if (!Utility::SetFileOwnership(ticketPath, user, group)) { + if (!Utility::SetFileOwnership(af.GetTempFilename(), user, group)) { Log(LogWarning, "cli") << "Cannot set ownership for user '" << user << "' group '" << group << "' on file '" << ticketPath << "'. Verify it yourself!"; } + + af << ticket; + af.Commit(); } /* If no parent connection was made, the user must supply the ca.crt before restarting Icinga 2.*/ diff --git a/lib/cli/nodewizardcommand.cpp b/lib/cli/nodewizardcommand.cpp index 1f7d33578..3a3cd42bd 100644 --- a/lib/cli/nodewizardcommand.cpp +++ b/lib/cli/nodewizardcommand.cpp @@ -553,15 +553,17 @@ wizard_global_zone_loop_start: if (!ticket.IsEmpty()) { String ticketPath = ApiListener::GetCertsDir() + "/ticket"; + AtomicFile af (ticketPath, 0600); - AtomicFile::Write(ticketPath, 0600, ticket); - - if (!Utility::SetFileOwnership(ticketPath, user, group)) { + if (!Utility::SetFileOwnership(af.GetTempFilename(), user, group)) { Log(LogWarning, "cli") << "Cannot set ownership for user '" << user << "' group '" << group << "' on file '" << ticketPath << "'. Verify it yourself!"; } + + af << ticket; + af.Commit(); } /* If no parent connection was made, the user must supply the ca.crt before restarting Icinga 2.*/