diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 6f1268bc1..c67f7b716 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -218,6 +218,16 @@ jobs: shell: pwsh run: sccache --show-stats + - name: Test startup config saves + shell: pwsh + env: + PACKAGE_DIR: ${{ steps.xmake_cache_paths.outputs.package_dir }} + run: | + $header = Get-ChildItem -LiteralPath (Join-Path $env:PACKAGE_DIR 't/toml++') -Recurse -Filter toml.h | + Where-Object { $_.Directory.Name -eq 'toml++' } | Select-Object -First 1 + if (-not $header) { throw 'Built toml++ package not found.' } + ./tests/run-config-save.ps1 -TomlInclude $header.Directory.Parent.FullName + - name: Package shell: pwsh run: | @@ -521,6 +531,16 @@ jobs: shell: bash run: sccache --show-stats + - name: Test startup config saves + shell: bash + env: + PACKAGE_DIR: ${{ steps.xmake_cache_paths.outputs.package_dir }} + run: | + set -euo pipefail + TOML_HEADER=$(find "$PACKAGE_DIR/t/toml++" -path '*/include/toml++/toml.h' -print -quit) + test -n "$TOML_HEADER" + bash tests/run-config-save.sh "$(dirname "$(dirname "$TOML_HEADER")")" + - name: Report Swift module cache shell: bash run: | diff --git a/docs/config-save.md b/docs/config-save.md new file mode 100644 index 000000000..608636154 --- /dev/null +++ b/docs/config-save.md @@ -0,0 +1,44 @@ +# Startup config saves + +`Config::Save` writes complete TOML documents for two startup callers: the initial +default config and the generated runtime snapshot. It keeps `File::MakePath` +routing and the existing generated-file warning. Save errors are logged once by +the caller; startup continues with the in-memory configuration. + +`SaveConfigDocument` serializes with toml++, parses the output before touching +disk, exclusively creates a private sibling temporary file, checks writing and closing, +and replaces the destination. No threads, frame callbacks, runtime controls or +shutdown interception are installed. This is not the preserving TOML editor: +whole-document saves do not merge concurrent setting changes or preserve comments. + +Staging is private before writing and remains private after close: Windows uses +a protected owner/system DACL, and macOS uses mode `0600`. New documents keep +these private permissions. + +Windows uses `ReplaceFileW` to preserve existing permissions and streams, with a +temporary backup for its documented partial-failure cases. A missing destination +falls back to a non-replacing move. The caller's startup existence check is not an +exclusive create transaction. Ordinary failures clean up the temporary file; partial +replacement failures retain recovery files and report their location. The backup +name is the reported temporary path plus `.bak`. Recovery is not automatic. +macOS uses rename after copying the existing permission bits. Extended metadata +and hard-link identity are not preserved by that path. Existing and dangling final symlinks are +resolved before staging; cyclic links fail without replacing the link. Replacement requires directory permissions in addition +to any file access checks; it cannot exactly match an in-place overwrite. + +Successful close/replacement is not a guarantee against power loss. A forced exit +can leave a temporary file. No automatic stale-file sweep is installed. + +Run the isolated Windows fixtures with `tests/run-config-save.ps1` after the +normal AX build has installed toml++; `-TomlInclude` can select another include +directory. Fixtures never access the installed game's files. + +On macOS, run `bash tests/run-config-save.sh TOML_INCLUDE_DIR`. Both native macOS +CI jobs run these fixtures after the normal build, including permission-bit and +symlink checks. The failure fixture injects short writes and failed closes at +compile time and checks staging permissions before/during writes and after close; +it does not install test controls in the mod. + +Native behavior references: +- [Windows ReplaceFileW](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-replacefilew) +- [POSIX rename](https://pubs.opengroup.org/onlinepubs/9799919799/functions/rename.html) diff --git a/mods/src/config.cc b/mods/src/config.cc index b69377fbb..e9d59de33 100644 --- a/mods/src/config.cc +++ b/mods/src/config.cc @@ -1,4 +1,5 @@ #include "config.h" +#include "config_save.h" #include "file.h" #include "patches/mapkey.h" #include "prime/KeyCode.h" @@ -93,10 +94,10 @@ Config::Config() void Config::Save(const toml::table& config, const std::string_view filename, bool apply_warning) { - std::ofstream config_file; + std::ostringstream config_file; auto config_path = File::MakePath(filename, true); - config_file.open(config_path); + config_file.exceptions(std::ios::badbit | std::ios::failbit); if (apply_warning) { char defaultFile[255], configFile[255]; @@ -118,8 +119,7 @@ void Config::Save(const toml::table& config, const std::string_view filename, bo config_file << "#######################################################################\n\n"; } - config_file << config; - config_file.close(); + SaveConfigDocument(config, std::filesystem::path(config_path), config_file.str()); } Config& Config::Get() @@ -1403,7 +1403,11 @@ void Config::Load() message << "Creating " << File::Config() << " (default config file)"; spdlog::warn(message.str()); - Config::Save(parsed, File::Config(), false); + try { + Config::Save(parsed, File::Config(), false); + } catch (const std::exception& error) { + spdlog::error("Could not save default config: {}", error.what()); + } } message.str(""); @@ -1418,7 +1422,11 @@ void Config::Load() std::filesystem::remove(FILE_DEF_PARSED); } - Config::Save(parsed, File::Vars()); + try { + Config::Save(parsed, File::Vars()); + } catch (const std::exception& error) { + spdlog::error("Could not save runtime config: {}", error.what()); + } std::cout << "\n\n-----------------------------\n\n" << parsed << "\n\n-----------------------------\nVersion " diff --git a/mods/src/config_save.cc b/mods/src/config_save.cc new file mode 100644 index 000000000..70ef49c87 --- /dev/null +++ b/mods/src/config_save.cc @@ -0,0 +1,141 @@ +#include "config_save.h" + +#include +#include +#include +#include +#include +#include + +#if _WIN32 +#include +#include +#include +#include +#pragma comment(lib, "advapi32.lib") +#else +#include +#include +#endif + +// Compile-time substitutions are used only by the isolated failure fixture. +#ifndef CONFIG_SAVE_WRITE +#define CONFIG_SAVE_WRITE std::fwrite +#endif +#ifndef CONFIG_SAVE_CLOSE +#define CONFIG_SAVE_CLOSE std::fclose +#endif + +void SaveConfigDocument(const toml::table& config, const std::filesystem::path& path, std::string_view header) +{ + // Serialize and validate before opening any file. Values are encoded by toml++, + // never interpolated into TOML source. Validate the header too. + std::ostringstream output; + output.exceptions(std::ios::badbit | std::ios::failbit); + output << header << config; + const auto bytes = output.str(); + (void)toml::parse(bytes); + + // weakly_canonical alone leaves a dangling final symlink unresolved. + // Follow it before staging, preserving the former ofstream behavior. + auto destination = std::filesystem::weakly_canonical(path); + unsigned links = 0; + while (std::filesystem::is_symlink(std::filesystem::symlink_status(destination))) { + if (++links > 40) + throw std::filesystem::filesystem_error("config symlink cycle", path, + std::make_error_code(std::errc::too_many_symbolic_link_levels)); + auto target = std::filesystem::read_symlink(destination); + destination = std::filesystem::weakly_canonical(target.is_absolute() ? target : destination.parent_path() / target); + } + static std::atomic sequence{0}; + auto temporary = destination; + temporary += ".tmp-" + std::to_string(std::chrono::steady_clock::now().time_since_epoch().count()) + "-" + + std::to_string(sequence.fetch_add(1, std::memory_order_relaxed)); + + // Configs may contain tokens. Protect staging at creation, before any bytes, + // even if the destination is private beneath a more permissive directory. + std::FILE* file = nullptr; +#if _WIN32 + PSECURITY_DESCRIPTOR security = nullptr; + if (!ConvertStringSecurityDescriptorToSecurityDescriptorW( + L"D:P(A;;FA;;;OW)(A;;FA;;;SY)", SDDL_REVISION_1, &security, nullptr)) + throw std::system_error(GetLastError(), std::system_category(), "could not protect temporary config file"); + SECURITY_ATTRIBUTES attributes{sizeof(SECURITY_ATTRIBUTES), security, FALSE}; + const auto handle = CreateFileW(temporary.c_str(), GENERIC_WRITE | READ_CONTROL, 0, &attributes, + CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr); + const auto create_error = GetLastError(); + LocalFree(security); + if (handle == INVALID_HANDLE_VALUE) + throw std::system_error(create_error, std::system_category(), "could not create temporary config file"); + const auto descriptor = _open_osfhandle(reinterpret_cast(handle), _O_WRONLY | _O_BINARY); + if (descriptor == -1) { + CloseHandle(handle); + } else { + file = _fdopen(descriptor, "wb"); + if (!file) + _close(descriptor); + } +#else + const auto descriptor = ::open(temporary.c_str(), O_WRONLY | O_CREAT | O_EXCL, 0600); + if (descriptor == -1) + throw std::system_error(errno, std::generic_category(), "could not create temporary config file"); + file = ::fdopen(descriptor, "wb"); + if (!file) + ::close(descriptor); +#endif + if (!file) { + const auto error = errno; + std::error_code ignored; + std::filesystem::remove(temporary, ignored); + throw std::system_error(error, std::generic_category(), "could not open temporary config stream"); + } + + bool replacing = false; + try { + if (CONFIG_SAVE_WRITE(bytes.data(), 1, bytes.size(), file) != bytes.size()) { + throw std::system_error(errno, std::generic_category(), "could not write temporary config file"); + } + const auto closed = CONFIG_SAVE_CLOSE(file); // Includes flushing; failure prevents replacement. + file = nullptr; + if (closed != 0) { + throw std::system_error(errno, std::generic_category(), "could not close temporary config file"); + } +#if _WIN32 + // Let Windows retain the existing file's permissions and streams. A backup + // protects the old contents in ReplaceFile's documented partial-failure cases. + auto backup = temporary; + backup += ".bak"; + if (!ReplaceFileW(destination.c_str(), temporary.c_str(), backup.c_str(), 0, nullptr, nullptr)) { + auto error = GetLastError(); + if (error == ERROR_FILE_NOT_FOUND) { + // Missing-destination fallback: do not overwrite a file appearing before + // this move. The caller's earlier existence check is not a create-only transaction. + error = MoveFileExW(temporary.c_str(), destination.c_str(), 0) ? ERROR_SUCCESS : GetLastError(); + } + if (error != ERROR_SUCCESS) { + replacing = error == ERROR_UNABLE_TO_MOVE_REPLACEMENT || error == ERROR_UNABLE_TO_MOVE_REPLACEMENT_2; + throw std::filesystem::filesystem_error( + replacing ? "config replacement failed; retain temporary/backup for recovery" : "config replacement failed", + temporary, destination, std::error_code(error, std::system_category())); + } + } + std::error_code ignored; + std::filesystem::remove(backup, ignored); +#else + // Preserve ordinary permission bits when replacing an existing config. + if (std::filesystem::exists(destination)) { + std::filesystem::permissions(temporary, std::filesystem::status(destination).permissions()); + } + std::filesystem::rename(temporary, destination); +#endif + } catch (...) { + if (file) { + std::fclose(file); + } + std::error_code ignored; + if (!replacing) { + std::filesystem::remove(temporary, ignored); + } + throw; + } +} diff --git a/mods/src/config_save.h b/mods/src/config_save.h new file mode 100644 index 000000000..2150f5225 --- /dev/null +++ b/mods/src/config_save.h @@ -0,0 +1,9 @@ +#pragma once + +#include +#include +#include + +// Synchronous whole-document output for startup, not a runtime setting editor. +// Throws on failure; the caller owns reporting. Does not guarantee power-loss durability. +void SaveConfigDocument(const toml::table& config, const std::filesystem::path& path, std::string_view header = {}); diff --git a/tests/config_save_failure_test.cc b/tests/config_save_failure_test.cc new file mode 100644 index 000000000..aadc0ef50 --- /dev/null +++ b/tests/config_save_failure_test.cc @@ -0,0 +1,113 @@ +#include +#include +#include +#include +#if _WIN32 +#include +#include +#include +#include +#else +#include +#endif + +static std::filesystem::path staging; +static void CheckPrivateStaging(std::FILE* file) +{ +#if _WIN32 + PACL acl = nullptr; + PSECURITY_DESCRIPTOR security = nullptr; + const auto handle = file ? reinterpret_cast(_get_osfhandle(_fileno(file))) : nullptr; + const auto error = file ? GetSecurityInfo(handle, SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, + nullptr, nullptr, &acl, nullptr, &security) + : GetNamedSecurityInfoW(const_cast(staging.c_str()), SE_FILE_OBJECT, DACL_SECURITY_INFORMATION, + nullptr, nullptr, &acl, nullptr, &security); + assert(error == ERROR_SUCCESS && acl && acl->AceCount == 2); + SECURITY_DESCRIPTOR_CONTROL control; + DWORD revision; + assert(GetSecurityDescriptorControl(security, &control, &revision) && (control & SE_DACL_PROTECTED)); + for (DWORD i = 0; i < acl->AceCount; ++i) { + void* entry = nullptr; + assert(GetAce(acl, i, &entry)); + const auto* ace = static_cast(entry); + assert(ace->Header.AceType == ACCESS_ALLOWED_ACE_TYPE && !(ace->Header.AceFlags & INHERITED_ACE)); + wchar_t* sid = nullptr; + assert(ConvertSidToStringSidW(const_cast(&ace->SidStart), &sid)); + assert(std::wstring_view(sid) == L"S-1-3-4" || std::wstring_view(sid) == L"S-1-5-18"); + LocalFree(sid); + } + LocalFree(security); +#else + struct stat status; + assert((file ? ::fstat(fileno(file), &status) : ::stat(staging.c_str(), &status)) == 0); + assert((status.st_mode & 0777) == 0600); +#endif +} + +static bool failClose = false; + +static std::size_t ShortWrite(const void* data, std::size_t size, std::size_t count, std::FILE* file) +{ + for (const auto& entry : std::filesystem::directory_iterator(staging.parent_path())) + if (entry.path().filename().string().find(".tmp-") != std::string::npos) + staging = entry.path(); + CheckPrivateStaging(file); + const auto written = std::fwrite(data, size, failClose ? count : count / 2, file); + CheckPrivateStaging(file); + if (failClose) + return written; + errno = ENOSPC; + return written; +} + +static int FailedClose(std::FILE* file) +{ + std::fclose(file); + CheckPrivateStaging(nullptr); + errno = ENOSPC; + return EOF; +} + +// Exercise the production cleanup path without adding runtime injection controls. +#define CONFIG_SAVE_WRITE ShortWrite +#define CONFIG_SAVE_CLOSE FailedClose +#include "../mods/src/config_save.cc" + +#include +#include +#include + +int main(int argc, char** argv) +{ + assert(argc == 2); + const std::filesystem::path root(argv[1]); + std::filesystem::create_directories(root); + const auto path = root / "settings.toml"; + staging = path; + const std::string original = "# keep this exactly\nenabled = false\n"; + { + std::ofstream out(path, std::ios::binary); + out << original; + } +#if !_WIN32 + ::umask(0022); + std::filesystem::permissions(path, std::filesystem::perms::owner_read | std::filesystem::perms::owner_write); +#endif + for (bool closeFailure : {false, true}) { + failClose = closeFailure; + bool failed = false; + try { + SaveConfigDocument(toml::table{{"enabled", true}}, path); + } catch (const std::system_error&) { + failed = true; + } + assert(failed); + std::ifstream input(path, std::ios::binary); + const std::string actual(std::istreambuf_iterator{input}, {}); + assert(actual == original); + for (const auto& entry : std::filesystem::directory_iterator(root)) { + assert(entry.path() == path); + } + } + std::cout << "Short-write and failed-close fixtures passed\n"; +} diff --git a/tests/config_save_test.cc b/tests/config_save_test.cc new file mode 100644 index 000000000..c4f713548 --- /dev/null +++ b/tests/config_save_test.cc @@ -0,0 +1,82 @@ +#include "config_save.h" + +#include +#include +#include + +#if _WIN32 +#include +#endif + +int main(int argc, char** argv) +{ + assert(argc == 2); + const std::filesystem::path root(argv[1]); + std::filesystem::create_directories(root); + const auto path = root / "settings.toml"; + const std::string value = "quotes: \"'\\\n[unexpected]\nenabled = true\nUnicode: \xc3\xa9"; + toml::table config{{"value", value}, {"enabled", false}}; + SaveConfigDocument(config, path, "# generated\n"); + auto parsed = toml::parse_file(path.string()); + assert(parsed["value"].value() == value); + assert(parsed.size() == 2); + config.insert_or_assign("enabled", true); + SaveConfigDocument(config, path); + assert(toml::parse_file(path.string())["enabled"].value() == true); + + bool failed = false; + try { + SaveConfigDocument(config, path, "invalid = [\n"); + } catch (const std::exception&) { + failed = true; + } + assert(failed); + assert(toml::parse_file(path.string())["value"].value() == value); + + const auto directory = root / "occupied"; + std::filesystem::create_directory(directory); + failed = false; + try { + SaveConfigDocument(config, directory); + } catch (const std::exception&) { + failed = true; + } + assert(failed && std::filesystem::is_directory(directory)); +#if _WIN32 + // A real sharing violation must leave the previous readable document intact. + auto handle = CreateFileW(path.c_str(), GENERIC_READ, FILE_SHARE_READ, nullptr, OPEN_EXISTING, 0, nullptr); + assert(handle != INVALID_HANDLE_VALUE); + failed = false; + config.insert_or_assign("enabled", false); + try { + SaveConfigDocument(config, path); + } catch (const std::exception&) { + failed = true; + } + CloseHandle(handle); + assert(failed); + assert(toml::parse_file(path.string())["enabled"].value() == true); +#else + const auto mode = std::filesystem::perms::owner_read | std::filesystem::perms::owner_write; + std::filesystem::permissions(path, mode); + const auto link = root / "linked.toml"; + std::filesystem::create_symlink(path, link); + config.insert_or_assign("enabled", false); + SaveConfigDocument(config, link); + assert(std::filesystem::is_symlink(link)); + assert(toml::parse_file(path.string())["enabled"].value() == false); + assert(std::filesystem::status(path).permissions() == mode); + for (bool relative : {false, true}) { + const auto target = root / (relative ? "relative-target.toml" : "absolute-target.toml"); + const auto dangling = root / (relative ? "relative-link.toml" : "absolute-link.toml"); + std::filesystem::create_symlink(relative ? target.filename() : std::filesystem::absolute(target), dangling); + SaveConfigDocument(config, dangling); + assert(std::filesystem::is_symlink(dangling)); + assert(toml::parse_file(target.string())["enabled"].value() == false); + } +#endif + for (const auto& entry : std::filesystem::directory_iterator(root)) { + assert(entry.path().filename().string().find(".tmp-") == std::string::npos); + } + std::cout << "Config save fixtures passed\n"; +} diff --git a/tests/run-config-save.ps1 b/tests/run-config-save.ps1 new file mode 100644 index 000000000..6c80cd0e5 --- /dev/null +++ b/tests/run-config-save.ps1 @@ -0,0 +1,64 @@ +[CmdletBinding()] +param([string]$TomlInclude) + +$ErrorActionPreference = 'Stop' +$repoRoot = Split-Path -Parent $PSScriptRoot +function Get-PermissionState([string]$Path) { + $acl = Get-Acl -LiteralPath $Path + # Windows can normalize descriptor control bits; compare actual rules, + # ownership and inheritance protection rather than serialized SDDL spelling. + [ordered]@{ + Owner = $acl.Owner + Group = $acl.Group + Protected = $acl.AreAccessRulesProtected + Rules = @($acl.Access | Select-Object IdentityReference, FileSystemRights, + AccessControlType, IsInherited, InheritanceFlags, PropagationFlags) + } | ConvertTo-Json -Depth 5 -Compress +} +Push-Location $repoRoot +try { + if (-not $TomlInclude) { + $packageRoot = Join-Path $env:LOCALAPPDATA '.xmake/packages/t/toml++' + $header = Get-ChildItem -LiteralPath $packageRoot -Recurse -Filter toml.h | + Where-Object { $_.Directory.Name -eq 'toml++' } | Select-Object -First 1 + if (-not $header) { throw 'Build with AX first, or supply -TomlInclude.' } + $TomlInclude = $header.Directory.Parent.FullName + } + New-Item -ItemType Directory -Force build/config-save-test | Out-Null + & clang++ --driver-mode=cl /std:c++latest /EHsc /MT /Imods/src "/I$TomlInclude" ` + tests/config_save_test.cc mods/src/config_save.cc /Febuild/config-save-test/test.exe ` + /Fobuild/config-save-test/ -Wno-deprecated-literal-operator + if ($LASTEXITCODE -ne 0) { throw 'Config save test compilation failed.' } + $fixtureRoot = Join-Path $repoRoot ('build/config-save-test/' + [guid]::NewGuid()) + & ./build/config-save-test/test.exe (Join-Path $fixtureRoot 'created') + if ($LASTEXITCODE -ne 0) { throw 'Initial config creation regression failed.' } + $fixtureRoot = Join-Path $fixtureRoot 'permissions' + # Establish the baseline independently, before the first production save. + New-Item -ItemType Directory -Path $fixtureRoot | Out-Null + $testFile = Join-Path $fixtureRoot 'settings.toml' + Set-Content -LiteralPath $testFile -Value 'enabled = false' + if (-not ((Get-Acl -LiteralPath $testFile).Access | Where-Object IsInherited)) { + throw 'Fixture must have inherited permission entries.' + } + $inherited = Get-PermissionState $testFile + & ./build/config-save-test/test.exe $fixtureRoot + if ($LASTEXITCODE -ne 0 -or (Get-PermissionState $testFile) -ne $inherited) { + throw 'Inherited ACL regression failed.' + } + $acl = Get-Acl -LiteralPath $testFile + $acl.SetAccessRuleProtection($true, $true) + Set-Acl -LiteralPath $testFile -AclObject $acl + $explicit = Get-PermissionState $testFile + & ./build/config-save-test/test.exe $fixtureRoot + if ($LASTEXITCODE -ne 0 -or (Get-PermissionState $testFile) -ne $explicit) { + throw 'Explicit ACL regression failed.' + } + & clang++ --driver-mode=cl /std:c++latest /EHsc /MT /Imods/src "/I$TomlInclude" ` + tests/config_save_failure_test.cc /Febuild/config-save-test/failure-test.exe ` + /Fobuild/config-save-test/ -Wno-deprecated-literal-operator + if ($LASTEXITCODE -ne 0) { throw 'Config failure test compilation failed.' } + & ./build/config-save-test/failure-test.exe (Join-Path $fixtureRoot 'failures') + if ($LASTEXITCODE -ne 0) { throw 'Config failure regression failed.' } +} finally { + Pop-Location +} diff --git a/tests/run-config-save.sh b/tests/run-config-save.sh new file mode 100644 index 000000000..c785669f2 --- /dev/null +++ b/tests/run-config-save.sh @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Pass the include directory of the toml++ package used by the normal build. +toml_include="${1:?usage: run-config-save.sh TOML_INCLUDE_DIR}" +cd "$(dirname "$0")/.." +mkdir -p build/config-save-test +test_root="$(mktemp -d "$PWD/build/config-save-test/run-XXXXXX")" + +clang++ -std=c++23 -I mods/src -I "$toml_include" \ + tests/config_save_test.cc mods/src/config_save.cc -o "$test_root/test" +"$test_root/test" "$test_root/ordinary" +clang++ -std=c++23 -I mods/src -I "$toml_include" \ + tests/config_save_failure_test.cc -o "$test_root/failure-test" +"$test_root/failure-test" "$test_root/failures"