From c3aa4bce47783b439cc6f5711020fe8215fa5980 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 20:13:51 -0500 Subject: [PATCH 1/6] Check startup config output before replacing files --- docs/config-save.md | 33 ++++++++++++++ mods/src/config.cc | 20 ++++++--- mods/src/config_save.cc | 91 +++++++++++++++++++++++++++++++++++++++ mods/src/config_save.h | 9 ++++ tests/config_save_test.cc | 64 +++++++++++++++++++++++++++ tests/run-config-save.ps1 | 25 +++++++++++ 6 files changed, 236 insertions(+), 6 deletions(-) create mode 100644 docs/config-save.md create mode 100644 mods/src/config_save.cc create mode 100644 mods/src/config_save.h create mode 100644 tests/config_save_test.cc create mode 100644 tests/run-config-save.ps1 diff --git a/docs/config-save.md b/docs/config-save.md new file mode 100644 index 000000000..7b129ca6f --- /dev/null +++ b/docs/config-save.md @@ -0,0 +1,33 @@ +# 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 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. + +Windows uses `ReplaceFileW` to preserve existing permissions and streams, with a +temporary backup for its documented partial-failure cases. Initial creation uses +a non-replacing move. 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 symlinks are +resolved before staging. 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. + +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 25086345e..04f4db9f7 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() @@ -1419,7 +1419,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(""); @@ -1434,7 +1438,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..59240819b --- /dev/null +++ b/mods/src/config_save.cc @@ -0,0 +1,91 @@ +#include "config_save.h" + +#include +#include +#include +#include +#include +#include + +#if _WIN32 +#include +#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); + + // Follow existing symlinks as the former ofstream save did. A sibling stays on + // the same filesystem. Exclusive creation avoids truncating another save's file. + const auto destination = std::filesystem::weakly_canonical(path); + 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)); + + // C11 exclusive creation avoids depending on newer libc++ fstream runtime + // support on our minimum supported macOS version. +#if _WIN32 + std::FILE* file = nullptr; + _wfopen_s(&file, temporary.c_str(), L"wbx"); +#else + auto* file = std::fopen(temporary.c_str(), "wbx"); +#endif + if (!file) { + throw std::system_error(errno, std::generic_category(), "could not create temporary config file"); + } + + bool replacing = false; + try { + if (std::fwrite(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 = std::fclose(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) { + // Initial creation must not replace a config created in the meantime. + 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_test.cc b/tests/config_save_test.cc new file mode 100644 index 000000000..b25a3b5d5 --- /dev/null +++ b/tests/config_save_test.cc @@ -0,0 +1,64 @@ +#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); +#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..f41c44ffd --- /dev/null +++ b/tests/run-config-save.ps1 @@ -0,0 +1,25 @@ +[CmdletBinding()] +param([string]$TomlInclude) + +$ErrorActionPreference = 'Stop' +$repoRoot = Split-Path -Parent $PSScriptRoot +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 $fixtureRoot + if ($LASTEXITCODE -ne 0) { throw 'Config save regression failed.' } +} finally { + Pop-Location +} From 3ba601e19f05dde0d113fdd47210c19606b9c5d2 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 20:17:20 -0500 Subject: [PATCH 2/6] Exercise startup save failures and permission retention --- docs/config-save.md | 5 +-- mods/src/config_save.cc | 15 ++++++-- tests/config_save_failure_test.cc | 60 +++++++++++++++++++++++++++++++ tests/run-config-save.ps1 | 20 +++++++++++ 4 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 tests/config_save_failure_test.cc diff --git a/docs/config-save.md b/docs/config-save.md index 7b129ca6f..357160b0d 100644 --- a/docs/config-save.md +++ b/docs/config-save.md @@ -12,8 +12,9 @@ shutdown interception are installed. This is not the preserving TOML editor: whole-document saves do not merge concurrent setting changes or preserve comments. Windows uses `ReplaceFileW` to preserve existing permissions and streams, with a -temporary backup for its documented partial-failure cases. Initial creation uses -a non-replacing move. Ordinary failures clean up the temporary file; partial +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 diff --git a/mods/src/config_save.cc b/mods/src/config_save.cc index 59240819b..9e9a82b6d 100644 --- a/mods/src/config_save.cc +++ b/mods/src/config_save.cc @@ -11,6 +11,14 @@ #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++, @@ -43,10 +51,10 @@ void SaveConfigDocument(const toml::table& config, const std::filesystem::path& bool replacing = false; try { - if (std::fwrite(bytes.data(), 1, bytes.size(), file) != bytes.size()) { + 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 = std::fclose(file); // Includes flushing; failure prevents replacement. + 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"); @@ -59,7 +67,8 @@ void SaveConfigDocument(const toml::table& config, const std::filesystem::path& if (!ReplaceFileW(destination.c_str(), temporary.c_str(), backup.c_str(), 0, nullptr, nullptr)) { auto error = GetLastError(); if (error == ERROR_FILE_NOT_FOUND) { - // Initial creation must not replace a config created in the meantime. + // 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) { diff --git a/tests/config_save_failure_test.cc b/tests/config_save_failure_test.cc new file mode 100644 index 000000000..8bc083c3c --- /dev/null +++ b/tests/config_save_failure_test.cc @@ -0,0 +1,60 @@ +#include +#include + +static bool failClose = false; + +static std::size_t ShortWrite(const void* data, std::size_t size, std::size_t count, std::FILE* file) +{ + if (failClose) { + return std::fwrite(data, size, count, file); + } + const auto written = std::fwrite(data, size, count / 2, file); + errno = ENOSPC; + return written; +} + +static int FailedClose(std::FILE* file) +{ + std::fclose(file); + 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"; + const std::string original = "# keep this exactly\nenabled = false\n"; + { + std::ofstream out(path, std::ios::binary); + out << original; + } + 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/run-config-save.ps1 b/tests/run-config-save.ps1 index f41c44ffd..bdb7aa29b 100644 --- a/tests/run-config-save.ps1 +++ b/tests/run-config-save.ps1 @@ -20,6 +20,26 @@ try { $fixtureRoot = Join-Path $repoRoot ('build/config-save-test/' + [guid]::NewGuid()) & ./build/config-save-test/test.exe $fixtureRoot if ($LASTEXITCODE -ne 0) { throw 'Config save regression failed.' } + $testFile = Join-Path $fixtureRoot 'settings.toml' + $inherited = (Get-Acl -LiteralPath $testFile).Sddl + & ./build/config-save-test/test.exe $fixtureRoot + if ($LASTEXITCODE -ne 0 -or (Get-Acl -LiteralPath $testFile).Sddl -ne $inherited) { + throw 'Inherited ACL regression failed.' + } + $acl = Get-Acl -LiteralPath $testFile + $acl.SetAccessRuleProtection($true, $true) + Set-Acl -LiteralPath $testFile -AclObject $acl + $explicit = (Get-Acl -LiteralPath $testFile).Sddl + & ./build/config-save-test/test.exe $fixtureRoot + if ($LASTEXITCODE -ne 0 -or (Get-Acl -LiteralPath $testFile).Sddl -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 } From a8ed7bc77b96dcf77ea90fa3cf6678e9e96c06c8 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 20:19:38 -0500 Subject: [PATCH 3/6] Capture inherited permissions before the first save --- tests/run-config-save.ps1 | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/tests/run-config-save.ps1 b/tests/run-config-save.ps1 index bdb7aa29b..1fbcfada1 100644 --- a/tests/run-config-save.ps1 +++ b/tests/run-config-save.ps1 @@ -3,6 +3,18 @@ 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) { @@ -18,20 +30,24 @@ try { /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 $fixtureRoot - if ($LASTEXITCODE -ne 0) { throw 'Config save regression failed.' } + # Establish the baseline independently, before the first production save. + New-Item -ItemType Directory -Path $fixtureRoot | Out-Null $testFile = Join-Path $fixtureRoot 'settings.toml' - $inherited = (Get-Acl -LiteralPath $testFile).Sddl + 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-Acl -LiteralPath $testFile).Sddl -ne $inherited) { + 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-Acl -LiteralPath $testFile).Sddl + $explicit = Get-PermissionState $testFile & ./build/config-save-test/test.exe $fixtureRoot - if ($LASTEXITCODE -ne 0 -or (Get-Acl -LiteralPath $testFile).Sddl -ne $explicit) { + 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" ` From 4d1a476962fee61a0be2929ba7baef5d7a35adc0 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 20:20:05 -0500 Subject: [PATCH 4/6] Retain missing-file creation coverage alongside ACL fixtures --- tests/run-config-save.ps1 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/run-config-save.ps1 b/tests/run-config-save.ps1 index 1fbcfada1..6c80cd0e5 100644 --- a/tests/run-config-save.ps1 +++ b/tests/run-config-save.ps1 @@ -30,6 +30,9 @@ try { /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' From eb56fa0b430b5e010e28975664c7cf9b0398f384 Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Fri, 11 Sep 2026 20:28:02 -0500 Subject: [PATCH 5/6] Run config-save fixtures on native Windows and macOS CI --- .github/workflows/ci.yaml | 20 ++++++++++++++++++++ docs/config-save.md | 5 +++++ tests/config_save_test.cc | 10 ++++++++++ tests/run-config-save.sh | 15 +++++++++++++++ 4 files changed, 50 insertions(+) create mode 100644 tests/run-config-save.sh diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c4db85207..de14c8611 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -196,6 +196,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: | @@ -477,6 +487,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 index 357160b0d..2bc9367ec 100644 --- a/docs/config-save.md +++ b/docs/config-save.md @@ -29,6 +29,11 @@ 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; 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/tests/config_save_test.cc b/tests/config_save_test.cc index b25a3b5d5..c34c7c409 100644 --- a/tests/config_save_test.cc +++ b/tests/config_save_test.cc @@ -56,6 +56,16 @@ int main(int argc, char** argv) 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); #endif for (const auto& entry : std::filesystem::directory_iterator(root)) { assert(entry.path().filename().string().find(".tmp-") == std::string::npos); 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" From 25d78b535335ea1a83863d4220c2cb19ec4152db Mon Sep 17 00:00:00 2001 From: Guffawaffle Date: Thu, 1 Oct 2026 20:11:52 -0500 Subject: [PATCH 6/6] Protect staged configuration contents and follow dangling links --- docs/config-save.md | 13 +++++-- mods/src/config_save.cc | 59 +++++++++++++++++++++++++----- tests/config_save_failure_test.cc | 61 +++++++++++++++++++++++++++++-- tests/config_save_test.cc | 8 ++++ 4 files changed, 124 insertions(+), 17 deletions(-) diff --git a/docs/config-save.md b/docs/config-save.md index 2bc9367ec..608636154 100644 --- a/docs/config-save.md +++ b/docs/config-save.md @@ -6,11 +6,15 @@ 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 sibling temporary file, checks writing and closing, +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 @@ -18,8 +22,8 @@ exclusive create transaction. Ordinary failures clean up the temporary file; par 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 symlinks are -resolved before staging. Replacement requires directory permissions in addition +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 @@ -32,7 +36,8 @@ 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; it does not install test controls in the mod. +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) diff --git a/mods/src/config_save.cc b/mods/src/config_save.cc index 9e9a82b6d..70ef49c87 100644 --- a/mods/src/config_save.cc +++ b/mods/src/config_save.cc @@ -9,6 +9,13 @@ #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. @@ -29,24 +36,58 @@ void SaveConfigDocument(const toml::table& config, const std::filesystem::path& const auto bytes = output.str(); (void)toml::parse(bytes); - // Follow existing symlinks as the former ofstream save did. A sibling stays on - // the same filesystem. Exclusive creation avoids truncating another save's file. - const auto destination = std::filesystem::weakly_canonical(path); + // 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)); - // C11 exclusive creation avoids depending on newer libc++ fstream runtime - // support on our minimum supported macOS version. -#if _WIN32 + // 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; - _wfopen_s(&file, temporary.c_str(), L"wbx"); +#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 - auto* file = std::fopen(temporary.c_str(), "wbx"); + 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) { - throw std::system_error(errno, std::generic_category(), "could not create temporary config 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; diff --git a/tests/config_save_failure_test.cc b/tests/config_save_failure_test.cc index 8bc083c3c..aadc0ef50 100644 --- a/tests/config_save_failure_test.cc +++ b/tests/config_save_failure_test.cc @@ -1,14 +1,61 @@ #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) { - if (failClose) { - return std::fwrite(data, size, count, file); - } - const auto written = std::fwrite(data, size, count / 2, 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; } @@ -16,6 +63,7 @@ static std::size_t ShortWrite(const void* data, std::size_t size, std::size_t co static int FailedClose(std::FILE* file) { std::fclose(file); + CheckPrivateStaging(nullptr); errno = ENOSPC; return EOF; } @@ -35,11 +83,16 @@ int main(int argc, char** argv) 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; diff --git a/tests/config_save_test.cc b/tests/config_save_test.cc index c34c7c409..c4f713548 100644 --- a/tests/config_save_test.cc +++ b/tests/config_save_test.cc @@ -66,6 +66,14 @@ int main(int argc, char** argv) 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);