From 92618d25c387597b05e00de181552c0281712bc1 Mon Sep 17 00:00:00 2001 From: SandstromPL Date: Tue, 13 Oct 2026 00:48:06 +0530 Subject: [PATCH 1/2] fix(relinker): replace output files instead of rewriting them in place Co-Authored-By: Claude Opus 5.5 --- core/relinker/io/src/FileWriter.cpp | 67 ++++++++-- core/relinker/io/tests/FileWriterTests.cpp | 142 ++++++++++++++++++++- 2 files changed, 193 insertions(+), 16 deletions(-) diff --git a/core/relinker/io/src/FileWriter.cpp b/core/relinker/io/src/FileWriter.cpp index e80448f503..1760af8f97 100644 --- a/core/relinker/io/src/FileWriter.cpp +++ b/core/relinker/io/src/FileWriter.cpp @@ -1,28 +1,73 @@ #include #include #include +#include #include +#include namespace Io { -void FileWriter::Write(const std::string& path, const std::vector& data) { - std::ofstream f(NativePath(path), std::ios::binary); +namespace { + +template +bool writeFile(const std::filesystem::path& file, const std::string& path, const std::ios::openmode mode, const TFill& fill) { + std::ofstream f(file, mode); if (!f) - throw Domain::RelinkerException("Cannot open output file: " + path); - f.write(reinterpret_cast(data.data()), static_cast(data.size())); + return false; + fill(f); f.close(); if (!f) throw Domain::RelinkerException("Failed to write file: " + path); + return true; +} + +#ifndef _WIN32 +bool moveOver(const std::filesystem::path& temporary, const std::filesystem::path& target, const std::filesystem::file_status& previous) { + std::error_code error; + if (std::filesystem::is_regular_file(previous)) + std::filesystem::permissions(temporary, previous.permissions() & std::filesystem::perms::all, error); + if (!error) + std::filesystem::rename(temporary, target, error); + if (!error) + return true; + std::filesystem::remove(temporary, error); + return false; +} +#endif + +template +void replaceFile(const std::string& path, const std::ios::openmode mode, const TFill& fill) { + const auto target = NativePath(path); +#ifndef _WIN32 + std::error_code error; + const auto previous = std::filesystem::symlink_status(target, error); + auto temporary = target; + temporary += ".tmp"; + if ((std::filesystem::is_regular_file(previous) || previous.type() == std::filesystem::file_type::not_found) + && std::filesystem::symlink_status(temporary, error).type() == std::filesystem::file_type::not_found) { + try { + if (writeFile(temporary, path, mode, fill) && moveOver(temporary, target, previous)) + return; + } catch (...) { + std::filesystem::remove(temporary, error); + throw; + } + } +#endif + if (!writeFile(target, path, mode, fill)) + throw Domain::RelinkerException("Cannot open output file: " + path); +} + +} + +void FileWriter::Write(const std::string& path, const std::vector& data) { + replaceFile(path, std::ios::binary, [&data](std::ofstream& f) { + f.write(reinterpret_cast(data.data()), static_cast(data.size())); + }); } void FileWriter::Write(const std::string& path, const std::string& content) { - std::ofstream f(NativePath(path)); - if (!f) - throw Domain::RelinkerException("Cannot open output file: " + path); - f << content; - f.close(); - if (!f) - throw Domain::RelinkerException("Failed to write file: " + path); + replaceFile(path, std::ios::out, [&content](std::ofstream& f) { f << content; }); } } diff --git a/core/relinker/io/tests/FileWriterTests.cpp b/core/relinker/io/tests/FileWriterTests.cpp index 8edc710d55..3ca7427598 100644 --- a/core/relinker/io/tests/FileWriterTests.cpp +++ b/core/relinker/io/tests/FileWriterTests.cpp @@ -1,9 +1,18 @@ #include #include +#include +#include +#include #include +#include #include #include +#include #include +#include +#include +#include +#include namespace { @@ -21,6 +30,124 @@ void requireWriteFailure(const TValue& data) { throw std::runtime_error("Write to /dev/full reported success"); } +std::string readFile(const std::filesystem::path& path) { + std::ifstream file(path, std::ios::binary); + return {std::istreambuf_iterator(file), std::istreambuf_iterator()}; +} + +template +void requireRunningReplacement(const std::filesystem::path& directory, const TValue& data) { + const auto output = directory / "running"; + std::filesystem::copy_file("/bin/sleep", output); + const auto permissions = std::filesystem::perms::owner_all | std::filesystem::perms::group_read | std::filesystem::perms::group_exec; + std::filesystem::permissions(output, permissions); + const auto previous = readFile(output); + const pid_t child = fork(); + if (child < 0) + throw std::runtime_error("Cannot start the previous executable"); + if (child == 0) { + execl(output.c_str(), "sleep", "30", nullptr); + _exit(127); + } + std::error_code error; + bool started = false; + for (int attempt = 0; attempt < 2000 && !started; ++attempt) { + started = std::filesystem::read_symlink("/proc/" + std::to_string(child) + "/exe", error) == output; + if (!started) + usleep(1000); + } + const int descriptor = open(output.c_str(), O_RDONLY); + std::string failure; + if (started) { + try { + Io::FileWriter().Write(output.string(), data); + } catch (const std::exception& e) { + failure = e.what(); + } + } + std::string kept(previous.size(), '\0'); + const bool keptPrevious = pread(descriptor, kept.data(), kept.size(), 0) == static_cast(kept.size()) && kept == previous; + close(descriptor); + const bool running = waitpid(child, nullptr, WNOHANG) == 0; + if (running) { + kill(child, SIGKILL); + waitpid(child, nullptr, 0); + } + if (!started) + throw std::runtime_error("The previous executable did not start"); + if (!failure.empty()) + throw std::runtime_error("Replacing a running executable failed: " + failure); + if (readFile(output) != std::string(data.begin(), data.end())) + throw std::runtime_error("Running executable was not replaced"); + if ((std::filesystem::status(output).permissions() & std::filesystem::perms::all) != permissions) + throw std::runtime_error("Replacement did not keep the previous permissions"); + if (!keptPrevious) + throw std::runtime_error("An open descriptor no longer reads the previous file"); + if (!running) + throw std::runtime_error("Running executable stopped"); + if (std::filesystem::exists(directory / "running.tmp")) + throw std::runtime_error("Temporary file left behind"); +} + +template +void requireFailedReplacement(const std::filesystem::path& directory, const TValue& data) { + const auto output = directory / "limited"; + std::ofstream(output, std::ios::binary) << "previous"; + rlimit limit{}; + getrlimit(RLIMIT_FSIZE, &limit); + rlimit lowered = limit; + lowered.rlim_cur = 1; + const auto handler = std::signal(SIGXFSZ, SIG_IGN); + setrlimit(RLIMIT_FSIZE, &lowered); + std::string failure; + try { + Io::FileWriter().Write(output.string(), data); + } catch (const Domain::RelinkerException& e) { + failure = e.what(); + } + setrlimit(RLIMIT_FSIZE, &limit); + std::signal(SIGXFSZ, handler); + if (failure != "Failed to write file: " + output.string()) + throw std::runtime_error("Write past the file size limit did not fail: " + failure); + if (readFile(output) != "previous" || std::filesystem::exists(directory / "limited.tmp")) + throw std::runtime_error("Failed write did not keep the previous file"); +} + +template +void requireInPlaceWrites(const std::filesystem::path& directory, const TValue& data) { + const std::string expected(data.begin(), data.end()); + const auto target = directory / "target"; + const auto link = directory / "link"; + std::ofstream(target, std::ios::binary) << "previous"; + std::filesystem::create_symlink(target, link); + Io::FileWriter().Write(link.string(), data); + if (!std::filesystem::is_symlink(link) || readFile(target) != expected) + throw std::runtime_error("Symbolic link output was not written through"); + const auto reserved = directory / "reserved"; + std::ofstream(reserved, std::ios::binary) << "previous"; + std::ofstream(directory / "reserved.tmp", std::ios::binary) << "unrelated"; + Io::FileWriter().Write(reserved.string(), data); + if (readFile(reserved) != expected || readFile(directory / "reserved.tmp") != "unrelated") + throw std::runtime_error("Existing temporary file name was not left alone"); +} + +template +void requireReplacement(const TValue& data) { + const auto directory = std::filesystem::canonical(std::filesystem::temp_directory_path()) / ("anyps5-file-writer-" + std::to_string(getpid())); + std::error_code error; + std::filesystem::remove_all(directory, error); + std::filesystem::create_directory(directory); + try { + requireRunningReplacement(directory, data); + requireFailedReplacement(directory, data); + requireInPlaceWrites(directory, data); + } catch (...) { + std::filesystem::remove_all(directory, error); + throw; + } + std::filesystem::remove_all(directory, error); +} + } int main(int argc, char* argv[]) { @@ -28,12 +155,17 @@ int main(int argc, char* argv[]) { if (argc != 2) throw std::runtime_error("Expected binary or text"); const std::string mode = argv[1]; for (const std::size_t size : {std::size_t{3}, std::size_t{65536}}) { - if (mode == "binary") - requireWriteFailure(std::vector(size, 0x41)); - else if (mode == "text") - requireWriteFailure(std::string(size, 'A')); - else + if (mode == "binary") { + const std::vector data(size, 0x41); + requireWriteFailure(data); + requireReplacement(data); + } else if (mode == "text") { + const std::string data(size, 'A'); + requireWriteFailure(data); + requireReplacement(data); + } else { throw std::runtime_error("Unknown test mode: " + mode); + } } } catch (const std::exception& e) { std::cerr << e.what() << '\n'; From 26c62cc63d404d81392f9f4ab004d3a6cd29fe2e Mon Sep 17 00:00:00 2001 From: SandstromPL Date: Wed, 14 Oct 2026 07:27:54 +0530 Subject: [PATCH 2/2] fix(relinker): give the output's temporary file a unique name A relink killed between writing .tmp and renaming it left that file behind, and every later relink of the output then fell back to the in-place write, bringing back the failure on a running build. The temporary file is now created under a new name beside the output with O_CREAT | O_EXCL and the previous permission bits, and written through that descriptor, so a file left by an earlier run never blocks a replacement and is never touched. Co-Authored-By: Claude Opus 5.5 --- core/relinker/io/src/FileWriter.cpp | 95 +++++++++++++++------- core/relinker/io/tests/FileWriterTests.cpp | 30 +++++-- 2 files changed, 90 insertions(+), 35 deletions(-) diff --git a/core/relinker/io/src/FileWriter.cpp b/core/relinker/io/src/FileWriter.cpp index 1760af8f97..49df133411 100644 --- a/core/relinker/io/src/FileWriter.cpp +++ b/core/relinker/io/src/FileWriter.cpp @@ -4,17 +4,24 @@ #include #include #include +#ifndef _WIN32 +#include +#include +#include +#include +#include +#include +#endif namespace Io { namespace { -template -bool writeFile(const std::filesystem::path& file, const std::string& path, const std::ios::openmode mode, const TFill& fill) { +bool writeFile(const std::filesystem::path& file, const std::string& path, const std::ios::openmode mode, const char* data, const std::size_t size) { std::ofstream f(file, mode); if (!f) return false; - fill(f); + f.write(data, static_cast(size)); f.close(); if (!f) throw Domain::RelinkerException("Failed to write file: " + path); @@ -22,12 +29,57 @@ bool writeFile(const std::filesystem::path& file, const std::string& path, const } #ifndef _WIN32 -bool moveOver(const std::filesystem::path& temporary, const std::filesystem::path& target, const std::filesystem::file_status& previous) { +int createTemporary(const std::filesystem::path& target, const mode_t mode, std::filesystem::path& temporary) { + constexpr std::string_view characters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"; + std::random_device device; + for (int attempt = 0; attempt < 100; ++attempt) { + std::string suffix = "."; + for (int index = 0; index < 6; ++index) + suffix += characters[device() % characters.size()]; + temporary = target; + temporary += suffix; + const int descriptor = open(temporary.c_str(), O_WRONLY | O_CREAT | O_EXCL | O_CLOEXEC, mode); + if (descriptor >= 0 || errno != EEXIST) + return descriptor; + } + return -1; +} + +bool writeAll(const int descriptor, const char* data, std::size_t size) { + while (size != 0) { + const auto written = write(descriptor, data, size); + if (written < 0 && errno == EINTR) + continue; + if (written <= 0) + return false; + data += written; + size -= static_cast(written); + } + return true; +} + +bool replaceFile(const std::filesystem::path& target, const std::string& path, const char* data, const std::size_t size) { std::error_code error; - if (std::filesystem::is_regular_file(previous)) - std::filesystem::permissions(temporary, previous.permissions() & std::filesystem::perms::all, error); - if (!error) - std::filesystem::rename(temporary, target, error); + const auto previous = std::filesystem::symlink_status(target, error); + const bool regular = std::filesystem::is_regular_file(previous); + if (!regular && previous.type() != std::filesystem::file_type::not_found) + return false; + const auto mode = regular ? static_cast(previous.permissions() & std::filesystem::perms::all) : static_cast(0666); + std::filesystem::path temporary; + const int descriptor = createTemporary(target, mode, temporary); + if (descriptor < 0) + return false; + if (regular && fchmod(descriptor, mode) != 0) { + close(descriptor); + std::filesystem::remove(temporary, error); + return false; + } + const bool written = writeAll(descriptor, data, size); + if (close(descriptor) != 0 || !written) { + std::filesystem::remove(temporary, error); + throw Domain::RelinkerException("Failed to write file: " + path); + } + std::filesystem::rename(temporary, target, error); if (!error) return true; std::filesystem::remove(temporary, error); @@ -35,39 +87,24 @@ bool moveOver(const std::filesystem::path& temporary, const std::filesystem::pat } #endif -template -void replaceFile(const std::string& path, const std::ios::openmode mode, const TFill& fill) { +void writeOutput(const std::string& path, const std::ios::openmode mode, const char* data, const std::size_t size) { const auto target = NativePath(path); #ifndef _WIN32 - std::error_code error; - const auto previous = std::filesystem::symlink_status(target, error); - auto temporary = target; - temporary += ".tmp"; - if ((std::filesystem::is_regular_file(previous) || previous.type() == std::filesystem::file_type::not_found) - && std::filesystem::symlink_status(temporary, error).type() == std::filesystem::file_type::not_found) { - try { - if (writeFile(temporary, path, mode, fill) && moveOver(temporary, target, previous)) - return; - } catch (...) { - std::filesystem::remove(temporary, error); - throw; - } - } + if (replaceFile(target, path, data, size)) + return; #endif - if (!writeFile(target, path, mode, fill)) + if (!writeFile(target, path, mode, data, size)) throw Domain::RelinkerException("Cannot open output file: " + path); } } void FileWriter::Write(const std::string& path, const std::vector& data) { - replaceFile(path, std::ios::binary, [&data](std::ofstream& f) { - f.write(reinterpret_cast(data.data()), static_cast(data.size())); - }); + writeOutput(path, std::ios::binary, reinterpret_cast(data.data()), data.size()); } void FileWriter::Write(const std::string& path, const std::string& content) { - replaceFile(path, std::ios::out, [&content](std::ofstream& f) { f << content; }); + writeOutput(path, std::ios::out, content.data(), content.size()); } } diff --git a/core/relinker/io/tests/FileWriterTests.cpp b/core/relinker/io/tests/FileWriterTests.cpp index 3ca7427598..900518b16f 100644 --- a/core/relinker/io/tests/FileWriterTests.cpp +++ b/core/relinker/io/tests/FileWriterTests.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -35,11 +36,23 @@ std::string readFile(const std::filesystem::path& path) { return {std::istreambuf_iterator(file), std::istreambuf_iterator()}; } +std::vector siblings(const std::filesystem::path& output) { + std::vector names; + for (const auto& entry : std::filesystem::directory_iterator(output.parent_path())) { + const auto name = entry.path().filename().string(); + if (name.starts_with(output.filename().string() + ".")) + names.push_back(name); + } + return names; +} + template -void requireRunningReplacement(const std::filesystem::path& directory, const TValue& data) { - const auto output = directory / "running"; +void requireRunningReplacement(const std::filesystem::path& directory, const TValue& data, const bool leftover) { + const auto output = directory / (leftover ? "leftover" : "running"); + if (leftover) + std::ofstream(directory / "leftover.tmp", std::ios::binary) << "killed"; std::filesystem::copy_file("/bin/sleep", output); - const auto permissions = std::filesystem::perms::owner_all | std::filesystem::perms::group_read | std::filesystem::perms::group_exec; + const auto permissions = std::filesystem::perms::owner_all | std::filesystem::perms::group_all; std::filesystem::permissions(output, permissions); const auto previous = readFile(output); const pid_t child = fork(); @@ -59,11 +72,13 @@ void requireRunningReplacement(const std::filesystem::path& directory, const TVa const int descriptor = open(output.c_str(), O_RDONLY); std::string failure; if (started) { + const auto mask = umask(077); try { Io::FileWriter().Write(output.string(), data); } catch (const std::exception& e) { failure = e.what(); } + umask(mask); } std::string kept(previous.size(), '\0'); const bool keptPrevious = pread(descriptor, kept.data(), kept.size(), 0) == static_cast(kept.size()) && kept == previous; @@ -85,8 +100,10 @@ void requireRunningReplacement(const std::filesystem::path& directory, const TVa throw std::runtime_error("An open descriptor no longer reads the previous file"); if (!running) throw std::runtime_error("Running executable stopped"); - if (std::filesystem::exists(directory / "running.tmp")) + if (siblings(output) != (leftover ? std::vector{"leftover.tmp"} : std::vector{})) throw std::runtime_error("Temporary file left behind"); + if (leftover && readFile(directory / "leftover.tmp") != "killed") + throw std::runtime_error("A file left by an earlier run was changed"); } template @@ -109,7 +126,7 @@ void requireFailedReplacement(const std::filesystem::path& directory, const TVal std::signal(SIGXFSZ, handler); if (failure != "Failed to write file: " + output.string()) throw std::runtime_error("Write past the file size limit did not fail: " + failure); - if (readFile(output) != "previous" || std::filesystem::exists(directory / "limited.tmp")) + if (readFile(output) != "previous" || !siblings(output).empty()) throw std::runtime_error("Failed write did not keep the previous file"); } @@ -138,7 +155,8 @@ void requireReplacement(const TValue& data) { std::filesystem::remove_all(directory, error); std::filesystem::create_directory(directory); try { - requireRunningReplacement(directory, data); + requireRunningReplacement(directory, data, false); + requireRunningReplacement(directory, data, true); requireFailedReplacement(directory, data); requireInPlaceWrites(directory, data); } catch (...) {