fix: use-after-free in MoonrakerPrinterAgent teardown

src/slic3r/Utils/CrealityPrintAgent.hpp

@@ -37,7 +37,7 @@ public:
     };
 
     explicit CrealityPrintAgent(std::string log_dir);
-    ~CrealityPrintAgent() override = default;
+    ~CrealityPrintAgent() override { shutdown(); }
 
     static AgentInfo get_agent_info_static();
     AgentInfo        get_agent_info() override { return get_agent_info_static(); }

src/slic3r/Utils/MoonrakerPrinterAgent.cpp

@@ -287,18 +287,20 @@ bool moonraker_is_light_name(const std::string& name)
 
 MoonrakerPrinterAgent::MoonrakerPrinterAgent(std::string log_dir) : m_cloud_agent(nullptr) { (void) log_dir; }
 
-MoonrakerPrinterAgent::~MoonrakerPrinterAgent()
+MoonrakerPrinterAgent::~MoonrakerPrinterAgent() { shutdown(); }
+
+void MoonrakerPrinterAgent::shutdown()
 {
-    // Detached fetch_filament_info() threads (see QidiPrinterAgent::fetch_filament_info)
-    // hold a raw `this` with no other lifetime protection — wait for them to finish before
-    // any part of this object is torn down, so they never touch freed memory.
-    while (filament_fetch_in_flight.load() > 0) {
-        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    {
+        std::lock_guard<std::mutex> lock(fetch_lifecycle_mutex);
+        if (shutting_down.exchange(true)) {
+            return;
+        }
     }
 
+    // Stop the producers before waiting on in-flight fetches.
     {
         std::lock_guard<std::recursive_mutex> lock(connect_mutex);
-        device_info = MoonrakerDeviceInfo{};
         ++connect_generation;
     }
     if (connect_thread.joinable()) {
@@ -314,12 +316,25 @@ MoonrakerPrinterAgent::~MoonrakerPrinterAgent()
     if (cmd_thread.joinable()) {
         cmd_thread.join();
     }
+
+    while (filament_fetch_in_flight.load() > 0) {
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    }
+
+    // Fetches read device_info without the lock; clear it once none can run.
+    {
+        std::lock_guard<std::recursive_mutex> lock(connect_mutex);
+        device_info = MoonrakerDeviceInfo{};
+    }
 }
 
 void MoonrakerPrinterAgent::enqueue_command(std::function<void()> fn)
 {
     {
         std::lock_guard<std::mutex> lock(cmd_mutex);
+        if (cmd_stop) {
+            return;
+        }
         if (!cmd_thread.joinable()) {
             cmd_thread = std::thread(&MoonrakerPrinterAgent::run_command_worker, this);
         }

src/slic3r/Utils/MoonrakerPrinterAgent.hpp

@@ -156,12 +156,16 @@ protected:
     // State access for derived classes
     mutable std::recursive_mutex       state_mutex;
 
-    // Counts detached fetch_filament_info() background threads currently touching `this`
-    // (see QidiPrinterAgent::fetch_filament_info). Those threads hold a raw `this` with no
-    // other lifetime protection, so the destructor waits for this to reach 0 before any part
-    // of the object is torn down — see ~MoonrakerPrinterAgent().
+    // Detached fetch threads hold a raw `this`; shutdown() waits for this to reach 0.
     std::atomic<int> filament_fetch_in_flight{0};
 
+    // Idempotent teardown; must be called from the most-derived destructor.
+    void shutdown();
+    std::atomic<bool> shutting_down{false};
+
+    // Serializes the shutting_down check with the in-flight reservation.
+    std::mutex fetch_lifecycle_mutex;
+
     // Helpers
     bool        is_numeric(const std::string& value);
     std::string normalize_base_url(bool use_ssl, const std::string& host, const std::string& port);

src/slic3r/Utils/QidiPrinterAgent.cpp

@@ -16,6 +16,7 @@
 #include "libslic3r/Preset.hpp"
 #include <cstddef>
 #include <map>
+#include <mutex>
 #include <sstream>
 #include <thread>
 #include <string>
@@ -40,13 +41,16 @@ bool has_visible_base_preset(const PresetCollection& filaments, const std::strin
     return false;
 }
 
-// RAII decrement for MoonrakerPrinterAgent::filament_fetch_in_flight — guarantees the
-// counter drops back down on every exit path (early return or fall-through) inside the
-// detached fetch thread below, so ~MoonrakerPrinterAgent()'s wait loop can't stall forever.
+// RAII decrement for the in-flight fetch count; movable so a failed thread start still releases it.
 struct InFlightGuard
 {
-    std::atomic<int>& counter;
-    ~InFlightGuard() { counter.fetch_sub(1, std::memory_order_relaxed); }
+    std::atomic<int>* counter;
+    explicit InFlightGuard(std::atomic<int>& c) noexcept : counter(&c) {}
+    InFlightGuard(InFlightGuard&& other) noexcept : counter(other.counter) { other.counter = nullptr; }
+    InFlightGuard(const InFlightGuard&) = delete;
+    InFlightGuard& operator=(const InFlightGuard&) = delete;
+    InFlightGuard& operator=(InFlightGuard&&) = delete;
+    ~InFlightGuard() { if (counter) counter->fetch_sub(1, std::memory_order_relaxed); }
 };
 
 } // anonymous namespace
@@ -74,18 +78,26 @@ bool QidiPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSyncMode
     if (sync_mode != get_filament_sync_mode())
         return false;
 
-    // Snapshot only what the fetch needs, rather than reading device_info live from the
-    // background thread below — device_info can be concurrently rewritten by a reconnect
-    // on another thread while this fetch is still in flight.
+    // Snapshot what the fetch needs; a reconnect can rewrite device_info meanwhile.
     ConnectionSettings connection = get_connection_settings();
-    std::string model_id   = device_info.model_id;
-    std::string model_name = device_info.model_name;
+    std::string        model_id;
+    std::string        model_name;
+    {
+        std::lock_guard<std::recursive_mutex> lock(connect_mutex);
+        model_id   = device_info.model_id;
+        model_name = device_info.model_name;
+    }
 
-    filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed);
-
-    std::thread([this, connection = std::move(connection), model_id, model_name]() mutable {
-        InFlightGuard guard{filament_fetch_in_flight};
+    // Reserve under the same mutex shutdown() uses, so the flag and the count can't race.
+    {
+        std::lock_guard<std::mutex> lock(fetch_lifecycle_mutex);
+        if (shutting_down.load())
+            return false;
+        filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed);
+    }
 
+    InFlightGuard guard{filament_fetch_in_flight};
+    std::thread([this, guard = std::move(guard), connection = std::move(connection), model_id, model_name]() mutable {
         std::string error;
 
         // 1. Fetch device info and infer series_id

src/slic3r/Utils/QidiPrinterAgent.hpp

@@ -16,7 +16,7 @@ class QidiPrinterAgent final : public MoonrakerPrinterAgent
 {
 public:
     explicit QidiPrinterAgent(std::string log_dir);
-    ~QidiPrinterAgent() override = default;
+    ~QidiPrinterAgent() override { shutdown(); }
 
     static AgentInfo get_agent_info_static();
     AgentInfo        get_agent_info() override { return get_agent_info_static(); }

src/slic3r/Utils/SnapmakerPrinterAgent.cpp

@@ -11,6 +11,7 @@
 #include <boost/log/trivial.hpp>
 #include <chrono>
 #include <cstdint>
+#include <mutex>
 #include <sstream>
 #include <thread>
 #include <vector>
@@ -34,6 +35,18 @@ int64_t now_ms()
         std::chrono::steady_clock::now().time_since_epoch()).count();
 }
 
+// RAII decrement for the in-flight fetch count; movable so a failed thread start still releases it.
+struct InFlightGuard
+{
+    std::atomic<int>* counter;
+    explicit InFlightGuard(std::atomic<int>& c) noexcept : counter(&c) {}
+    InFlightGuard(InFlightGuard&& other) noexcept : counter(other.counter) { other.counter = nullptr; }
+    InFlightGuard(const InFlightGuard&) = delete;
+    InFlightGuard& operator=(const InFlightGuard&) = delete;
+    InFlightGuard& operator=(InFlightGuard&&) = delete;
+    ~InFlightGuard() { if (counter) counter->fetch_sub(1, std::memory_order_relaxed); }
+};
+
 // Safely access a parallel array by index, returning a fallback if out of bounds.
 template<typename T>
 T safe_at(const std::vector<T>& vec, int index, const T& fallback)
@@ -155,18 +168,24 @@ bool SnapmakerPrinterAgent::fetch_filament_info(std::string dev_id, FilamentSync
     if (sync_mode != get_filament_sync_mode())
         return false;
 
-    const std::string base_url = device_info.base_url;
-    const std::string api_key  = device_info.api_key;
+    std::string base_url;
+    std::string api_key;
+    {
+        std::lock_guard<std::recursive_mutex> lock(connect_mutex);
+        base_url = device_info.base_url;
+        api_key  = device_info.api_key;
+    }
 
-    filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed);
-
-    std::thread([this, base_url, api_key]() {
-        struct InFlightGuard
-        {
-            std::atomic<int>& counter;
-            ~InFlightGuard() { counter.fetch_sub(1, std::memory_order_relaxed); }
-        } guard{filament_fetch_in_flight};
+    // Reserve under the same mutex shutdown() uses, so the flag and the count can't race.
+    {
+        std::lock_guard<std::mutex> lock(fetch_lifecycle_mutex);
+        if (shutting_down.load())
+            return false;
+        filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed);
+    }
 
+    InFlightGuard guard{filament_fetch_in_flight};
+    std::thread([this, guard = std::move(guard), base_url, api_key]() {
         const std::string url = join_url(base_url, "/printer/objects/query?print_task_config&filament_detect");
 
         std::string response_body;

src/slic3r/Utils/SnapmakerPrinterAgent.hpp

@@ -13,7 +13,7 @@ class SnapmakerPrinterAgent final : public MoonrakerPrinterAgent
 {
 public:
     explicit SnapmakerPrinterAgent(std::string log_dir);
-    ~SnapmakerPrinterAgent() override = default;
+    ~SnapmakerPrinterAgent() override { shutdown(); }
 
     static AgentInfo get_agent_info_static();
     AgentInfo        get_agent_info() override { return get_agent_info_static(); }

tests/slic3rutils/test_printer_agent.cpp

@@ -170,6 +170,119 @@ TEST_CASE("unit: a fire-and-forget override of fetch_filament_info is not waited
     REQUIRE(agent->invoked.load() == true);
 }
 
+namespace {
+
+// Globals so a parked proxy fetch thread never dereferences a freed agent.
+std::atomic<int>  g_deferred_fetch_running{0};
+std::atomic<bool> g_deferred_destroy_returned{false};
+
+// Joins on scope exit so a throwing REQUIRE does not std::terminate.
+class ScopedJoiner
+{
+public:
+    explicit ScopedJoiner(std::thread& t) : m_thread(t) {}
+    ~ScopedJoiner() { if (m_thread.joinable()) m_thread.join(); }
+    ScopedJoiner(const ScopedJoiner&) = delete;
+    ScopedJoiner& operator=(const ScopedJoiner&) = delete;
+
+private:
+    std::thread& m_thread;
+};
+
+// A fetch that parks before touching the in-flight counter, so teardown's wait can
+// observe zero first.
+class DeferredFetchAgent : public MoonrakerPrinterAgent
+{
+public:
+    explicit DeferredFetchAgent(std::string log_dir) : MoonrakerPrinterAgent(std::move(log_dir)) {}
+
+    // Shared so a parked proxy fetch can never outlive the stack that owns it.
+    std::shared_ptr<std::promise<void>> entered{std::make_shared<std::promise<void>>()};
+    std::shared_ptr<std::promise<void>> allow_fetch{std::make_shared<std::promise<void>>()};
+    std::shared_ptr<std::promise<void>> allow_finish{std::make_shared<std::promise<void>>()};
+
+    // Runs the callable on the command worker, which teardown joins.
+    void post(std::function<void()> fn) { enqueue_command(std::move(fn)); }
+
+    bool fetch_filament_info(std::string /*dev_id*/, FilamentSyncMode /*sync_mode*/ = FilamentSyncMode::pull) override
+    {
+        // Resumes after ~DeferredFetchAgent destroyed these members; snapshot up front.
+        auto entered_p      = entered;
+        auto allow_fetch_p  = allow_fetch;
+        auto allow_finish_p = allow_finish;
+
+        entered_p->set_value();
+        allow_fetch_p->get_future().wait();
+
+        filament_fetch_in_flight.fetch_add(1, std::memory_order_relaxed);
+        std::thread([this, finish = std::move(allow_finish_p)] {
+            struct InFlightGuard
+            {
+                std::atomic<int>& counter;
+                ~InFlightGuard() { counter.fetch_sub(1, std::memory_order_relaxed); }
+            } guard{filament_fetch_in_flight};
+
+            g_deferred_fetch_running.fetch_add(1, std::memory_order_relaxed);
+            finish->get_future().wait();
+            g_deferred_fetch_running.fetch_sub(1, std::memory_order_relaxed);
+        }).detach();
+        return true;
+    }
+};
+
+} // namespace
+
+// REGRESSION - teardown must not return while a fetch it started is in flight.
+// The command worker parks a fetch before it reserves the in-flight slot, forcing
+// the "wait already observed zero" interleaving deterministically.
+TEST_CASE("an agent's destruction waits for a fetch started by its worker during teardown",
+          "[unit][moonraker][Regression]")
+{
+    g_deferred_fetch_running.store(0);
+    g_deferred_destroy_returned.store(false);
+
+    auto agent        = std::make_shared<DeferredFetchAgent>(std::string{});
+    auto entered      = agent->entered;
+    auto allow_fetch  = agent->allow_fetch;
+    auto allow_finish = agent->allow_finish;
+
+    // Park a fetch inside the command worker while the agent is still complete.
+    agent->post([ptr = agent.get()] { ptr->fetch_filament_info("dev", FilamentSyncMode::pull); });
+    REQUIRE(entered->get_future().wait_for(std::chrono::seconds(5)) == std::future_status::ready);
+
+    // Destroy on another thread so this one can drive the parked fetch.
+    std::thread destroyer([owned = std::move(agent)]() mutable {
+        owned.reset();
+        g_deferred_destroy_returned.store(true);
+    });
+    ScopedJoiner join_destroyer{destroyer};
+
+    // Let teardown pass its wait; the worker has not reserved yet.
+    std::this_thread::sleep_for(std::chrono::milliseconds(300));
+    allow_fetch->set_value();
+
+    // Get the fetch actually in flight (parked on allow_finish).
+    for (int i = 0; i < 200 && g_deferred_fetch_running.load() == 0; ++i)
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+    REQUIRE(g_deferred_fetch_running.load() == 1);
+
+    // A correct teardown cannot return while the fetch is parked; give a buggy one time.
+    for (int i = 0; i < 200 && !g_deferred_destroy_returned.load(); ++i)
+        std::this_thread::sleep_for(std::chrono::milliseconds(10));
+
+    if (g_deferred_destroy_returned.load()) {
+        // Bug: teardown returned with a fetch still running. Don't release allow_finish.
+        CHECK(g_deferred_fetch_running.load() == 0);
+        return;
+    }
+
+    // Fixed order: destruction is still blocked on the in-flight fetch.
+    allow_finish->set_value();
+    destroyer.join();
+    CHECK(g_deferred_destroy_returned.load());
+    CHECK(g_deferred_fetch_running.load() == 0);
+}
+
 // ===========================================================================
 // UNIT - printer-agent registry duplicate handling.
 // Confirms a duplicate agent id is rejected so a plugin cannot shadow a built-in