From 16efd3458b5d64d69e243712fad21253686f93ab Mon Sep 17 00:00:00 2001 From: packerlschupfer <83344883+packerlschupfer@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:57:08 +0200 Subject: [PATCH] 3mf: claim the backup lock when the backup's state cannot be established (#16431) has_restore_data() signals "this backup is in use, leave it alone" by setting origin to "". Its caller treats anything else as permission to delete (Plater.cpp, the EVT_RESTORE_PROJECT handler): try { if (originfile != "") // see bbs_3mf.cpp for lock detail boost::filesystem::remove_all(last); } catch (...) {} So every "return false" that leaves origin empty is a delete instruction, and three of them fired without anyone having established that the backup was spent. lock.txt exists but cannot be read. load_string_file() was outside the try, so the exception left has_restore_data() altogether. The handler is queued and has no try around the call, so this took the application down. Fixes #15670. lock.txt reads but holds no pid. A crash or a full disk mid-write leaves a zero-length lock.txt, because save_string_file() truncates before it writes. boost::lexical_cast("") throws into the catch, which returned false with origin still empty, and the caller deleted the backup. Also #15670. The .3mf probe could not answer. A path that cannot be probed at all reports false exactly like a missing one, so an unreachable backup looked spent. The error_code now separates the two: ENOENT and ENOTDIR mean "not there" and stay collectable, any other error means the question was never answered. All three now claim the lock. Declining a restore is recoverable; deleting a backup nobody could examine is not. What deliberately did not change: a readable directory with no lock and no project still comes back with origin empty and stays collectable, as do a lock held by another process and a directory that does not exist. Claiming the lock whenever anything looked amiss would strand every spent backup on disk, so those three cases are tests rather than prose. Tested in tests/libslic3r/test_3mf.cpp. Against the previous behaviour the new cases fail as they should -- the two uninterpretable locks come back with an empty origin, and the unreadable one escapes as basic_ios::clear: iostream error while the three collectable cases pass unchanged, before and after. --- src/libslic3r/Format/bbs_3mf.cpp | 17 ++++-- tests/libslic3r/test_3mf.cpp | 91 ++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 4 deletions(-) diff --git a/src/libslic3r/Format/bbs_3mf.cpp b/src/libslic3r/Format/bbs_3mf.cpp index 2914693c42..86e306d4ac 100644 --- a/src/libslic3r/Format/bbs_3mf.cpp +++ b/src/libslic3r/Format/bbs_3mf.cpp @@ -51,6 +51,7 @@ #include #include #include +#include #include #include #include @@ -9776,13 +9777,13 @@ bool has_restore_data(std::string & path, std::string& origin) origin = ""; return false; } - // ORCA: non-throwing probes; ec is deliberately unread, a backup path that cannot be - // probed is treated as having no restore data rather than aborting the application. + // ORCA: the caller removes this directory unless origin is "", so every way of + // failing to establish the backup's state has to claim the lock instead. boost::system::error_code ec; if (boost::filesystem::exists(path + "/lock.txt", ec)) { std::string pid; - load_string_file(path + "/lock.txt", pid); try { + load_string_file(path + "/lock.txt", pid); if (get_process_name(boost::lexical_cast(pid)) == get_process_name(0)) { origin = ""; @@ -9790,12 +9791,20 @@ bool has_restore_data(std::string & path, std::string& origin) } } catch (...) { + origin = ""; return false; } } std::string file3mf = path + "/.3mf"; - if (!boost::filesystem::exists(file3mf, ec)) + if (!boost::filesystem::exists(file3mf, ec)) { + // ORCA: a .3mf that is simply absent is a spent backup and stays collectable. A probe + // that could not look must not be: ec reports ENOENT/ENOTDIR for "not there" as + // well, so only a different error means the answer is unknown. + if (ec && ec != boost::system::errc::no_such_file_or_directory && + ec != boost::system::errc::not_a_directory) + origin = ""; return false; + } try { if (boost::filesystem::exists(path + "/origin.txt", ec)) load_string_file(path + "/origin.txt", origin); diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index e3cf4b92c5..2d5656b733 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -2361,5 +2361,96 @@ TEST_CASE("has_restore_data reports no restore data for a path it cannot probe", REQUIRE_NOTHROW(has = has_restore_data(unprobeable, origin2)); CHECK_FALSE(has); CHECK(unprobeable == backup.string()); // not rewritten to the .3mf it could not confirm + CHECK(origin2 == ""); // and the caller is told not to remove it } #endif // _WIN32 + +// Every "return false" that leaves origin empty is an instruction to the caller to +// remove_all() the backup directory (Plater.cpp's EVT_RESTORE_PROJECT handler), so the +// distinction these cases draw is between a backup that is spent and one whose state could +// not be established. Only the first may be collected. +TEST_CASE("has_restore_data claims the lock when lock.txt cannot be interpreted", "[3mf]") { + ScopedTemporaryDir outer("orca_lock"); + boost::filesystem::path backup = outer.path() / "backup"; + REQUIRE(boost::filesystem::create_directory(backup)); + std::ofstream((backup / ".3mf").string()) << "stand-in for the backed-up project"; + + SECTION("an empty lock.txt holds no pid") { + std::ofstream lock((backup / "lock.txt").string()); + lock.close(); // created, left zero length + REQUIRE(boost::filesystem::file_size(backup / "lock.txt") == 0); + + std::string path = backup.string(), origin; + bool has = true; + REQUIRE_NOTHROW(has = has_restore_data(path, origin)); + CHECK_FALSE(has); + CHECK(origin == ""); + } + + SECTION("lock.txt holds something that is not a pid") { + std::ofstream((backup / "lock.txt").string()) << "not-a-pid"; + + std::string path = backup.string(), origin; + bool has = true; + REQUIRE_NOTHROW(has = has_restore_data(path, origin)); + CHECK_FALSE(has); + CHECK(origin == ""); + } +} + +#ifndef _WIN32 +TEST_CASE("has_restore_data claims the lock when lock.txt cannot be read", "[3mf]") { + if (::geteuid() == 0) + SKIP("root ignores the file permissions this test depends on"); + + ScopedTemporaryDir outer("orca_lock"); + boost::filesystem::path backup = outer.path() / "backup"; + REQUIRE(boost::filesystem::create_directory(backup)); + std::ofstream((backup / ".3mf").string()) << "stand-in for the backed-up project"; + std::ofstream((backup / "lock.txt").string()) << "1"; + boost::system::error_code mode_ec; // a filesystem that refuses the mode change must skip below + boost::filesystem::permissions(backup / "lock.txt", boost::filesystem::no_perms, mode_ec); + + boost::nowide::ifstream probe((backup / "lock.txt").string()); + if (probe.good()) + SKIP("this filesystem does not enforce file read permissions"); + + // load_string_file() throws here. + std::string path = backup.string(), origin; + bool has = true; + REQUIRE_NOTHROW(has = has_restore_data(path, origin)); + CHECK_FALSE(has); + CHECK(origin == ""); +} +#endif // _WIN32 + +// The negative control for the three cases above: claiming the lock whenever anything is +// amiss would strand every spent backup on disk forever, so a readable directory that +// simply has nothing to restore must still come back collectable. +TEST_CASE("has_restore_data leaves a spent backup collectable", "[3mf]") { + ScopedTemporaryDir outer("orca_spent"); + boost::filesystem::path backup = outer.path() / "backup"; + REQUIRE(boost::filesystem::create_directory(backup)); + + SECTION("no lock and no project") { + std::string path = backup.string(), origin; + CHECK_FALSE(has_restore_data(path, origin)); + CHECK(origin.empty()); + } + + SECTION("a lock from a process that is not us, and no project") { + // get_process_name() compares executable names and answers empty for a pid it cannot + // read, so pid 1 never matches this process and the lock must not be claimed. + std::ofstream((backup / "lock.txt").string()) << "1"; + + std::string path = backup.string(), origin; + CHECK_FALSE(has_restore_data(path, origin)); + CHECK(origin.empty()); + } + + SECTION("the directory does not exist at all") { + std::string path = (outer.path() / "gone").string(), origin; + CHECK_FALSE(has_restore_data(path, origin)); + CHECK(origin.empty()); + } +}