Keep the Sweep Out of the Write Helper and Honour a Read-Only Target

Sweeping the target's directory from inside write_file_atomically() made
every settings export into a user's folder delete their own numbered
files that matched the older config temporary form, and cost a directory
walk per save. The helper writes its target and nothing else; the user
preset scan, the bundle metadata reads and AppConfig::load() sweep the
directories the application owns, once, while they hold the lock.

Replacing a file needs only a writable directory, so a preset or config
the user made read-only was overwritten where the in-place write used to
fail; such a target is refused before anything is written. The CLI's
load_if_exists() takes no lock and creates no lock file, since the CLI
never saves. The lock guard holds the slot mutex through a unique_lock,
so an exception during construction cannot leave the slot locked for
good, and it counts its entry last so a throw leaves the slot as found;
the cool-down after a failed open is set where the failure is seen.

The cloud agent's sync state and secret fallback file and the 3DPrinterOS
session file wrote through a fixed ".tmp" name with a non-Unicode stream;
they call the helper. The vendor cache failure test makes the cache
read-only, which the helper refuses on every platform, and the utility
tests carry the PascalCase tag the test rules ask for.

src/libslic3r/AppConfig.cpp

@@ -731,12 +731,16 @@ static bool verify_config_file_checksum(boost::nowide::ifstream &ifs)
 
 
 #ifdef USE_JSON_CONFIG
-std::string AppConfig::load()
+std::string AppConfig::load(bool read_only)
 {
     json j;
 
     // Keep another instance from replacing or restoring the file mid-read.
-    InstanceLock instance_lock(lock_path());
+    InstanceLock instance_lock(read_only ? std::string() : lock_path());
+    if (instance_lock.locked()) {
+        const boost::filesystem::path conf(config_path());
+        remove_stale_temp_files(conf.parent_path(), conf.filename().string());
+    }
 
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
@@ -1141,10 +1145,14 @@ void AppConfig::save()
 
 #else
 
-std::string AppConfig::load()
+std::string AppConfig::load(bool read_only)
 {
     // Keep another instance from replacing or restoring the file mid-read.
-    InstanceLock instance_lock(lock_path());
+    InstanceLock instance_lock(read_only ? std::string() : lock_path());
+    if (instance_lock.locked()) {
+        const boost::filesystem::path conf(config_path());
+        remove_stale_temp_files(conf.parent_path(), conf.filename().string());
+    }
 
     // 1) Read the complete config file into a boost::property_tree.
     namespace pt = boost::property_tree;
@@ -1866,7 +1874,7 @@ bool AppConfig::exists()
 
 std::string AppConfig::load_if_exists()
 {
-    return boost::filesystem::exists(loading_path()) ? load() : std::string();
+    return boost::filesystem::exists(loading_path()) ? load(/*read_only=*/true) : std::string();
 }
 
 }; // namespace Slic3r

src/libslic3r/AppConfig.hpp

@@ -121,8 +121,9 @@ public:
 
 	// Load the slic3r.ini from a user profile directory (or a datadir, if configured).
 	// Return an error string, or an empty string on success.
-	std::string         load();
+	std::string         load(bool read_only = false);
 	// Treat a missing config as default state; otherwise load it normally.
+	// The CLI's load: it never saves, so it takes no lock and creates no lock file.
 	std::string         load_if_exists();
 	// Store the slic3r.ini into a user profile directory (or a datadir, if configured).
 	void 			   	save();

src/libslic3r/InstanceLock.cpp

@@ -2,7 +2,6 @@
 
 #include <map>
 #include <memory>
-#include <mutex>
 #include <thread>
 
 #include <boost/interprocess/sync/file_lock.hpp>
@@ -46,21 +45,23 @@ InstanceLock::Slot &InstanceLock::slot_for(const std::string &lock_file_path)
     return *slot;
 }
 
