From 5490320b8b26759f218a383d588523f7e7ec0ddb Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Thu, 24 Sep 2026 20:18:20 +0800 Subject: [PATCH] Never Mistake a Numbered Backup for a Temporary and Check Real Write Access The sweep's prefix form counted any . file as a leftover of the old config writer, so a user's OrcaSlicer.conf.1 backup went at the next start. Only the ...tmp form is a temporary now, and the name is tested before the entry is stat'ed. The read-only check went by mode bits, which misses a deny-write ACL on Windows and refuses a root-owned file root can write; it asks the OS with access() instead. The two-step rename fallback runs only for the errors a refused replace produces, never for an I/O error that would only lose the target, and says so if the second step fails after the old file is gone. The orphaned-.info scan on the sync thread reads each .info under the lock, so a remove_files() in another instance is seen whole or not at all, and queues the cloud delete after the lock is released, keeping the queue mutex outside it. AppConfig::save() assembles its text before it takes the lock, like Preset::save(). The two bundle loops share one metadata loader, and the two-thread write test counts leftover temporaries rather than every directory entry, since a scanner on Windows may hold the old file under another name for a moment. --- src/libslic3r/AppConfig.cpp | 6 +++-- src/libslic3r/PresetBundle.cpp | 25 ++++++++---------- src/libslic3r/Utils.hpp | 6 ++--- src/libslic3r/utils.cpp | 47 +++++++++++++++++++++------------- src/slic3r/GUI/GUI_App.cpp | 22 +++++++++------- tests/libslic3r/test_utils.cpp | 19 +++++++------- 6 files changed, 69 insertions(+), 56 deletions(-) diff --git a/src/libslic3r/AppConfig.cpp b/src/libslic3r/AppConfig.cpp index 8f7c69375c..7ae24fc7c5 100644 --- a/src/libslic3r/AppConfig.cpp +++ b/src/libslic3r/AppConfig.cpp @@ -991,7 +991,6 @@ void AppConfig::save() // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); - InstanceLock instance_lock(lock_path()); json j; @@ -1122,6 +1121,8 @@ 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, @@ -1291,7 +1292,6 @@ void AppConfig::save() // The config is first written to a file with a PID suffix and then moved // to avoid race conditions with multiple instances of Slic3r const auto path = config_path(); - InstanceLock instance_lock(lock_path()); std::stringstream config_ss; if (m_mode == EAppMode::Editor) @@ -1328,6 +1328,8 @@ void AppConfig::save() config_ss << std::endl; std::string config_str = config_ss.str(); + // 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, diff --git a/src/libslic3r/PresetBundle.cpp b/src/libslic3r/PresetBundle.cpp index 46459e96d1..f66a64ad27 100644 --- a/src/libslic3r/PresetBundle.cpp +++ b/src/libslic3r/PresetBundle.cpp @@ -1228,6 +1228,15 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For const auto user_load_t0 = std::chrono::steady_clock::now(); + // Reads one bundle's metadata under the lock, per file, so the lock is never + // held when bundles.WriteLock() is taken afterwards. + auto load_bundle_metadata = [read_only](const fs::path &bundle_dir, const fs::path &metadata_file, BundleMetadata &metadata) { + InstanceLock instance_lock(user_presets_lock_path(read_only)); + if (instance_lock.locked()) + remove_stale_temp_files(bundle_dir); + return metadata.load_from_json(metadata_file.string()); + }; + // Load bundle metadata from _local directory first fs::path local_dir(folder / PRESET_LOCAL_DIR); if (fs::exists(local_dir)) { @@ -1241,13 +1250,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For if (!fs::exists(metadata_file)) continue; BundleMetadata metadata; - { - // Per file, so the lock is never held when bundles.WriteLock() is taken below. - InstanceLock instance_lock(user_presets_lock_path(read_only)); - if (instance_lock.locked()) - remove_stale_temp_files(entry.path()); - if (!metadata.load_from_json(metadata_file.string())) continue; - } + if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); @@ -1282,13 +1285,7 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For if (!fs::exists(metadata_file)) continue; BundleMetadata metadata; - { - // Per file, so the lock is never held when bundles.WriteLock() is taken below. - InstanceLock instance_lock(user_presets_lock_path(read_only)); - if (instance_lock.locked()) - remove_stale_temp_files(entry.path()); - if (!metadata.load_from_json(metadata_file.string())) continue; - } + if (!load_bundle_metadata(entry.path(), metadata_file, metadata)) continue; metadata.print_presets.clear(); metadata.filament_presets.clear(); metadata.printer_presets.clear(); diff --git a/src/libslic3r/Utils.hpp b/src/libslic3r/Utils.hpp index cc56f0624c..9de742dc14 100644 --- a/src/libslic3r/Utils.hpp +++ b/src/libslic3r/Utils.hpp @@ -235,10 +235,8 @@ extern std::error_code write_file_atomically(const std::string &path, std::initi 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); } // Remove the `...tmp` files a crashed write_file_atomically() left in -// `dir` at least ten minutes ago. With `name_prefix`, only names starting with it -// go, and so do `.` leftovers of the older AppConfig writer, so -// the prefix form is for directories the application owns, never a user's export -// folder. Returns how many were removed. +// `dir` at least ten minutes ago, only names starting with `name_prefix` when it is +// given. Meant for directories the application owns. Returns how many were removed. extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string()); enum CopyFileResult { diff --git a/src/libslic3r/utils.cpp b/src/libslic3r/utils.cpp index dcae3b1330..5f3cf6680c 100644 --- a/src/libslic3r/utils.cpp +++ b/src/libslic3r/utils.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -711,16 +712,30 @@ std::error_code rename_file(const std::string &from, const std::string &to) return {}; const int err = errno; // Some mounts (sshfs, gvfs, MTP and a few SMB setups) refuse to replace an - // existing target in one step, with whichever error they see fit; take the - // old two-step route whenever the target is still there to be replaced. + // existing target in one step and report it as one of these; take the old + // 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 (err != ENOENT && err != EXDEV && boost::filesystem::exists(to, ec) && boost::nowide::remove(to.c_str()) == 0 && - boost::nowide::rename(from.c_str(), to.c_str()) == 0) - return {}; + if (replace_refused && boost::filesystem::exists(to, ec) && 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); + } return std::make_error_code(static_cast(err)); #endif } +// Whether this process may open `path` for writing. +static bool is_writable(const std::string &path) +{ +#ifdef _WIN32 + return ::_waccess(boost::nowide::widen(path).c_str(), 2) == 0; +#else + return ::access(path.c_str(), W_OK) == 0; +#endif +} + static std::error_code write_whole_file(const std::string &path, std::initializer_list chunks, bool binary) { errno = 0; @@ -740,9 +755,9 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ const bool target_exists = ! bec && boost::filesystem::exists(target); if (target_exists && ! boost::filesystem::is_regular_file(target)) return write_whole_file(path, chunks, binary); - // Replacing needs only a writable directory, so a file the user made - // read-only has to be honoured here, as the in-place write used to. - if (target_exists && (target.permissions() & (boost::filesystem::owner_write | boost::filesystem::group_write | boost::filesystem::others_write)) == boost::filesystem::no_perms) + // 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)) return std::make_error_code(std::errc::permission_denied); // Unique per process and per call, so two threads writing one target @@ -781,22 +796,17 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix) { - auto all_digits = [](std::string::const_iterator begin, std::string::const_iterator end) { - return begin != end && std::all_of(begin, end, [](char c) { return c >= '0' && c <= '9'; }); - }; - // ....tmp, or the older . when a prefix is given. - auto is_temp_name = [&](const std::string &name) { + // ....tmp + auto is_temp_name = [&name_prefix](const std::string &name) { if (name.compare(0, name_prefix.size(), name_prefix) != 0) return false; - if (! name_prefix.empty() && name.size() > name_prefix.size() + 1 && name[name_prefix.size()] == '.' && - all_digits(name.begin() + name_prefix.size() + 1, name.end())) - return true; static const std::string suffix = ".tmp"; if (name.size() <= suffix.size() || name.compare(name.size() - suffix.size(), suffix.size(), suffix) != 0) return false; const size_t digits_end = name.size() - suffix.size(); const size_t dot = name.rfind('.', digits_end - 1); - return dot != std::string::npos && dot != 0 && all_digits(name.begin() + dot + 1, name.begin() + digits_end); + return dot != std::string::npos && dot != 0 && dot + 1 != digits_end && + std::all_of(name.begin() + dot + 1, name.begin() + digits_end, [](char c) { return c >= '0' && c <= '9'; }); }; // An instance that gave up waiting for the lock writes unlocked by design, // so a temporary this young may still be in flight; a crash leftover is old. @@ -805,7 +815,8 @@ size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::st size_t removed = 0; boost::system::error_code ec; for (boost::filesystem::directory_iterator it(dir, ec), end; ! ec && it != end; it.increment(ec)) { - if (! boost::filesystem::is_regular_file(it->symlink_status(ec)) || ! is_temp_name(it->path().filename().string())) + // 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))) continue; const std::time_t written = boost::filesystem::last_write_time(it->path(), ec); if (ec || now - written < stale_age) { diff --git a/src/slic3r/GUI/GUI_App.cpp b/src/slic3r/GUI/GUI_App.cpp index 2df8dca3b4..7a91487acf 100644 --- a/src/slic3r/GUI/GUI_App.cpp +++ b/src/slic3r/GUI/GUI_App.cpp @@ -8956,15 +8956,19 @@ void GUI_App::scan_orphaned_info_files() fs::path preset_file = info_file; preset_file.replace_extension(".json"); - // If .json doesn't exist, .info is orphaned - if (!fs::exists(preset_file)) { - // Extract setting_id from .info file - std::string setting_id = extract_setting_id_from_info(info_file.string()); - if (!setting_id.empty()) { - // Add to need_delete_presets - delete_preset_from_cloud(setting_id, info_file.string()); - BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string(); - } + // If .json doesn't exist, .info is orphaned. Read under the lock, so a + // remove_files() in another instance is seen whole or not at all; the + // delete queue's own mutex is taken after the lock is released. + std::string setting_id; + { + InstanceLock instance_lock(user_presets_lock_path()); + if (!fs::exists(preset_file)) + setting_id = extract_setting_id_from_info(info_file.string()); + } + if (!setting_id.empty()) { + // Add to need_delete_presets + delete_preset_from_cloud(setting_id, info_file.string()); + BOOST_LOG_TRIVIAL(info) << "Found orphaned .info file on startup: " << info_file.string(); } } if (ec) diff --git a/tests/libslic3r/test_utils.cpp b/tests/libslic3r/test_utils.cpp index c60c955d33..df10b2985b 100644 --- a/tests/libslic3r/test_utils.cpp +++ b/tests/libslic3r/test_utils.cpp @@ -155,12 +155,13 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[Uti load_string_file(target, content); const bool whole = content == a || content == b; REQUIRE(whole); - size_t entries = 0; - for (auto &entry : boost::filesystem::directory_iterator(dir.path())) { - (void) entry; - ++entries; - } - REQUIRE(entries == 1); + // No temporary may be left; a scanner on Windows may briefly hold the old + // file under another name, so only the temporaries are counted. + size_t temporaries = 0; + for (auto &entry : boost::filesystem::directory_iterator(dir.path())) + if (entry.path().extension() == ".tmp") + ++temporaries; + REQUIRE(temporaries == 0); } TEST_CASE("remove_stale_temp_files removes only old ..tmp files", "[Utils]") { @@ -173,11 +174,11 @@ TEST_CASE("remove_stale_temp_files removes only old ..tmp files", "[U // Just written: possibly another instance's in-flight save, so it stays. REQUIRE_FALSE(write_file_atomically((dir.path() / "g.json.7.tmp").string(), "x")); - SECTION("with a name prefix only matching names go, including the older . form") { - REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 3); + SECTION("with a name prefix only matching names go, and a numbered backup is not a temporary") { + REQUIRE(remove_stale_temp_files(dir.path(), "a.json") == 2); REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.123.tmp")); - REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.99")); REQUIRE_FALSE(boost::filesystem::exists(dir.path() / "a.json.12.3.tmp")); + REQUIRE(boost::filesystem::exists(dir.path() / "a.json.99")); REQUIRE(boost::filesystem::exists(dir.path() / "b.info.4.tmp")); } SECTION("without a prefix every stale temporary goes and nothing else") {