From dda6646c4983cac61ee45150451b7aae1c7df874 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 21:32:30 +0800 Subject: [PATCH] Keep Replacing a Read-Only Config and Include What the Helpers Need Everywhere The config was always replaced by a rename, never written in place, so the write helper's refusal of a read-only target, right for the presets it mirrors, stopped a read-only OrcaSlicer.conf from ever being saved again. The helper takes an explicit opt-in to replace such a file, and the two config save variants share one writer that uses it. The identity helper's stat headers were included only under the Linux guard, which the Linux-only build never noticed; they are included on every platform now. A temporary the longer name pushes past the path limit is written in place like one refused for permissions, the rename retry that waits for a Windows reader runs on Windows only, a failing stat in the sweep skips that entry rather than ending the sweep, and the rename fallback no longer stats the target it is about to remove. --- src/libslic3r/AppConfig.cpp | 48 +++++++++++++--------------------- src/libslic3r/AppConfig.hpp | 5 +++- src/libslic3r/Utils.hpp | 16 +++++++----- src/libslic3r/utils.cpp | 46 ++++++++++++++++++-------------- tests/libslic3r/test_utils.cpp | 19 ++++++++++++++ 5 files changed, 77 insertions(+), 57 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 7ae24fc7c5..2012a72aee 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -1120,28 +1120,9 @@ void AppConfig::save() j["local_machines"][local_machine.first] = m_json; } - std::string config_str = j.dump(1, '\t'); - // Everything above is assembly; only the writes need the other instances kept out. - InstanceLock instance_lock(lock_path()); -#ifdef WIN32 - // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API - // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, - // we save the config file into a backup first before moving it to the final destination. - // load() verifies the checksum over the text up to the closing brace, so it is taken before the newline. - const std::string md5_line = appconfig_md5_hash_line(config_str); - config_str += "\n"; - config_str += md5_line; - const std::string backup_path = (boost::format("%1%.bak") % path).str(); - if (const std::error_code ec = write_file_atomically(backup_path, config_str)) - BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message(); -#else - config_str += "\n"; -#endif - if (const std::error_code ec = write_file_atomically(path, config_str)) { - BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message(); - return; - } - m_dirty = false; + const std::string config_str = j.dump(1, '\t'); + if (write_config_file(path, config_str + "\n", config_str)) + m_dirty = false; } #else @@ -1327,25 +1308,32 @@ void AppConfig::save() // One empty line before the MD5 sum. config_ss << std::endl; - std::string config_str = config_ss.str(); - // Everything above is assembly; only the writes need the other instances kept out. + const std::string config_str = config_ss.str(); + if (write_config_file(path, config_str, config_str)) + m_dirty = false; +} +#endif + +bool AppConfig::write_config_file(const std::string &path, std::string body, const std::string &checksum_source) +{ + // Everything before this is assembly; only the writes need the other instances kept out. InstanceLock instance_lock(lock_path()); #ifdef WIN32 // WIN32 specific: the final replace is not safe in case of an application crash, there is no atomic "rename file" API // provided by Windows (sic!). Therefore we save a MD5 checksum to be able to verify file corruption. In addition, // we save the config file into a backup first before moving it to the final destination. - config_str += appconfig_md5_hash_line(config_str); + body += appconfig_md5_hash_line(checksum_source); const std::string backup_path = (boost::format("%1%.bak") % path).str(); - if (const std::error_code ec = write_file_atomically(backup_path, config_str)) + if (const std::error_code ec = write_file_atomically(backup_path, body, false, /*replace_read_only=*/true)) BOOST_LOG_TRIVIAL(error) << "Failed to write the backup configuration " << backup_path << ": " << ec.message(); #endif - if (const std::error_code ec = write_file_atomically(path, config_str)) { + // The config was always replaced, never written in place, so a read-only one is replaced still. + if (const std::error_code ec = write_file_atomically(path, body, false, /*replace_read_only=*/true)) { BOOST_LOG_TRIVIAL(error) << "Failed to write the configuration " << path << ": " << ec.message(); - return; + return false; } - m_dirty = false; + return true; } -#endif bool AppConfig::get_variant(const std::string &vendor, const std::string &model, const std::string &variant) const { diff --git a/src/libslic3r/AppConfig.hpp b/src/libslic3r/AppConfig.hpp index 1e6a211985..29e8353b4c 100644 --- a/src/libslic3r/AppConfig.hpp +++ b/src/libslic3r/AppConfig.hpp @@ -451,7 +451,10 @@ private: // Preset for each machine MachineSettingMap m_printer_settings; - // Has any value been modified since the config.ini has been last saved or loaded? + // Writes the assembled config text, and on Windows its checksum and a backup copy. `checksum_source` + // is the text load() will verify, which for the JSON config ends before the trailing newline. + bool write_config_file(const std::string &path, std::string body, const std::string &checksum_source); + bool m_dirty; // Original version found in the ini file before it was overwritten Semver m_orig_version; diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index 380190ac64..18414fc1bb 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -227,13 +227,15 @@ extern std::error_code rename_file(const std::string &from, const std::string &t // Write `chunks`, in order, to `path` through a temporary file beside it that is // then renamed over the target, so a concurrent reader sees the old or the new // file, never a partial one. The temporary is removed on failure and an existing -// target keeps its permissions; one without write permission is refused, as an -// in-place write would be. A target that is not a regular file (a symlink, -// device or pipe) is written in place, since replacing it would change what it -// is, and so is a target whose replace the filesystem refuses. -extern std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary = false); -inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false) - { return write_file_atomically(path, { std::string_view(content) }, binary); } +// target keeps its permissions. A target this process may not write is refused, +// as an in-place write would be, unless `replace_read_only` is set for a file +// that was always replaced rather than written, such as the app config. A +// target that is not a regular file (a symlink, device or pipe) is written in +// place, since replacing it would change what it is, and so is a target whose +// replace the filesystem refuses or beside which no temporary can be created. +extern std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary = false, bool replace_read_only = false); +inline std::error_code write_file_atomically(const std::string &path, const std::string &content, bool binary = false, bool replace_read_only = false) + { return write_file_atomically(path, { std::string_view(content) }, binary, replace_read_only); } // Remove the `...tmp` files a crashed write_file_atomically() left in // `dir` at least an hour ago, only names starting with `name_prefix` when it is // given. Meant for directories the application owns. Returns how many were removed. diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index baf04117a9..eb55ea46fa 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -14,6 +14,12 @@ #include #include #include +#ifdef _WIN32 +#include +#else +#include +#include +#endif #include #include @@ -718,8 +724,7 @@ std::error_code rename_file(const std::string &from, const std::string &to) // two-step route there, but not for an I/O error that would only lose the // target for nothing. const bool replace_refused = err == EPERM || err == EACCES || err == EEXIST || err == ENOTEMPTY || err == EBUSY || err == ENOTSUP || err == EOPNOTSUPP; - boost::system::error_code ec; - if (replace_refused && boost::filesystem::exists(to, ec) && boost::nowide::remove(to.c_str()) == 0) { + if (replace_refused && boost::nowide::remove(to.c_str()) == 0) { if (boost::nowide::rename(from.c_str(), to.c_str()) == 0) return {}; BOOST_LOG_TRIVIAL(error) << "Replacing " << to << " failed after the old file was removed: " << std::strerror(errno); @@ -750,7 +755,7 @@ static std::error_code write_whole_file(const std::string &path, std::initialize return std::make_error_code(errno != 0 ? static_cast(errno) : std::errc::io_error); } -std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary) +std::error_code write_file_atomically(const std::string &path, std::initializer_list chunks, bool binary, bool replace_read_only) { boost::system::error_code bec; const boost::filesystem::file_status target = boost::filesystem::symlink_status(path, bec); @@ -759,7 +764,7 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ return write_whole_file(path, chunks, binary); // Replacing needs only a writable directory, so a file this process may // not write has to be refused here, as the in-place write used to be. - if (target_exists && ! is_writable(path)) + if (target_exists && ! replace_read_only && ! is_writable(path)) return std::make_error_code(std::errc::permission_denied); // Unique per process and per call, so two threads writing one target @@ -768,8 +773,11 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ const std::string tmp_path = path + "." + std::to_string(get_current_pid()) + "." + std::to_string(counter++) + ".tmp"; if (std::error_code ec = write_whole_file(tmp_path, chunks, binary)) { boost::nowide::remove(tmp_path.c_str()); - // A directory that allows writing the file but not creating one beside it. - if (target_exists && ec == std::errc::permission_denied) { + // A directory that allows writing the file but not creating one beside + // it, or a name the longer temporary pushes past the path limit. + const bool only_the_temporary_failed = ec == std::errc::permission_denied || ec == std::errc::no_such_file_or_directory || + ec == std::errc::filename_too_long; + if (target_exists && only_the_temporary_failed) { BOOST_LOG_TRIVIAL(warning) << "Cannot create a temporary beside " << path << " (" << ec.message() << "); writing in place"; return write_whole_file(path, chunks, binary); } @@ -781,17 +789,17 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ if (target_exists) boost::filesystem::permissions(tmp_path, target.permissions(), bec); #endif + std::error_code ec = rename_file(tmp_path, path); +#ifdef _WIN32 // Windows refuses to replace a file another process holds open without // FILE_SHARE_DELETE, which is how the C runtime opens files for reading; a // reader is done in milliseconds, so wait it out before giving up on the - // atomic path. - std::error_code ec; - for (int attempt = 0; attempt < 20; ++ attempt) { - ec = rename_file(tmp_path, path); - if (ec != std::errc::permission_denied) - break; + // atomic path. Elsewhere a refused replace does not clear by waiting. + for (int attempt = 0; attempt < 20 && ec == std::errc::permission_denied; ++ attempt) { std::this_thread::sleep_for(std::chrono::milliseconds(25)); + ec = rename_file(tmp_path, path); } +#endif if (ec) { boost::nowide::remove(tmp_path.c_str()); // Still refused, or a mount that cannot replace a file in one step at @@ -852,18 +860,18 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st boost::system::error_code ec; for (boost::filesystem::directory_iterator it(dir, ec), end; ! ec && it != end; it.increment(ec)) { // The name test first: it is free, the stat is not. - if (! is_temp_name(it->path().filename().string()) || ! boost::filesystem::is_regular_file(it->symlink_status(ec))) + if (! is_temp_name(it->path().filename().string())) continue; - const std::time_t written = boost::filesystem::last_write_time(it->path(), ec); - if (ec || now - written < stale_age) { - ec.clear(); + boost::system::error_code entry_ec; + if (! boost::filesystem::is_regular_file(it->symlink_status(entry_ec))) continue; - } - if (boost::filesystem::remove(it->path(), ec)) { + const std::time_t written = boost::filesystem::last_write_time(it->path(), entry_ec); + if (entry_ec || now - written < stale_age) + continue; + if (boost::filesystem::remove(it->path(), entry_ec)) { BOOST_LOG_TRIVIAL(info) << "Removed stale temporary file " << it->path(); ++ removed; } - ec.clear(); } return removed; } diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index 23c42a944e..bd9d5afc49 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -111,6 +111,25 @@ TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouc REQUIRE(content == "pinned"); } +TEST_CASE("write_file_atomically replaces a read-only target when asked to", "[Utils]") { +#ifndef _WIN32 + if (::geteuid() == 0) + SKIP("a read-only file does not stop root"); +#endif + ScopedTemporaryDir dir; + const boost::filesystem::path target = dir.path() / "pinned.json"; + REQUIRE_FALSE(write_file_atomically(target.string(), "pinned")); + boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::group_read | boost::filesystem::others_read); + + const std::error_code ec = write_file_atomically(target.string(), "replaced", false, /*replace_read_only=*/true); + boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::owner_write | boost::filesystem::group_read | boost::filesystem::others_read); + + REQUIRE_FALSE(ec); + std::string content; + load_string_file(target, content); + REQUIRE(content == "replaced"); +} + TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[Utils]") { ScopedTemporaryDir dir; const boost::filesystem::path target = dir.path() / "blob.bin";