-// Creates the lock file if needed and opens it. Called with the slot mutex held.
-static bool open_lock_file(std::unique_ptr<boost::interprocess::file_lock> &file_lock, const std::string &lock_file_path)
+// Creates the lock file if needed and opens it, or starts the cool-down.
+// Called with the slot mutex held.
+bool InstanceLock::open_lock_file(Slot &slot, const std::string &lock_file_path)
 {
     try {
         // file_lock only opens existing files.
         boost::nowide::ofstream(lock_file_path, std::ios::app).close();
 #ifdef _WIN32
-        file_lock = std::make_unique<boost::interprocess::file_lock>(boost::nowide::widen(lock_file_path).c_str());
+        slot.file_lock = std::make_unique<boost::interprocess::file_lock>(boost::nowide::widen(lock_file_path).c_str());
 #else
-        file_lock = std::make_unique<boost::interprocess::file_lock>(lock_file_path.c_str());
+        slot.file_lock = std::make_unique<boost::interprocess::file_lock>(lock_file_path.c_str());
 #endif
         return true;
     } catch (const std::exception &e) {
+        slot.retry_at = std::chrono::steady_clock::now() + cooldown;
         BOOST_LOG_TRIVIAL(warning) << "Cannot open lock file " << lock_file_path << ": " << e.what()
-                                   << "; other instances are not excluded from writing";
+                                   << "; other instances are not excluded from writing for the next " << cooldown.count() << " ms";
         return false;
     }
 }
