From 5e861b31f99d8ffe6a428606249e5bc78a8f6096 Mon Sep 17 00:00:00 2001 From: Ian Chua Date: Tue, 6 Oct 2026 20:54:47 +0800 Subject: [PATCH] fix: use-after-free in MoonrakerPrinterAgent teardown --- src/slic3r/Utils/CrealityPrintAgent.hpp | 2 +- src/slic3r/Utils/MoonrakerPrinterAgent.cpp | 29 ++++-- src/slic3r/Utils/MoonrakerPrinterAgent.hpp | 12 ++- src/slic3r/Utils/QidiPrinterAgent.cpp | 40 +++++--- src/slic3r/Utils/QidiPrinterAgent.hpp | 2 +- src/slic3r/Utils/SnapmakerPrinterAgent.cpp | 39 +++++-- src/slic3r/Utils/SnapmakerPrinterAgent.hpp | 2 +- tests/slic3rutils/test_printer_agent.cpp | 113 +++++++++++++++++++++ 8 files changed, 201 insertions(+), 38 deletions(-) diff --git a/src/slic3r/Utils/CrealityPrintAgent.hpp b/src/slic3r/Utils/CrealityPrintAgent.hpp index 3b925c5d21..6c2aded326 100644 --- a/src/slic3r/Utils/CrealityPrintAgent.hpp +++ b/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(); } diff --git a/src/slic3r/Utils/MoonrakerPrinterAgent.cpp b/src/slic3r/Utils/MoonrakerPrinterAgent.cpp index 8990a6cd00..10cce3e330 100644 --- a/src/slic3r/Utils/MoonrakerPrinterAgent.cpp +++ b/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 lock(fetch_lifecycle_mutex); + if (shutting_down.exchange(true)) { + return; + } } + // Stop the producers before waiting on in-flight fetches. { std::lock_guard 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 lock(connect_mutex); + device_info = MoonrakerDeviceInfo{}; + } } void MoonrakerPrinterAgent::enqueue_command(std::function fn) { { std::lock_guard lock(cmd_mutex); + if (cmd_stop) { + return; + } if (!cmd_thread.joinable()) { cmd_thread = std::thread(&MoonrakerPrinterAgent::run_command_worker, this); } diff --git a/src/slic3r/Utils/MoonrakerPrinterAgent.hpp b/src/slic3r/Utils/MoonrakerPrinterAgent.hpp index d1ff866dad..550355d995 100644 --- a/src/slic3r/Utils/MoonrakerPrinterAgent.hpp +++ b/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 filament_fetch_in_flight{0}; + // Idempotent teardown; must be called from the most-derived destructor. + void shutdown(); + std::atomic 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); diff --git a/src/slic3r/Utils/QidiPrinterAgent.cpp b/src/slic3r/Utils/QidiPrinterAgent.cpp index 175c3f88f7..09b5649953 100644 --- a/src/slic3r/Utils/QidiPrinterAgent.cpp +++ b/src/slic3r/Utils/QidiPrinterAgent.cpp @@ -16,6 +16,7 @@ #include "libslic3r/Preset.hpp" #include #include +#include #include #include #include @@ -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& counter; - ~InFlightGuard() { counter.fetch_sub(1, std::memory_order_relaxed); } + std::atomic* counter; + explicit InFlightGuard(std::atomic& 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 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 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 diff --git a/src/slic3r/Utils/QidiPrinterAgent.hpp b/src/slic3r/Utils/QidiPrinterAgent.hpp index 0c83fa3d95..c2229ec4c9 100644 --- a/src/slic3r/Utils/QidiPrinterAgent.hpp +++ b/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(); } diff --git a/src/slic3r/Utils/SnapmakerPrinterAgent.cpp b/src/slic3r/Utils/SnapmakerPrinterAgent.cpp index 71ec358b62..2a09ffa71d 100644 --- a/src/slic3r/Utils/SnapmakerPrinterAgent.cpp +++ b/src/slic3r/Utils/SnapmakerPrinterAgent.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -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* counter; + explicit InFlightGuard(std::atomic& 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 T safe_at(const std::vector& 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 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& 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 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; diff --git a/src/slic3r/Utils/SnapmakerPrinterAgent.hpp b/src/slic3r/Utils/SnapmakerPrinterAgent.hpp index db3ea4622c..8a40434424 100644 --- a/src/slic3r/Utils/SnapmakerPrinterAgent.hpp +++ b/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(); } diff --git a/tests/slic3rutils/test_printer_agent.cpp b/tests/slic3rutils/test_printer_agent.cpp index 4a82c69664..088e319f5d 100644 --- a/tests/slic3rutils/test_printer_agent.cpp +++ b/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 g_deferred_fetch_running{0}; +std::atomic 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> entered{std::make_shared>()}; + std::shared_ptr> allow_fetch{std::make_shared>()}; + std::shared_ptr> allow_finish{std::make_shared>()}; + + // Runs the callable on the command worker, which teardown joins. + void post(std::function 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& 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(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