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

@@ -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
 {

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;

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<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); }
+// 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<std::string_view> 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 `<name>.<pid>.<n>.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.

src/libslic3r/utils.cpp

@@ -14,6 +14,12 @@
 #include <thread>
 #include <cstring>
 #include <iomanip>
+#ifdef _WIN32
+#include <io.h>
+#else
+#include <unistd.h>
+#include <sys/stat.h>
+#endif
 #include <algorithm>
 #include <cmath>
 
@@ -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<std::errc>(errno) : std::errc::io_error);
 }
 
-std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> chunks, bool binary)
+std::error_code write_file_atomically(const std::string &path, std::initializer_list<std::string_view> 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;
 }

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";