@@ -69,11 +70,11 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
 {
     if (lock_file_path.empty())
         return;
-    m_slot = &slot_for(lock_file_path);
-    m_slot->mutex.lock();
+    m_slot       = &slot_for(lock_file_path);
+    m_slot_guard = std::unique_lock<std::recursive_mutex>(m_slot->mutex);
     const auto now = std::chrono::steady_clock::now();
-    if (m_slot->depth++ == 0 && now >= m_slot->retry_at &&
-        (m_slot->file_lock || open_lock_file(m_slot->file_lock, lock_file_path))) {
+    if (m_slot->depth == 0 && now >= m_slot->retry_at &&
+        (m_slot->file_lock || open_lock_file(*m_slot, lock_file_path))) {
         const bool wait     = now >= m_slot->skip_waiting_until;
         const auto deadline = now + timeout;
         for (;;) {
@@ -99,9 +100,9 @@ InstanceLock::InstanceLock(const std::string &lock_file_path, std::chrono::milli
             }
             std::this_thread::sleep_for(std::chrono::milliseconds(5));
         }
-    } else if (m_slot->depth == 1 && ! m_slot->file_lock && now >= m_slot->retry_at) {
-        m_slot->retry_at = now + cooldown;
     }
+    // Counted last, so a throw above leaves the slot exactly as it was found.
+    ++ m_slot->depth;
     m_locked = m_slot->file_locked;
 }
 
@@ -109,7 +110,7 @@ InstanceLock::~InstanceLock()
 {
     if (m_slot == nullptr)
         return;
-    if (--m_slot->depth == 0 && m_slot->file_locked) {
+    if (-- m_slot->depth == 0 && m_slot->file_locked) {
         try {
             m_slot->file_lock->unlock();
         } catch (const std::exception &e) {
@@ -117,7 +118,6 @@ InstanceLock::~InstanceLock()
         }
         m_slot->file_locked = false;
     }
-    m_slot->mutex.unlock();
 }
 
 } // namespace Slic3r

src/libslic3r/InstanceLock.hpp

@@ -1,6 +1,7 @@
 #pragma once
 
 #include <chrono>
+#include <mutex>
 #include <string>
 
 namespace Slic3r {
@@ -49,8 +50,10 @@ public:
 private:
     struct Slot;
     static Slot &slot_for(const std::string &lock_file_path);
+    static bool  open_lock_file(Slot &slot, const std::string &lock_file_path);
 
     Slot *m_slot{nullptr};
+    std::unique_lock<std::recursive_mutex> m_slot_guard;
     bool  m_locked{false};
 };
 

src/libslic3r/PresetBundle.cpp

@@ -1244,6 +1244,8 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
             {
                 // 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;
             }
             metadata.print_presets.clear();
@@ -1283,6 +1285,8 @@ PresetsConfigSubstitutions PresetBundle::load_user_presets(std::string user, For
             {
                 // 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;
             }
             metadata.print_presets.clear();

src/libslic3r/Utils.hpp

@@ -227,17 +227,18 @@ 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. A target that is not a regular file (a symlink,
+// 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. A successful
-// write also removes stale temporaries of the same target.
+// 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<std::string_view> 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); }
 // Remove the `<name>.<pid>.<n>.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 `<name_prefix>.<pid>` leftovers of the older AppConfig writer.
-// Returns how many were removed.
+// go, and so do `<name_prefix>.<pid>` 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.
 extern size_t remove_stale_temp_files(const boost::filesystem::path &dir, const std::string &name_prefix = std::string());
 
 enum CopyFileResult {

src/libslic3r/utils.cpp

@@ -740,6 +740,10 @@ 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)
+		return std::make_error_code(std::errc::permission_denied);
 
 	// Unique per process and per call, so two threads writing one target
 	// without a lock never share a temporary.
@@ -772,8 +776,6 @@ std::error_code write_file_atomically(const std::string &path, std::initializer_
 	if (target_exists)
 		boost::filesystem::permissions(path, target.permissions(), bec);
 #endif
-	const boost::filesystem::path target_path(path);
-	remove_stale_temp_files(target_path.parent_path(), target_path.filename().string());
 	return {};
 }
 

src/slic3r/Utils/3DPrinterOS.cpp

@@ -2,6 +2,7 @@
 
 #include <algorithm>
 #include <sstream>
+#include <system_error>
 #include <exception>
 #include <boost/format.hpp>
 #include <boost/log/trivial.hpp>
@@ -580,9 +581,10 @@ bool C3DPrinterOS::save_api_session(const std::string &session, const std::strin
     j.put("session", session);
     j.put("email", email);
     try {
-        auto temp_path = m_api_session_file_path + ".tmp";
-        pt::write_json(temp_path, j);
-        boost::filesystem::rename(temp_path, m_api_session_file_path);
+        std::ostringstream json;
+        pt::write_json(json, j);
+        if (const std::error_code ec = write_file_atomically(m_api_session_file_path, json.str()))
+            throw std::system_error(ec);
     } catch (const std::exception &err) {
         BOOST_LOG_TRIVIAL(error) << __FUNCTION__ << ": failed to write json to file. Path = "
                                  << m_api_session_file_path

src/slic3r/Utils/OrcaCloudServiceAgent.cpp

@@ -1475,15 +1475,8 @@ void OrcaCloudServiceAgent::save_sync_state()
     if (sync_state_path.empty())
         return;
 
-    try {
-        std::string tmp_path = sync_state_path + ".tmp";
-        std::ofstream ofs(tmp_path, std::ios::out | std::ios::trunc);
-        if (ofs.good()) {
-            ofs << std::to_string(sync_state.last_sync_timestamp);
-            ofs.close();
-            boost::filesystem::rename(tmp_path, sync_state_path);
-        }
-    } catch (...) {}
+    if (const std::error_code ec = write_file_atomically(sync_state_path, std::to_string(sync_state.last_sync_timestamp)))
+        BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: failed to save the sync state: " << ec.message();
 }
 
 void OrcaCloudServiceAgent::clear_sync_state()
@@ -1572,22 +1565,10 @@ void OrcaCloudServiceAgent::persist_user_secret(const std::string& secret)
             wxFileName::Mkdir(path.GetPath(), wxS_DIR_DEFAULT, wxPATH_MKDIR_FULL);
         }
 
-        const std::string tmp_path = secret_fallback_path + ".tmp";
-        std::ofstream ofs(tmp_path, std::ios::out | std::ios::trunc | std::ios::binary);
-        if (ofs.good()) {
-            ofs << signed_payload;
-            ofs.flush();
-            ofs.close();
-
-            if (wxRenameFile(wxString::FromUTF8(tmp_path.c_str()), wxString::FromUTF8(secret_fallback_path.c_str()), true)) {
-                stored = true;
-            } else {
-                wxRemoveFile(wxString::FromUTF8(tmp_path.c_str()));
-                BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: failed to atomically replace user secret file";
-            }
-        } else {
-            BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: cannot open user secret file for write - " << secret_fallback_path;
-        }
+        if (const std::error_code ec = write_file_atomically(secret_fallback_path, signed_payload, /*binary=*/true))
+            BOOST_LOG_TRIVIAL(warning) << "OrcaCloudServiceAgent: cannot write user secret file " << secret_fallback_path << ": " << ec.message();
+        else
+            stored = true;
     } else {
         // Use wxSecretStore only
         wxSecretStore store = wxSecretStore::GetDefault();

tests/libslic3r/test_utils.cpp

@@ -20,7 +20,7 @@
 
 using namespace Slic3r;
 
-TEST_CASE("per_user_temp_dir composes a per-user temp root", "[utils]") {
+TEST_CASE("per_user_temp_dir composes a per-user temp root", "[Utils]") {
     const std::string base = "/tmp";
 
     SECTION("an empty id returns base unchanged") {
@@ -34,7 +34,7 @@ TEST_CASE("per_user_temp_dir composes a per-user temp root", "[utils]") {
     }
 }
 
-TEST_CASE("per_user_temp_id follows the platform contract", "[utils]") {
+TEST_CASE("per_user_temp_id follows the platform contract", "[Utils]") {
     const std::string id = per_user_temp_id();
 
     SECTION("stable across calls") {
@@ -54,7 +54,7 @@ TEST_CASE("per_user_temp_id follows the platform contract", "[utils]") {
 
 // The end-to-end contract callers depend on: the temp root is left alone on
 // Windows and isolated per user on Linux/macOS.
-TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[utils]") {
+TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[Utils]") {
     const std::string base = "/tmp";
     const std::string root = per_user_temp_dir(base, per_user_temp_id());
 #ifdef _WIN32
@@ -65,7 +65,7 @@ TEST_CASE("per-user temp root is unchanged on Windows, isolated elsewhere", "[ut
 #endif
 }
 
-TEST_CASE("write_file_atomically replaces the target and leaves no temporary file", "[utils]") {
+TEST_CASE("write_file_atomically replaces the target and leaves no temporary file", "[Utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "preset.json";
 
@@ -83,7 +83,7 @@ TEST_CASE("write_file_atomically replaces the target and leaves no temporary fil
     REQUIRE(entries == 1);
 }
 
-TEST_CASE("write_file_atomically reports a missing directory and writes nothing", "[utils]") {
+TEST_CASE("write_file_atomically reports a missing directory and writes nothing", "[Utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "missing" / "preset.json";
 
@@ -92,7 +92,22 @@ TEST_CASE("write_file_atomically reports a missing directory and writes nothing"
     REQUIRE_FALSE(boost::filesystem::exists(target));
 }
 
-TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[utils]") {
+TEST_CASE("write_file_atomically refuses a read-only target and leaves it untouched", "[Utils]") {
+    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");
+    boost::filesystem::permissions(target, boost::filesystem::owner_read | boost::filesystem::owner_write | boost::filesystem::group_read | boost::filesystem::others_read);
+
+    REQUIRE(ec == std::errc::permission_denied);
+    std::string content;
+    load_string_file(target, content);
+    REQUIRE(content == "pinned");
+}
+
+TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[Utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "blob.bin";
     const std::string bytes("a\r\nb\0c", 6);
@@ -102,7 +117,7 @@ TEST_CASE("write_file_atomically keeps bytes intact in binary mode", "[utils]")
 }
 
 #ifndef _WIN32
-TEST_CASE("write_file_atomically writes through a symlink and keeps the target's permissions", "[utils]") {
+TEST_CASE("write_file_atomically writes through a symlink and keeps the target's permissions", "[Utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path real = dir.path() / "real.json";
     const boost::filesystem::path link = dir.path() / "link.json";
@@ -123,7 +138,7 @@ TEST_CASE("write_file_atomically writes through a symlink and keeps the target's
 }
 #endif
 
-TEST_CASE("write_file_atomically survives two threads writing one target", "[utils]") {
+TEST_CASE("write_file_atomically survives two threads writing one target", "[Utils]") {
     ScopedTemporaryDir dir;
     const boost::filesystem::path target = dir.path() / "shared.json";
     const std::string a(20000, 'a'), b(20000, 'b');
@@ -148,7 +163,7 @@ TEST_CASE("write_file_atomically survives two threads writing one target", "[uti
     REQUIRE(entries == 1);
 }
 
-TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[utils]") {
+TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[Utils]") {
     ScopedTemporaryDir dir;
     for (const char *name : { "a.json.123.tmp", "b.info.4.tmp", "c.json", "d.tmp", "e.json.x.tmp", "f.json..tmp", "a.json.99", "a.json.12.3.tmp" }) {
         REQUIRE_FALSE(write_file_atomically((dir.path() / name).string(), "x"));
@@ -178,7 +193,7 @@ TEST_CASE("remove_stale_temp_files removes only old <name>.<pid>.tmp files", "[u
     }
 }
 
-TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[utils]") {
+TEST_CASE("copy_file reports the OS error when the destination cannot be written", "[Utils]") {
     ScopedTemporaryFile source(".txt");
     {
         std::ofstream ofs(source.string(), std::ios::binary);
@@ -207,7 +222,7 @@ TEST_CASE("copy_file reports the OS error when the destination cannot be written
 #endif // _WIN32
 }
 
-TEST_CASE("A resolved input path still names the same file after the working directory changes", "[utils]") {
+TEST_CASE("A resolved input path still names the same file after the working directory changes", "[Utils]") {
     ScopedTemporaryFile model(".3mf");
     { std::ofstream out(model.string()); out << "3mf"; }
     const std::string name = model.path().filename().string();
@@ -224,7 +239,7 @@ TEST_CASE("A resolved input path still names the same file after the working dir
     REQUIRE_FALSE(boost::filesystem::exists(name));
 }
 
-TEST_CASE("resolve_cli_input_path completes a relative path against the working directory", "[utils]") {
+TEST_CASE("resolve_cli_input_path completes a relative path against the working directory", "[Utils]") {
     ScopedWorkingDirectory cwd(boost::filesystem::temp_directory_path());
     // Read back rather than reusing temp_directory_path(): changing to it resolves any symlink.
     const boost::filesystem::path here = boost::filesystem::current_path();
@@ -240,7 +255,7 @@ TEST_CASE("resolve_cli_input_path completes a relative path against the working
     }
 }
 
-TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed unchanged", "[utils]") {
+TEST_CASE("resolve_cli_input_path leaves inputs that must not be completed unchanged", "[Utils]") {
     SECTION("an absolute path") {
         const boost::filesystem::path absolute = (boost::filesystem::temp_directory_path() / "model.3mf").make_preferred();
         REQUIRE(resolve_cli_input_path(absolute.string()) == absolute.string());

tests/libslic3r/test_vendor_cache.cpp

@@ -1,9 +1,5 @@
 #include <catch2/catch_all.hpp>
 
-#ifndef _WIN32
-#include <unistd.h>
-#endif
-
 #include <boost/filesystem.hpp>
 #include <boost/crc.hpp>
 #include <cereal/archives/binary.hpp>
@@ -1360,31 +1356,22 @@ TEST_CASE("a header claiming more body than the file holds is rejected", "[Vendo
     REQUIRE_FALSE(bundle.load_vendor_cache(cache, "Bounded", Semver(1, 0, 0)));
 }
 
-#ifndef _WIN32
-// Permissions are what makes the write fail here, which Windows does not
-// express through chmod and root ignores; the helper's own tests cover the rest.
 TEST_CASE("a failed write leaves the previous cache in place", "[VendorCache]")
 {
-    if (::geteuid() == 0)
-        SKIP("permissions do not apply to root");
     TempDir tmp;
     const std::string cache = (tmp.path / "Durable.opc").string();
     REQUIRE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "1.0.0"));
     const std::string before = slurp(cache);
 
-    // No temp file can be created beside the cache and the cache itself cannot
-    // be opened for writing: the write cannot complete, and must not have
-    // destroyed what was already there to find that out.
+    // A read-only cache (the read-only attribute on Windows) is refused before
+    // anything is written, so what was there must survive the attempt.
     fs::permissions(cache, fs::owner_read | fs::group_read | fs::others_read);
-    fs::permissions(tmp.path, fs::owner_read | fs::owner_exe | fs::group_read | fs::group_exe | fs::others_read | fs::others_exe);
 
     REQUIRE_FALSE(save_one_vendor(cache, one_vendor("Durable"), "Durable", "2.0.0"));
 
-    fs::permissions(tmp.path, fs::owner_all | fs::group_read | fs::group_exe | fs::others_read | fs::others_exe);
     fs::permissions(cache, fs::owner_read | fs::owner_write | fs::group_read | fs::others_read);
     CHECK(slurp(cache) == before);
 }
-#endif
 
 TEST_CASE("a cache written by another build's option ordering still loads", "[VendorCache]")
 {