Keep NAND moves internal and prevent destination overwrite races

This commit is contained in:
patchzyy
2026-09-07 22:50:13 +02:00
parent 58bfd32022
commit afb8f49866
7 changed files with 224 additions and 85 deletions
+27
View File
@@ -46,3 +46,30 @@ jobs:
- name: Test
run: dotnet test translator/Translator.sln -c Release --no-build --verbosity normal
nand:
name: NAND regression tests (${{ matrix.os }})
runs-on: ${{ matrix.os }}
timeout-minutes: 10
strategy:
fail-fast: false
matrix:
os: [windows-latest, ubuntu-latest, macos-14]
steps:
- uses: actions/checkout@v7
with:
persist-credentials: false
- name: Compile and run NAND tests
shell: bash
run: |
flags=(-std=c++17 -Wall -Wextra -Werror)
if [[ "$RUNNER_OS" != Windows ]]; then flags+=(-pthread); fi
clang++ "${flags[@]}" -Iruntime/src/hle/storage \
runtime/tests/nand_move_tests.cpp runtime/src/hle/storage/nand_file_ops.cpp \
-o "$RUNNER_TEMP/nand_move_tests.exe"
"$RUNNER_TEMP/nand_move_tests.exe"
for test in nand_save nand_settings; do
clang++ "${flags[@]}" -Iruntime/include "runtime/tests/${test}_tests.cpp" \
-o "$RUNNER_TEMP/${test}_tests.exe"
"$RUNNER_TEMP/${test}_tests.exe"
done
+6 -2
View File
@@ -282,8 +282,12 @@ target_include_directories(mkw_nand_save_tests PRIVATE "${CMAKE_CURRENT_LIST_DIR
target_compile_features(mkw_nand_save_tests PRIVATE cxx_std_17)
add_test(NAME mkw_nand_save_tests COMMAND mkw_nand_save_tests)
add_executable(mkw_nand_move_tests "${CMAKE_CURRENT_LIST_DIR}/tests/nand_move_tests.cpp")
target_include_directories(mkw_nand_move_tests PRIVATE "${CMAKE_CURRENT_LIST_DIR}/include")
add_executable(mkw_nand_move_tests
"${CMAKE_CURRENT_LIST_DIR}/tests/nand_move_tests.cpp"
"${CMAKE_CURRENT_LIST_DIR}/src/hle/storage/nand_file_ops.cpp")
find_package(Threads REQUIRED)
target_link_libraries(mkw_nand_move_tests PRIVATE Threads::Threads)
target_include_directories(mkw_nand_move_tests PRIVATE "${CMAKE_CURRENT_LIST_DIR}/src/hle/storage")
target_compile_features(mkw_nand_move_tests PRIVATE cxx_std_17)
add_test(NAME mkw_nand_move_tests COMMAND mkw_nand_move_tests)
-69
View File
@@ -1,69 +0,0 @@
#pragma once
#include <atomic>
#include <chrono>
#include <filesystem>
#include <string>
#include <system_error>
namespace RuntimeNandMove {
// NANDMove does not replace an existing entry. Riivolution save redirects can
// put the destination on a different filesystem from the guest's /tmp files.
inline void Move(const std::filesystem::path& source, const std::filesystem::path& destination,
std::error_code& ec) {
namespace fs = std::filesystem;
const auto status = fs::symlink_status(destination, ec);
if (ec && ec != std::errc::no_such_file_or_directory) return;
ec.clear();
if (fs::exists(status)) {
ec = std::make_error_code(std::errc::file_exists);
return;
}
fs::rename(source, destination, ec);
if (ec != std::errc::cross_device_link) return;
// Do not turn a directory move into a partially completed recursive copy,
// or follow a symlink and delete the link after copying its target.
const auto sourceStatus = fs::symlink_status(source, ec);
if (ec) return;
if (!fs::is_regular_file(sourceStatus)) {
ec = std::make_error_code(std::errc::cross_device_link);
return;
}
static std::atomic<unsigned long long> sequence{0};
const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count();
fs::path stagingDirectory;
bool created = false;
for (int attempt = 0; attempt < 64; ++attempt) {
stagingDirectory = destination.parent_path() /
(".nand-move-" + std::to_string(stamp) + "-" + std::to_string(sequence++));
created = fs::create_directory(stagingDirectory, ec);
if (created) break;
if (ec && ec != std::errc::file_exists) return;
}
if (!created) {
ec = std::make_error_code(std::errc::file_exists);
return;
}
const auto staged = stagingDirectory / "data";
fs::copy_file(source, staged, fs::copy_options::none, ec);
if (!ec) {
// Publication is a same-filesystem rename: readers never see a partial
// copy. Keep the source until the complete destination is in place.
const auto current = fs::symlink_status(destination, ec);
if (ec == std::errc::no_such_file_or_directory) ec.clear();
if (!ec && fs::exists(current)) ec = std::make_error_code(std::errc::file_exists);
if (!ec) fs::rename(staged, destination, ec);
if (!ec) fs::remove(source, ec);
}
// On failure the source remains available; after publication a failed
// source removal leaves both complete copies. Preserve the original error.
std::error_code cleanupError;
fs::remove(staged, cleanupError);
fs::remove(stagingDirectory, cleanupError);
}
} // namespace RuntimeNandMove
+2 -2
View File
@@ -3,7 +3,7 @@
// Shared state and helpers live in nand_internal.h.
#include "nand_internal.h"
#include "nand_move.h"
#include "nand_file_ops.h"
// ============================================================================
// Local helpers
@@ -379,7 +379,7 @@ extern "C" int32_t NANDMove_HLE(uint32_t srcPathPtr, uint32_t dstPathPtr) {
}
std::error_code ec;
RuntimeNandMove::Move(srcHost, dstHost, ec);
NandMove(srcHost, dstHost, ec);
if (!ec) {
return NAND_RESULT_OK;
}
+115
View File
@@ -0,0 +1,115 @@
#include "nand_file_ops.h"
#include <atomic>
#include <cerrno>
#include <chrono>
#include <filesystem>
#include <string>
#include <system_error>
#ifdef _WIN32
#include <windows.h>
#elif defined(__linux__)
#include <fcntl.h>
#include <linux/fs.h>
#include <sys/syscall.h>
#include <unistd.h>
#elif defined(__APPLE__)
#include <stdio.h>
#endif
namespace {
// Unlike std::filesystem::rename on POSIX, this cannot overwrite a destination
// created by another writer between checking it and publishing the move.
void RenameNoReplace(const std::filesystem::path& source,
const std::filesystem::path& destination, std::error_code& ec) {
#ifdef _WIN32
if (MoveFileExW(source.c_str(), destination.c_str(), 0)) ec.clear();
else ec = std::error_code(GetLastError(), std::system_category());
#else
#ifdef __linux__
const auto result = syscall(SYS_renameat2, AT_FDCWD, source.c_str(),
AT_FDCWD, destination.c_str(), RENAME_NOREPLACE);
#else
const auto result = renamex_np(source.c_str(), destination.c_str(), RENAME_EXCL);
#endif
if (result == 0) {
ec.clear();
return;
}
ec = std::error_code(errno, std::generic_category());
if (ec != std::errc::function_not_supported && ec != std::errc::invalid_argument &&
ec != std::errc::operation_not_supported) return;
// Older filesystems may lack exclusive rename. Linking also publishes
// without replacement; never fall back to an overwriting rename.
std::filesystem::create_hard_link(source, destination, ec);
if (!ec) std::filesystem::remove(source, ec);
#endif
}
struct StagedMove {
std::filesystem::path directory;
std::filesystem::path file;
~StagedMove() {
std::error_code ignored;
std::filesystem::remove(file, ignored);
std::filesystem::remove(directory, ignored);
}
};
} // namespace
// NANDMove does not replace an existing entry. Riivolution save redirects can
// put the destination on a different filesystem from the guest's /tmp files.
void NandMove(const std::filesystem::path& source, const std::filesystem::path& destination,
std::error_code& ec) {
namespace fs = std::filesystem;
const auto status = fs::symlink_status(destination, ec);
if (ec && ec != std::errc::no_such_file_or_directory) return;
ec.clear();
if (fs::exists(status)) {
ec = std::make_error_code(std::errc::file_exists);
return;
}
RenameNoReplace(source, destination, ec);
if (ec != std::errc::cross_device_link) return;
// Do not turn a directory move into a partially completed recursive copy,
// or follow a symlink and delete the link after copying its target.
const auto sourceStatus = fs::symlink_status(source, ec);
if (ec) return;
if (!fs::is_regular_file(sourceStatus)) {
ec = std::make_error_code(std::errc::cross_device_link);
return;
}
static std::atomic<unsigned long long> sequence{0};
const auto stamp = std::chrono::steady_clock::now().time_since_epoch().count();
fs::path stagingDirectory;
bool created = false;
for (int attempt = 0; attempt < 64; ++attempt) {
stagingDirectory = destination.parent_path() /
(".nand-move-" + std::to_string(stamp) + "-" + std::to_string(sequence++));
created = fs::create_directory(stagingDirectory, ec);
if (created) break;
if (ec && ec != std::errc::file_exists) return;
}
if (!created) {
ec = std::make_error_code(std::errc::file_exists);
return;
}
const StagedMove staging{stagingDirectory, stagingDirectory / "data"};
const auto& staged = staging.file;
fs::copy_file(source, staged, fs::copy_options::none, ec);
if (!ec) {
// Publication is a same-filesystem rename: readers never see a partial
// copy. Keep the source until the complete destination is in place.
RenameNoReplace(staged, destination, ec);
if (!ec) fs::remove(source, ec);
}
// A failed source removal leaves both complete copies and reports failure.
// Staging cleanup preserves ec, including on exceptions.
}
+10
View File
@@ -0,0 +1,10 @@
#pragma once
#include <filesystem>
#include <system_error>
// Move without replacing an existing entry. Cross-filesystem regular files are
// staged at the destination before removing the source. On failure the source
// remains; a failed source removal can leave both complete copies.
void NandMove(const std::filesystem::path& source, const std::filesystem::path& destination,
std::error_code& ec);
+64 -12
View File
@@ -1,6 +1,12 @@
#include "nand_move.h"
#include "nand_file_ops.h"
#include <atomic>
#include <chrono>
#include <fstream>
#include <functional>
#include <iterator>
#include <string>
#include <thread>
#include <iostream>
#include <stdexcept>
@@ -30,6 +36,40 @@ static std::string Read(const fs::path& path) {
return {std::istreambuf_iterator<char>(input), std::istreambuf_iterator<char>()};
}
// Competing moves must have exactly one winner, retain the losing source, and
// publish the winner's complete contents. Exercise both direct and staged moves.
static void CheckCompetingMoves(const fs::path& sourceRoot, const fs::path& destinationRoot) {
const auto left = sourceRoot / "race-left";
const auto right = sourceRoot / "race-right";
const auto target = destinationRoot / "race-target";
const std::string leftBytes(65536, 'L'), rightBytes(65536, 'R');
for (int attempt = 0; attempt < 128; ++attempt) {
Write(left, leftBytes);
Write(right, rightBytes);
std::atomic<int> ready{0};
std::error_code leftError, rightError;
auto move = [&](const fs::path& source, std::error_code& ec) {
++ready;
while (ready.load() != 2) std::this_thread::yield();
NandMove(source, target, ec);
};
std::thread first(move, std::cref(left), std::ref(leftError));
std::thread second(move, std::cref(right), std::ref(rightError));
first.join();
second.join();
Require(bool(leftError) != bool(rightError), "Competing moves must have exactly one winner");
const bool leftWon = !leftError;
Require((leftWon ? rightError : leftError) == std::errc::file_exists,
"Losing move must report an existing destination");
Require(Read(target) == (leftWon ? leftBytes : rightBytes), "Winner's contents were overwritten");
Require(!fs::exists(leftWon ? left : right), "Winning source was not removed");
Require(Read(leftWon ? right : left) == (leftWon ? rightBytes : leftBytes),
"Losing source must remain intact");
fs::remove(leftWon ? right : left);
fs::remove(target);
}
}
int main() {
const auto name = "wiicomp-nand-move-" +
std::to_string(std::chrono::steady_clock::now().time_since_epoch().count());
@@ -41,19 +81,29 @@ int main() {
const auto destination = root / "moved.bin";
std::error_code ec;
Write(source, "banner");
RuntimeNandMove::Move(source, destination, ec);
NandMove(source, destination, ec);
Require(!ec && !fs::exists(source) && Read(destination) == "banner", "Same-device move failed");
CheckCompetingMoves(root, root);
Write(source, "keep source");
RuntimeNandMove::Move(source, destination, ec);
NandMove(source, destination, ec);
Require(ec == std::errc::file_exists && Read(source) == "keep source" && Read(destination) == "banner",
"Existing destination must not be overwritten");
RuntimeNandMove::Move(root / "missing", root / "absent", ec);
NandMove(root / "missing", root / "absent", ec);
Require(bool(ec) && !fs::exists(root / "absent"), "Missing source must fail");
NandMove(source, root / "missing-parent/file", ec);
Require(bool(ec) && Read(source) == "keep source", "Missing parent must preserve source");
fs::create_directory(root / "directory");
Write(root / "directory/child", "child");
RuntimeNandMove::Move(root / "directory", root / "renamed-directory", ec);
NandMove(root / "directory", root / "renamed-directory", ec);
Require(!ec && Read(root / "renamed-directory/child") == "child", "Same-device directory move regressed");
#ifndef _WIN32
fs::create_symlink(root / "missing", root / "same-link");
NandMove(root / "same-link", root / "moved-link", ec);
Require(!ec && fs::is_symlink(fs::symlink_status(root / "moved-link")) &&
!fs::is_symlink(fs::symlink_status(root / "same-link")), "Same-device symlink move regressed");
#endif
#ifdef __linux__
// /dev/shm is a separate tmpfs on ordinary Linux systems, including CI
// and WSL. Fail rather than silently passing without exercising EXDEV.
@@ -62,30 +112,32 @@ int main() {
struct stat left{}, right{};
Require(::stat(root.c_str(), &left) == 0 && ::stat(other.c_str(), &right) == 0 && left.st_dev != right.st_dev,
"Cross-device test requires /tmp and /dev/shm on separate filesystems");
CheckCompetingMoves(root, other);
Require(fs::is_empty(other), "Competing moves left staging files behind");
const auto target = other / "banner.bin";
const std::string bytes = std::string(8192, '\0') + "banner payload";
Write(source, bytes);
fs::rename(source, target, ec);
Require(ec == std::errc::cross_device_link, "Fixture must reproduce the original EXDEV failure");
RuntimeNandMove::Move(source, target, ec);
NandMove(source, target, ec);
Require(!ec && !fs::exists(source) && Read(target) == bytes, "Cross-device move must preserve every byte");
Write(source, "do not overwrite");
RuntimeNandMove::Move(source, target, ec);
NandMove(source, target, ec);
Require(ec == std::errc::file_exists && Read(source) == "do not overwrite" && Read(target) == bytes,
"Cross-device move must preserve an existing destination");
fs::remove(target);
fs::create_symlink(other / "missing", target);
RuntimeNandMove::Move(source, target, ec);
NandMove(source, target, ec);
Require(ec == std::errc::file_exists && fs::is_symlink(target) && Read(source) == "do not overwrite",
"Dangling destination symlink must not be replaced");
fs::remove(target);
RuntimeNandMove::Move(root / "renamed-directory", other / "directory", ec);
NandMove(root / "renamed-directory", other / "directory", ec);
Require(ec == std::errc::cross_device_link && Read(root / "renamed-directory/child") == "child" &&
!fs::exists(other / "directory"), "Unsupported directory move must leave source intact");
fs::create_symlink(source, root / "link");
RuntimeNandMove::Move(root / "link", other / "link", ec);
NandMove(root / "link", other / "link", ec);
Require(ec == std::errc::cross_device_link && fs::is_symlink(root / "link") && !fs::exists(other / "link"),
"Cross-device source symlinks must not be dereferenced");
@@ -97,7 +149,7 @@ int main() {
limited.rlim_cur = 1024;
const auto oldHandler = std::signal(SIGXFSZ, SIG_IGN);
Require(setrlimit(RLIMIT_FSIZE, &limited) == 0, "Cannot set file-size limit");
RuntimeNandMove::Move(source, target, ec);
NandMove(source, target, ec);
const auto copyError = ec;
const auto restored = setrlimit(RLIMIT_FSIZE, &saved);
std::signal(SIGXFSZ, oldHandler);
@@ -107,7 +159,7 @@ int main() {
Require(geteuid() != 0, "Run permission tests as an unprivileged user");
fs::permissions(root, fs::perms::owner_read | fs::perms::owner_exec);
RuntimeNandMove::Move(source, target, ec);
NandMove(source, target, ec);
fs::permissions(root, fs::perms::owner_all);
Require(bool(ec) && Read(source) == bytes && Read(target) == bytes,
"Failed source deletion must leave both complete copies");