From 38a4163173f6f44bdb5f8e43012d5e639737aff9 Mon Sep 17 00:00:00 2001 From: Connor McLaughlin Date: Wed, 10 Aug 2022 17:45:34 +1000 Subject: [PATCH] IniSettingsInterface: Make writes atomic Fixes potential settings corruption if we crash while saving. --- src/util/ini_settings_interface.cpp | 63 ++++++++++++++++++++++++++++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/src/util/ini_settings_interface.cpp b/src/util/ini_settings_interface.cpp index ef44c978b0..0c1e922834 100644 --- a/src/util/ini_settings_interface.cpp +++ b/src/util/ini_settings_interface.cpp @@ -1,16 +1,64 @@ #include "ini_settings_interface.h" #include "common/file_system.h" #include "common/log.h" +#include "common/path.h" #include "common/string_util.h" #include #include #include Log_SetChannel(INISettingsInterface); +#ifdef _WIN32 +#include // _mktemp_s +#else +#include // mktemp +#endif + // To prevent races between saving and loading settings, particularly with game settings, // we only allow one ini to be parsed at any point in time. static std::mutex s_ini_load_save_mutex; +static std::string GetTemporaryFileName(const std::string& original_filename) +{ + std::string temporary_filename; + temporary_filename.reserve(original_filename.length() + 8); + +#ifdef _WIN32 + // On UWP, preserve the extension, as it affects permissions. +#ifdef _UWP + const std::string_view original_view(original_filename); + const std::string_view extension(Path::GetExtension(original_view)); + if (extension.length() < original_filename.length()) + { + temporary_filename.append(original_view.substr(0, original_filename.length() - extension.length() - 1)); + temporary_filename.append("_XXXXXX"); + _mktemp_s(temporary_filename.data(), temporary_filename.length() + 1); + temporary_filename += '.'; + temporary_filename.append(extension); + } + else + { + temporary_filename.append(original_filename); + temporary_filename.append(".XXXXXXX"); + _mktemp_s(temporary_filename.data(), temporary_filename.length() + 1); + } +#else + temporary_filename.append(original_filename); + temporary_filename.append(".XXXXXXX"); + _mktemp_s(temporary_filename.data(), temporary_filename.length() + 1); +#endif +#else + temporary_filename.append(".XXXXXX"); +#if defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__) + mkstemp(temporary_filename.data()); +#else + mktemp(temporary_filename.data()); +#endif +#endif + + return temporary_filename; +} + INISettingsInterface::INISettingsInterface(std::string filename) : m_filename(std::move(filename)), m_ini(true, true) {} INISettingsInterface::~INISettingsInterface() @@ -39,12 +87,25 @@ bool INISettingsInterface::Save() return false; std::unique_lock lock(s_ini_load_save_mutex); + std::string temp_filename(GetTemporaryFileName(m_filename)); SI_Error err = SI_FAIL; - std::FILE* fp = FileSystem::OpenCFile(m_filename.c_str(), "wb"); + std::FILE* fp = FileSystem::OpenCFile(temp_filename.c_str(), "wb"); if (fp) { err = m_ini.SaveFile(fp, false); std::fclose(fp); + + if (err != SI_OK) + { + // remove temporary file + FileSystem::DeleteFile(temp_filename.c_str()); + } + else if (!FileSystem::RenamePath(temp_filename.c_str(), m_filename.c_str())) + { + Log_ErrorPrintf("Failed to rename '%s' to '%s'", temp_filename.c_str(), m_filename.c_str()); + FileSystem::DeleteFile(temp_filename.c_str()); + return false; + } } if (err != SI_OK)