From ac51acdafe0a607f13adf5c03b6ff18a5bdae971 Mon Sep 17 00:00:00 2001 From: bicarus <202771338+bicarus-dev@users.noreply.github.com> Date: Mon, 15 Jun 2026 02:06:04 -0700 Subject: [PATCH] cfg: fix config save atomicity (#760) ## Link to GitHub Issue or related Pull Request, if one exists n/a ## Description of change Fully flush `tmp` file and then use `MoveFileExW` instead of `CopyFileW` which guarantees atomic operation on NTFS. ## Testing --- src/spice2x/cfg/config.cpp | 27 +++++++++++++++----- src/spice2x/cfg/controller_presets.cpp | 13 ++++------ src/spice2x/overlay/windows/card_manager.cpp | 2 +- src/spice2x/util/fileutils.cpp | 16 +++++++++++- 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/spice2x/cfg/config.cpp b/src/spice2x/cfg/config.cpp index 8a80c0e..5f72d90 100644 --- a/src/spice2x/cfg/config.cpp +++ b/src/spice2x/cfg/config.cpp @@ -1331,17 +1331,32 @@ bool Config::firstFillConfigFile() { } void Config::saveConfigFile() { - // create a .tmp file and write to it... + // write the new config to a .tmp file first... const auto xml_result = this->configFile.SaveFile(this->configLocationTemp.c_str(), false); if (xml_result != tinyxml2::XMLError::XML_SUCCESS) { log_info("cfg", "failed to write file: {}", this->configLocationTemp); return; } - // copy the .tmp file to the main file... - if (CopyFileW(this->configLocationTemp.c_str(), this->configLocation.c_str(), false) == 0) { - log_warning("cfg", "CopyFileA failed: 0x{:08x}", GetLastError()); + + // ...flush the .tmp file to disk so a crash/power loss can't leave it half-written... + HANDLE tmp_handle = CreateFileW( + this->configLocationTemp.c_str(), + GENERIC_WRITE, FILE_SHARE_READ, nullptr, + OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr); + if (tmp_handle != INVALID_HANDLE_VALUE) { + FlushFileBuffers(tmp_handle); + CloseHandle(tmp_handle); + } + + // ...then atomically replace the real config with the .tmp file. + // Unlike CopyFile (which truncates and rewrites the destination in place), + // an NTFS rename is atomic: the existing config is never left half-overwritten, + // so an interrupted save can't corrupt it. MoveFileEx also removes the .tmp on success. + if (MoveFileExW( + this->configLocationTemp.c_str(), + this->configLocation.c_str(), + MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) == 0) { + log_warning("cfg", "MoveFileExW failed: 0x{:08x}", GetLastError()); return; } - // delete the .tmp file (not critical if this fails) - DeleteFileW(this->configLocationTemp.c_str()); } diff --git a/src/spice2x/cfg/controller_presets.cpp b/src/spice2x/cfg/controller_presets.cpp index 9b9a3e3..a0496dc 100644 --- a/src/spice2x/cfg/controller_presets.cpp +++ b/src/spice2x/cfg/controller_presets.cpp @@ -359,13 +359,12 @@ namespace overlay::windows { return false; } - std::error_code ec; - std::filesystem::rename(path_tmp, path, ec); - if (ec) { - log_warning("templates", "failed to rename templates file: {}", ec.message()); + if (MoveFileExW(path_tmp.c_str(), path.c_str(), MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) == 0) { + log_warning("templates", "failed to rename templates file: 0x{:08x}", GetLastError()); return false; } + log_info("templates", "templates file saved successfully"); return true; } @@ -397,8 +396,7 @@ namespace overlay::windows { auto path_tmp = path; path_tmp.replace_extension(L"tmp"); if (doc.SaveFile(path_tmp.c_str()) == tinyxml2::XML_SUCCESS) { - std::error_code ec; - std::filesystem::rename(path_tmp, path, ec); + MoveFileExW(path_tmp.c_str(), path.c_str(), MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH); } } @@ -434,8 +432,7 @@ namespace overlay::windows { auto path_tmp = path; path_tmp.replace_extension(L"tmp"); if (doc.SaveFile(path_tmp.c_str()) == tinyxml2::XML_SUCCESS) { - std::error_code ec; - std::filesystem::rename(path_tmp, path, ec); + MoveFileExW(path_tmp.c_str(), path.c_str(), MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH); } } diff --git a/src/spice2x/overlay/windows/card_manager.cpp b/src/spice2x/overlay/windows/card_manager.cpp index fd62510..79425d7 100644 --- a/src/spice2x/overlay/windows/card_manager.cpp +++ b/src/spice2x/overlay/windows/card_manager.cpp @@ -536,7 +536,7 @@ namespace overlay::windows { } void CardManager::config_save() { - log_info("cardmanager", "saving config"); + log_misc("cardmanager", "saving config..."); // create document Document doc; diff --git a/src/spice2x/util/fileutils.cpp b/src/spice2x/util/fileutils.cpp index 9d05598..6f56bf2 100644 --- a/src/spice2x/util/fileutils.cpp +++ b/src/spice2x/util/fileutils.cpp @@ -298,5 +298,19 @@ bool fileutils::write_config_file(const std::string_view &module, const std::fil // save file log_info(module, "saving config file: {}", censored_display); - return fileutils::text_write(path, text); + + // write to a .tmp file first, then atomically replace the real config — this way + // a crash or disk-full mid-write can't corrupt the existing config file. + auto path_tmp = path; + path_tmp += L".tmp"; + if (!fileutils::text_write(path_tmp, text)) { + log_warning(module, "failed to write temp config file: {}", censored_display); + return false; + } + if (MoveFileExW(path_tmp.c_str(), path.c_str(), MOVEFILE_REPLACE_EXISTING | MOVEFILE_WRITE_THROUGH) == 0) { + log_warning(module, "MoveFileExW failed: 0x{:08x}", GetLastError()); + return false; + } + + return true; }