From 77248a7e9d174df0ebeb19d3b42e590d4f90eb90 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Mon, 28 Sep 2026 05:56:15 +0800 Subject: [PATCH] Stop Malformed Network Responses from Crashing the App Duet, MKS and UltiMaker parsed print host replies with boost read_json inside the HTTP completion callback with no try, so an HTML or truncated reply threw out of the Physical Printer Test button and terminated the app, or killed the upload queue thread. The five identical copies of the parser (ESP3D's and Flashforge's were unused) are replaced by one shared PrintHost::get_err_code_from_body that reports a non-JSON reply as an error. The upload queue now catches a failing job per job, so one bad upload no longer leaves later jobs queued forever. Flashforge read material station slots with nlohmann value(), which throws on off-type fields or non-object entries. The parsing moves into Flashforge::parse_material_slots, which reads fields leniently with the existing try_parse_json_int and skips bad entries. UserManager::parse_json parsed the payload before its try block; the parse now happens inside it. --- src/slic3r/GUI/UserManager.cpp | 9 ++- src/slic3r/Utils/Duet.cpp | 9 --- src/slic3r/Utils/Duet.hpp | 1 - src/slic3r/Utils/ESP3D.cpp | 9 --- src/slic3r/Utils/ESP3D.hpp | 1 - src/slic3r/Utils/Flashforge.cpp | 40 +++++---- src/slic3r/Utils/Flashforge.hpp | 3 +- src/slic3r/Utils/MKS.cpp | 9 --- src/slic3r/Utils/MKS.hpp | 1 - src/slic3r/Utils/PrintHost.cpp | 24 +++++- src/slic3r/Utils/PrintHost.hpp | 2 + src/slic3r/Utils/UltiMaker.cpp | 9 --- src/slic3r/Utils/UltiMaker.hpp | 1 - tests/slic3rutils/CMakeLists.txt | 1 + tests/slic3rutils/test_printhost.cpp | 103 ++++++++++++++++++++++++ tests/slic3rutils/test_user_manager.cpp | 23 ++++++ 16 files changed, 184 insertions(+), 61 deletions(-) create mode 100644 tests/slic3rutils/test_user_manager.cpp diff --git a/src/slic3r/GUI/UserManager.cpp b/src/slic3r/GUI/UserManager.cpp index e874c2158d..f181f3e568 100644 --- a/src/slic3r/GUI/UserManager.cpp +++ b/src/slic3r/GUI/UserManager.cpp @@ -31,14 +31,15 @@ int UserManager::parse_json(std::string payload) { bool restored_json = false; json j; - json j_pre = json::parse(payload); - if (j_pre.empty()) { - return -1; - } //bind/unbind try { + json j_pre = json::parse(payload); + if (j_pre.empty()) { + return -1; + } + if (j_pre.contains("bind")) { if (j_pre["bind"].contains("command")) { diff --git a/src/slic3r/Utils/Duet.cpp b/src/slic3r/Utils/Duet.cpp index 37db6b6c33..e26a493131 100644 --- a/src/slic3r/Utils/Duet.cpp +++ b/src/slic3r/Utils/Duet.cpp @@ -274,13 +274,4 @@ bool Duet::start_print(wxString &msg, const std::string &filename, ConnectionTyp return res; } -int Duet::get_err_code_from_body(const std::string &body) const -{ - pt::ptree root; - std::istringstream iss (body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } diff --git a/src/slic3r/Utils/Duet.hpp b/src/slic3r/Utils/Duet.hpp index 2a91aa8536..7808c51fde 100644 --- a/src/slic3r/Utils/Duet.hpp +++ b/src/slic3r/Utils/Duet.hpp @@ -40,7 +40,6 @@ private: ConnectionType connect(wxString &msg) const; void disconnect(ConnectionType connectionType) const; bool start_print(wxString &msg, const std::string &filename, ConnectionType connectionType, bool simulationMode) const; - int get_err_code_from_body(const std::string &body) const; }; } diff --git a/src/slic3r/Utils/ESP3D.cpp b/src/slic3r/Utils/ESP3D.cpp index 6de41ebe55..5c10862118 100644 --- a/src/slic3r/Utils/ESP3D.cpp +++ b/src/slic3r/Utils/ESP3D.cpp @@ -146,15 +146,6 @@ bool ESP3D::start_print(wxString& msg, const std::string& filename) const return ret; } -int ESP3D::get_err_code_from_body(const std::string& body) const -{ - pt::ptree root; - std::istringstream iss(body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - // ESP3D only accepts 8.3 filenames else it crashes marlin and other undefined behaviour std::string ESP3D::get_short_name(const std::string& filename) const { diff --git a/src/slic3r/Utils/ESP3D.hpp b/src/slic3r/Utils/ESP3D.hpp index 7ac3c66f48..44d206d238 100644 --- a/src/slic3r/Utils/ESP3D.hpp +++ b/src/slic3r/Utils/ESP3D.hpp @@ -33,7 +33,6 @@ private: std::string m_console_port; bool start_print(wxString& msg, const std::string& filename) const; - int get_err_code_from_body(const std::string& body) const; std::string get_short_name(const std::string& filename) const; std::string format_command(const std::string& path, const std::string& arg, const std::string& val) const; }; diff --git a/src/slic3r/Utils/Flashforge.cpp b/src/slic3r/Utils/Flashforge.cpp index 88b3eb3f69..a042a889ae 100644 --- a/src/slic3r/Utils/Flashforge.cpp +++ b/src/slic3r/Utils/Flashforge.cpp @@ -510,12 +510,22 @@ bool Flashforge::fetch_material_slots(std::vector& slots if (!request_local_api_json("detail", json{{"serialNumber", m_serial_number}, {"checkCode", m_check_code}}.dump(), body, msg)) return false; - const auto parsed = json::parse(body, nullptr, false, true); - if (parsed.is_discarded()) { + if (!parse_material_slots(body, slots, supports_material_station)) { msg = _(L("Flashforge returned an invalid JSON response.")); return false; } + return true; +} + +bool Flashforge::parse_material_slots(const std::string& body, std::vector& slots, bool* supports_material_station) +{ + slots.clear(); + + const auto parsed = json::parse(body, nullptr, false, true); + if (parsed.is_discarded()) + return false; + const auto& detail = parsed.contains("detail") ? parsed["detail"] : parsed; const auto& station = detail.contains("matlStationInfo") ? detail["matlStationInfo"] : detail.contains("MatlStationInfo") ? detail["MatlStationInfo"] : json(); @@ -542,12 +552,21 @@ bool Flashforge::fetch_material_slots(std::vector& slots if (supports_material_station != nullptr) *supports_material_station = reports_material_station; + // Fields are read leniently: firmware may send numbers as strings or flags as numbers. for (const auto& slot : slot_infos) { + if (!slot.is_object()) + continue; FlashforgeMaterialSlot info; - info.slot_id = slot.value("slotId", static_cast(slots.size()) + 1); - info.has_filament = slot.value("hasFilament", false); - info.material_name = slot.value("materialName", std::string()); - info.material_color = slot.value("materialColor", std::string()); + info.slot_id = static_cast(slots.size()) + 1; + if (const auto it = slot.find("slotId"); it != slot.end()) + try_parse_json_int(*it, info.slot_id); + int has_filament = 0; + if (const auto it = slot.find("hasFilament"); it != slot.end() && try_parse_json_int(*it, has_filament)) + info.has_filament = has_filament != 0; + if (const auto it = slot.find("materialName"); it != slot.end() && it->is_string()) + info.material_name = it->get(); + if (const auto it = slot.find("materialColor"); it != slot.end() && it->is_string()) + info.material_color = it->get(); slots.emplace_back(std::move(info)); } @@ -670,13 +689,4 @@ std::string Flashforge::extract_host_name() const return out; } -int Flashforge::get_err_code_from_body(const std::string& body) const -{ - pt::ptree root; - std::istringstream iss(body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } // namespace Slic3r diff --git a/src/slic3r/Utils/Flashforge.hpp b/src/slic3r/Utils/Flashforge.hpp index ea7acdcb11..237cf4c009 100644 --- a/src/slic3r/Utils/Flashforge.hpp +++ b/src/slic3r/Utils/Flashforge.hpp @@ -45,6 +45,8 @@ public: PrintHostPostUploadActions get_post_upload_actions() const override { return PrintHostPostUploadAction::StartPrint; } std::string get_host() const override { return m_host; } bool fetch_material_slots(std::vector& slots, bool* supports_material_station, wxString& msg) const; + // Parses a local API "detail" reply. Returns false when the body is not valid JSON. + static bool parse_material_slots(const std::string& body, std::vector& slots, bool* supports_material_station); static bool discover_printers(std::vector& printers, wxString& msg, int timeout_ms = 10000, int idle_timeout_ms = 1500, int max_retries = 3); private: @@ -68,7 +70,6 @@ private: bool request_local_api_json(const std::string& path, const std::string& body, std::string& response_body, wxString& error_msg) const; std::string make_http_url(const std::string& path) const; std::string extract_host_name() const; - int get_err_code_from_body(const std::string &body) const; bool connect(wxString& msg) const; bool start_print(wxString& msg, const std::string& filename) const; }; diff --git a/src/slic3r/Utils/MKS.cpp b/src/slic3r/Utils/MKS.cpp index c4aa4c7262..ced7af9c9f 100644 --- a/src/slic3r/Utils/MKS.cpp +++ b/src/slic3r/Utils/MKS.cpp @@ -141,13 +141,4 @@ bool MKS::start_print(wxString& msg, const std::string& filename) const return ret; } -int MKS::get_err_code_from_body(const std::string& body) const -{ - pt::ptree root; - std::istringstream iss(body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } // Slic3r diff --git a/src/slic3r/Utils/MKS.hpp b/src/slic3r/Utils/MKS.hpp index 79143fdd9a..01cdafc405 100644 --- a/src/slic3r/Utils/MKS.hpp +++ b/src/slic3r/Utils/MKS.hpp @@ -34,7 +34,6 @@ private: std::string get_upload_url(const std::string& filename) const; bool start_print(wxString& msg, const std::string& filename) const; - int get_err_code_from_body(const std::string& body) const; }; } diff --git a/src/slic3r/Utils/PrintHost.cpp b/src/slic3r/Utils/PrintHost.cpp index f3333c731d..41241abb10 100644 --- a/src/slic3r/Utils/PrintHost.cpp +++ b/src/slic3r/Utils/PrintHost.cpp @@ -3,10 +3,13 @@ #include #include #include +#include #include #include #include #include +#include +#include #include #include @@ -172,6 +175,20 @@ std::string moonraker_error_reason(const std::string &body) } // namespace +int PrintHost::get_err_code_from_body(const std::string &body) +{ + boost::property_tree::ptree root; + std::istringstream iss(body); + try { + boost::property_tree::read_json(iss, root); + } catch (const std::exception &ex) { + BOOST_LOG_TRIVIAL(error) << "PrintHost: response is not valid JSON: " << ex.what(); + return -1; + } + + return root.get("err", 0); +} + wxString PrintHost::format_error(const std::string &body, const std::string &error, unsigned status) const { if (status != 0) { @@ -303,7 +320,12 @@ void PrintHostJobQueue::priv::bg_thread_main() % job.cancelled; if (! job.cancelled) { - perform_job(std::move(job)); + // A failing job must not stop the worker, or later jobs would stay queued forever. + try { + perform_job(std::move(job)); + } catch (const std::exception &e) { + emit_error(e.what()); + } } remove_source(); diff --git a/src/slic3r/Utils/PrintHost.hpp b/src/slic3r/Utils/PrintHost.hpp index 87a8df8934..ed86f213e7 100644 --- a/src/slic3r/Utils/PrintHost.hpp +++ b/src/slic3r/Utils/PrintHost.hpp @@ -87,6 +87,8 @@ public: static PrintHost* get_print_host(DynamicPrintConfig *config); static std::string get_print_host_webui(DynamicPrintConfig *config); + // Reads the "err" field of a JSON reply, 0 when absent. Returns -1 when the body is not valid JSON. + static int get_err_code_from_body(const std::string &body); //Support for cloud webui login virtual bool is_cloud() const { return false; } diff --git a/src/slic3r/Utils/UltiMaker.cpp b/src/slic3r/Utils/UltiMaker.cpp index 0376578db0..badb69d209 100644 --- a/src/slic3r/Utils/UltiMaker.cpp +++ b/src/slic3r/Utils/UltiMaker.cpp @@ -654,13 +654,4 @@ bool UltiMaker::start_print(wxString &msg, const std::string &filename, Connecti return res; } -int UltiMaker::get_err_code_from_body(const std::string &body) const -{ - pt::ptree root; - std::istringstream iss (body); // wrap returned json to istringstream - pt::read_json(iss, root); - - return root.get("err", 0); -} - } diff --git a/src/slic3r/Utils/UltiMaker.hpp b/src/slic3r/Utils/UltiMaker.hpp index b32b87434f..c1f9ced304 100644 --- a/src/slic3r/Utils/UltiMaker.hpp +++ b/src/slic3r/Utils/UltiMaker.hpp @@ -64,7 +64,6 @@ private: void set_auth(Http& http) const; void disconnect(ConnectionType connectionType) const; bool start_print(wxString &msg, const std::string &filename, ConnectionType connectionType) const; - int get_err_code_from_body(const std::string &body) const; }; } diff --git a/tests/slic3rutils/CMakeLists.txt b/tests/slic3rutils/CMakeLists.txt index 94ebaed35e..a3863134d5 100644 --- a/tests/slic3rutils/CMakeLists.txt +++ b/tests/slic3rutils/CMakeLists.txt @@ -28,6 +28,7 @@ add_executable(${_TEST_NAME}_tests test_plugin_audit.cpp test_shortcuts.cpp test_file_url.cpp + test_user_manager.cpp ../fff_print/test_helpers.cpp ) diff --git a/tests/slic3rutils/test_printhost.cpp b/tests/slic3rutils/test_printhost.cpp index 557d4a5c89..a102d83770 100644 --- a/tests/slic3rutils/test_printhost.cpp +++ b/tests/slic3rutils/test_printhost.cpp @@ -1,8 +1,12 @@ #include +#include +#include + #include #include "slic3r/Utils/PrintHost.hpp" +#include "slic3r/Utils/Flashforge.hpp" using namespace Slic3r; @@ -46,6 +50,14 @@ std::string moonraker_error(int code, const std::string& message, const std::str constexpr const char* k_busy_file_403 = R"JSON({"error": {"code": 403, "message": "Forbidden", "traceback": "Traceback (most recent call last):\n\n File \"/home/lava/moonraker/moonraker/components/file_manager/file_manager.py\", line 1017, in _finish_gcode_upload\n can_start = self._handle_operation_check(check_path)\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\nmoonraker.utils.exceptions.ServerError: File currently in use\n\nDuring handling of the above exception, another exception occurred:\n\nTraceback (most recent call last):\n\n File \"/home/lava/moonraker/moonraker/components/application.py\", line 1069, in post\n raise tornado.web.HTTPError(\ntornado.web.HTTPError: HTTP 403: Forbidden (File is loaded, upload not permitted)\n"}})JSON"; +// Replies a print host can send instead of JSON: a proxy or login page, nothing, a cut-off body. +const std::vector non_json_replies = { + "proxy login required", + "", + "{\"err\":", + "{\"detail\":{\"matlStationInfo\":{\"slotInfos\":[{\"slotId\":1,", +}; + } // namespace TEST_CASE("A Klipper upload error shows its reason instead of a Python traceback", "[PrintHost][Regression]") @@ -211,3 +223,94 @@ TEST_CASE("Error bodies that are not a Moonraker envelope are left unchanged", " CHECK(format_error("", "curl:Could not connect", 0) == "curl:Could not connect"); } } + +TEST_CASE("Print host error code is read from a JSON reply", "[PrintHost]") +{ + CHECK(PrintHost::get_err_code_from_body(R"({"err":0})") == 0); + CHECK(PrintHost::get_err_code_from_body(R"({"err":2})") == 2); + CHECK(PrintHost::get_err_code_from_body(R"({"status":"ok"})") == 0); +} + +TEST_CASE("Print host error code reports a reply that is not JSON as an error", "[PrintHost]") +{ + const std::string body = GENERATE(from_range(non_json_replies)); + int err = 0; + REQUIRE_NOTHROW(err = PrintHost::get_err_code_from_body(body)); + CHECK(err != 0); +} + +TEST_CASE("Print host error code tolerates a wrongly typed err field", "[PrintHost]") +{ + const std::string body = GENERATE(as{}, R"({"err":"busy"})", R"({"err":{"code":1}})", R"([1,2])"); + CHECK_NOTHROW(PrintHost::get_err_code_from_body(body)); +} + +TEST_CASE("Flashforge material slots are read from a well-formed reply", "[PrintHost][Flashforge]") +{ + const std::string body = R"({"code":0,"detail":{"hasMatlStation":true,"matlStationInfo":{"slotCnt":2,"slotInfos":[ + {"slotId":1,"hasFilament":true,"materialName":"PLA","materialColor":"#FFFFFF"}, + {"slotId":2,"hasFilament":false,"materialName":"","materialColor":""}]}}})"; + + std::vector slots; + bool supports_station = false; + REQUIRE(Flashforge::parse_material_slots(body, slots, &supports_station)); + CHECK(supports_station); + REQUIRE(slots.size() == 2); + CHECK(slots[0].slot_id == 1); + CHECK(slots[0].has_filament); + CHECK(slots[0].material_name == "PLA"); + CHECK(slots[0].material_color == "#FFFFFF"); + CHECK(slots[1].slot_id == 2); + CHECK_FALSE(slots[1].has_filament); +} + +TEST_CASE("Flashforge material slots accept numbers as strings and flags as numbers", "[PrintHost][Flashforge]") +{ + const std::string body = R"({"detail":{"matlStationInfo":{"slotInfos":[ + {"slotId":"3","hasFilament":1,"materialName":null,"materialColor":7}]}}})"; + + std::vector slots; + REQUIRE_NOTHROW(Flashforge::parse_material_slots(body, slots, nullptr)); + REQUIRE(slots.size() == 1); + CHECK(slots[0].slot_id == 3); + CHECK(slots[0].has_filament); + CHECK(slots[0].material_name.empty()); + CHECK(slots[0].material_color.empty()); +} + +TEST_CASE("Flashforge material slots skip entries that are not objects", "[PrintHost][Flashforge]") +{ + const std::string body = R"({"detail":{"matlStationInfo":{"slotInfos":[5,"slot",null,[], + {"slotId":4,"hasFilament":true,"materialName":"PETG"}]}}})"; + + std::vector slots; + REQUIRE_NOTHROW(Flashforge::parse_material_slots(body, slots, nullptr)); + REQUIRE(slots.size() == 1); + CHECK(slots[0].slot_id == 4); + CHECK(slots[0].material_name == "PETG"); +} + +TEST_CASE("Flashforge material slots tolerate slot info that is not a list", "[PrintHost][Flashforge]") +{ + const std::string body = GENERATE(as{}, + R"({"detail":{"matlStationInfo":{"slotInfos":5}}})", + R"({"detail":{"matlStationInfo":{"slotInfos":"none"}}})", + R"({"detail":{"matlStationInfo":7}})", + R"({"detail":"offline"})"); + + std::vector slots; + bool ok = false; + REQUIRE_NOTHROW(ok = Flashforge::parse_material_slots(body, slots, nullptr)); + CHECK(ok); + CHECK(slots.empty()); +} + +TEST_CASE("Flashforge material slots reject a reply that is not JSON", "[PrintHost][Flashforge]") +{ + const std::string body = GENERATE(from_range(non_json_replies)); + std::vector slots; + bool ok = true; + REQUIRE_NOTHROW(ok = Flashforge::parse_material_slots(body, slots, nullptr)); + CHECK_FALSE(ok); + CHECK(slots.empty()); +} diff --git a/tests/slic3rutils/test_user_manager.cpp b/tests/slic3rutils/test_user_manager.cpp new file mode 100644 index 0000000000..cfccebe1cc --- /dev/null +++ b/tests/slic3rutils/test_user_manager.cpp @@ -0,0 +1,23 @@ +#include + +#include + +#include "slic3r/GUI/UserManager.hpp" + +using namespace Slic3r; + +TEST_CASE("User message that is not JSON is rejected without throwing", "[UserManager]") +{ + const std::string payload = GENERATE(as{}, "not json", "", "", "{\"bind\":"); + UserManager manager; + int result = 0; + REQUIRE_NOTHROW(result = manager.parse_json(payload)); + CHECK(result == -1); +} + +TEST_CASE("User message without a successful bind is ignored", "[UserManager]") +{ + const std::string payload = GENERATE(as{}, "{}", R"({"bind":{"command":"unbind"}})", R"({"bind":"bind"})", "[1]"); + UserManager manager; + CHECK(manager.parse_json(payload) == -1); +}