From 3fd5c3353a6c59c75e69c32e7b33b72c3a825e26 Mon Sep 17 00:00:00 2001 From: Tommaso Bianchi Date: Mon, 24 Aug 2026 19:00:45 +0200 Subject: [PATCH] A reference is a reference whatever feature holds it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port of snaporca 1bb9825db0. Four defects from an independent 20-agent audit, each verified in the code first; two further findings from the same report were verified OUT and are not in this commit. remove_feature()/move_feature() remapped Extrude::sketch_ref and a Mate's two connectors and nothing else, leaving seven of the nine index-bearing fields — sweep_path_ref, loft_profile_refs[], pattern_curve_sketch, rib_sketch_ref, and sketch_ref on Revolve, Sweep, Rib and the Surface* family — pointing at whatever slid into the slot. Quiet by construction: the shifted index still names a real feature, recompute() succeeds, the solid is built from the wrong profile. The comment above the loop already required "EVERY field holding a feature index"; the code under it handled two, because a type switch is only correct on the day it is written. for_each_feature_ref() visits the FIELDS instead, so a feature type added later is covered the moment it reuses one. plane_base and axis_plane_a/b are excluded on purpose and documented at the helper — they encode an ordinal into the datum-plane list, not an index into features[], and are filed separately. The delete cascade got the same field-based treatment. The regression test was run against the pre-fix code to prove it bites: all three sections fail there, and move_feature returns TRUE while leaving sketch_ref == 1 where it must be 0 — success with the wrong answer, which is what makes this class expensive. apply_constraint, commit_entity_constraints and delete_constraint mutated the recipe with no checkpoint() and no sync_recipe_to_model(), alone among seventeen mutation sites in that file: Ctrl+Z reached past the constraint edit and discarded unrelated work, and saving persisted the pre-constraint blob. A rejected constraint now calls abandon_checkpoint() rather than leaving an undo step that does nothing. MCP: params["generation"].get() sat outside the try inside a bare CallAfter lambda, so one malformed string terminated the process through the wx event loop; it is type-checked now and the lambda lets nothing escape. The socket bound with no mode of its own in a world-writable directory — umask around bind() plus chmod, and it refuses to listen rather than listen wide. The reply write is no longer a bare write(), which could SIGPIPE the app when a client hung up. Kernel suite on this fork: 190 cases / 2562 assertions, green. GUI target compiles. The full ladder gate ran on snaporca (ALL LADDERS HELD — gestures 98/98, offer 108/108, corpus and corpus-scale green) and fork-check parity holds at 17 identical / 8 diverging as expected, which is what makes that gate transferable here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MrMzTpAf78U4NG2M8jfvHY --- src/libslic3r/CAD/CadDocument.cpp | 70 +++++++++++++++++++-------- src/libslic3r/CAD/CadDocument.hpp | 5 ++ src/slic3r/GUI/CAD/DesignPanel.cpp | 16 +++++++ src/slic3r/GUI/CAD/McpControl.cpp | 48 +++++++++++++++++-- tests/libslic3r/test_caddocument.cpp | 72 ++++++++++++++++++++++++++++ 5 files changed, 188 insertions(+), 23 deletions(-) diff --git a/src/libslic3r/CAD/CadDocument.cpp b/src/libslic3r/CAD/CadDocument.cpp index 42c49b4d91..64cdadddfe 100644 --- a/src/libslic3r/CAD/CadDocument.cpp +++ b/src/libslic3r/CAD/CadDocument.cpp @@ -1859,6 +1859,12 @@ void CadDocument::checkpoint() m_undo.erase(m_undo.begin()); } +void CadDocument::abandon_checkpoint() +{ + if (!m_undo.empty()) + m_undo.pop_back(); +} + bool CadDocument::undo() { if (m_undo.empty()) @@ -1915,6 +1921,33 @@ static bool commit_or_rollback(CadDocument& doc, std::vector& snapsh return false; } +// Visit every field of `f` that holds an index into features[]. +// +// Deliberately NOT dispatched on f.type. A type switch is what this replaced, and it was +// wrong in the way type switches go wrong: it listed Extrude and Mate, and every feature +// type added afterwards — Revolve, Sweep, Loft, Rib, Pattern-on-curve, the Surface* family — +// silently inherited a remap that skipped its references. Visiting the FIELDS instead means +// a new type is covered the moment it reuses one of them, and reaching a field its type +// never reads costs nothing: unset refs are -1 and every visitor here ignores those. +// +// The list is exactly the fields documented as "index into features[]" / "feature index" in +// CadDocument.hpp. Not included, because they are a different basis and need their own pass: +// plane_base / axis_plane_a / axis_plane_b encode `3 + N` = the Nth DATUM PLANE, an ordinal +// into resolve_datum_planes(), not into features[]. Everything named *_body / *_face / *_edge +// is a body index or a global topology id and must never be remapped here. +template +static void for_each_feature_ref(CadFeature& f, Visit&& visit) +{ + visit(f.sketch_ref); + visit(f.sweep_path_ref); + visit(f.pattern_curve_sketch); + visit(f.rib_sketch_ref); + visit(f.mate_cs_a); + visit(f.mate_cs_b); + for (int& r : f.loft_profile_refs) + visit(r); +} + bool CadDocument::remove_feature(int index) { if (index < 0 || index >= int(features.size())) @@ -1922,13 +1955,22 @@ bool CadDocument::remove_feature(int index) std::vector snapshot = features; - // Deleting a Sketch cascades to every Extrude that consumes it (a dangling - // Extrude would have no wire). A lone Sketch, by contrast, is harmless. + // Deleting a Sketch cascades to every feature that consumes it AS ITS PROFILE — the + // reason is the original one ("a dangling Extrude would have no wire"), and it applies + // unchanged to Revolve, Sweep, Rib and the Surface* family, which all read the profile + // through sketch_ref, plus the sweep spine and the rib line. Testing the FIELD rather + // than the type is what makes that true without a list to keep up to date. + // + // pattern_curve_sketch and loft_profile_refs are deliberately NOT cascaded: a Pattern or + // a Loft that loses one of several inputs is degraded, not meaningless, so those refs go + // to -1 in the remap below and the feature survives. A lone Sketch is harmless either way. std::vector remove{index}; if (features[index].type == CadFeatureType::Sketch) { - for (int j = 0; j < int(features.size()); ++j) - if (features[j].type == CadFeatureType::Extrude && features[j].sketch_ref == index) + for (int j = 0; j < int(features.size()); ++j) { + const CadFeature& c = features[j]; + if (c.sketch_ref == index || c.sweep_path_ref == index || c.rib_sketch_ref == index) remove.push_back(j); + } } std::sort(remove.begin(), remove.end()); remove.erase(std::unique(remove.begin(), remove.end()), remove.end()); @@ -1956,14 +1998,8 @@ bool CadDocument::remove_feature(int index) ref -= shift; } }; - for (auto& f : features) { - if (f.type == CadFeatureType::Extrude) { - remap(f.sketch_ref); - } else if (f.type == CadFeatureType::Mate) { - remap(f.mate_cs_a); - remap(f.mate_cs_b); - } - } + for (auto& f : features) + for_each_feature_ref(f, remap); return commit_or_rollback(*this, snapshot); } @@ -1985,14 +2021,8 @@ bool CadDocument::move_feature(int index, int delta) if (ref == index) ref = target; else if (ref == target) ref = index; }; - for (auto& f : features) { - if (f.type == CadFeatureType::Extrude) { - swap_ref(f.sketch_ref); - } else if (f.type == CadFeatureType::Mate) { - swap_ref(f.mate_cs_a); - swap_ref(f.mate_cs_b); - } - } + for (auto& f : features) + for_each_feature_ref(f, swap_ref); return commit_or_rollback(*this, snapshot); } diff --git a/src/libslic3r/CAD/CadDocument.hpp b/src/libslic3r/CAD/CadDocument.hpp index c97f220f9f..a0ad27a163 100644 --- a/src/libslic3r/CAD/CadDocument.hpp +++ b/src/libslic3r/CAD/CadDocument.hpp @@ -670,6 +670,11 @@ public: // body) is derived by recompute(), snapshotting `features` alone is a complete, // exact history; one checkpoint == one Ctrl+Z step. void checkpoint(); // snapshot `features` for undo + invalidate redo + // Drop the most recent checkpoint. For a mutation that took a checkpoint, then failed + // and restored the pre-mutation state itself (the constraint paths reject an + // over-constrained addition this way): the snapshot now describes a state identical to + // the current one, and leaving it turns the next Ctrl+Z into a press that does nothing. + void abandon_checkpoint(); bool can_undo() const { return !m_undo.empty(); } bool can_redo() const { return !m_redo.empty(); } size_t undo_depth() const { return m_undo.size(); } diff --git a/src/slic3r/GUI/CAD/DesignPanel.cpp b/src/slic3r/GUI/CAD/DesignPanel.cpp index a1e17f996f..1287dc1e35 100644 --- a/src/slic3r/GUI/CAD/DesignPanel.cpp +++ b/src/slic3r/GUI/CAD/DesignPanel.cpp @@ -7850,16 +7850,25 @@ void DesignPanel::commit_entity_constraints(const std::vector saved = feat.entities; const size_t before = feat.entity_constraints.size(); + // Undo boundary: adding a constraint. Without it Ctrl+Z reached PAST this edit to the + // previous boundary and threw away whatever happened in between — a constraint was the + // one document mutation the user could not take back on its own. + m_doc.checkpoint(); for (const auto& d : defs) feat.entity_constraints.push_back(d); if (!m_doc.solve_sketch_feature(m_constrain_feat)) { feat.entity_constraints.resize(before); feat.entities = saved; + m_doc.abandon_checkpoint(); // fully restored above: nothing happened, so nothing to undo m_status->SetForegroundColour(wxColour(235, 110, 110)); set_status(_L("Constraint rejected (over-constrained)")); m_status->Refresh(); return; } m_doc.recompute(); + // The constraint lives in the recipe, so the save path has to be told the recipe moved; + // otherwise saving after a constraint edit wrote the blob from before it. + sync_recipe_to_model(); + update_undo_redo_buttons(); m_viewport->update_constrain_entities(m_doc.features[m_constrain_feat].entities); if (!m_doc.display_mesh.its.indices.empty()) feed_bodies(); @@ -8029,11 +8038,14 @@ void DesignPanel::delete_constraint(int idx) CadFeature& feat = m_doc.features[m_constrain_feat]; if (idx < 0 || idx >= int(feat.entity_constraints.size())) return; + m_doc.checkpoint(); // undo boundary: deleting a constraint feat.entity_constraints.erase(feat.entity_constraints.begin() + idx); // Re-solve the remaining system (deleting a constraint can only free DoF, so it // cannot fail for over-constraint; ignore the bool and refresh either way). m_doc.solve_sketch_feature(m_constrain_feat); m_doc.recompute(); + sync_recipe_to_model(); // the removal is part of the recipe + update_undo_redo_buttons(); m_viewport->set_constraint_highlight({}); m_viewport->update_constrain_entities(m_doc.features[m_constrain_feat].entities); if (!m_doc.display_mesh.its.indices.empty()) @@ -8867,16 +8879,20 @@ void DesignPanel::apply_constraint(SketchConstraintType type) // solve_sketch_feature rewrites profile.points even on failure, so snapshot // the geometry to roll back a rejected constraint cleanly. const std::vector saved_pts = feat.profile.points; + m_doc.checkpoint(); // undo boundary: adding a constraint (profile sketches) feat.constraints.push_back(SketchConstraintDef{type, a, b, -1, -1, 0.0}); if (!m_doc.solve_sketch_feature(m_constrain_feat)) { feat.constraints.pop_back(); // reject the non-converging addition feat.profile.points = saved_pts; // and restore the pre-solve geometry + m_doc.abandon_checkpoint(); // restored: no state change, so no undo step m_status->SetForegroundColour(wxColour(235, 110, 110)); set_status(_L("Constraint rejected (over-constrained)")); m_status->Refresh(); return; } m_doc.recompute(); + sync_recipe_to_model(); + update_undo_redo_buttons(); m_viewport->update_constrain_profile(m_doc.features[m_constrain_feat].profile.points); if (!m_doc.display_mesh.its.indices.empty()) feed_bodies(); diff --git a/src/slic3r/GUI/CAD/McpControl.cpp b/src/slic3r/GUI/CAD/McpControl.cpp index c759511b2a..b1bd667714 100644 --- a/src/slic3r/GUI/CAD/McpControl.cpp +++ b/src/slic3r/GUI/CAD/McpControl.cpp @@ -4,6 +4,7 @@ #include #include +#include // umask/chmod: the socket's file mode IS its access control #include #include #include @@ -2008,7 +2009,14 @@ std::string handle_on_main(const std::string& method, const json& params, const // keeps every existing script working exactly as before, and supplying it is what buys the // guarantee. One check at the dispatcher rather than one per handler, so a method added // later cannot forget it. + // + // The type check is not decoration: this runs OUTSIDE the try below, and a bare + // get() on `"generation": "x"` throws nlohmann::type_error straight out of + // the CallAfter lambda that invoked us — through a wx event loop, which does not catch, + // so the whole GUI went down on one malformed line. Refuse it as a parameter error. if (params.is_object() && params.contains("generation")) { + if (!params["generation"].is_number_unsigned()) + return rpc_error(id, -32602, "generation must be an unsigned integer"); const uint64_t want = params["generation"].get(); const uint64_t have = panel->mcp_doc().topo_generation; if (want != have) @@ -2099,8 +2107,18 @@ std::string dispatch_request(const std::string& line) auto prom = std::make_shared>(); auto fut = prom->get_future(); + // Nothing may escape this lambda. It is invoked by the wx event loop, which has no + // handler of its own, so an escaping exception is std::terminate — the socket would + // become a way for any client to kill the application. handle_on_main() catches what + // it knows about; this catches what it does not, and still answers the caller. wxGetApp().CallAfter([prom, method, params, id]() { - prom->set_value(handle_on_main(method, params, id)); + try { + prom->set_value(handle_on_main(method, params, id)); + } catch (const std::exception& ex) { + prom->set_value(rpc_error(id, -32000, std::string("internal error: ") + ex.what())); + } catch (...) { + prom->set_value(rpc_error(id, -32000, "internal error: unknown exception")); + } }); if (fut.wait_for(std::chrono::seconds(15)) != std::future_status::ready) return rpc_error(id, -32000, "main-thread timeout"); @@ -2123,7 +2141,14 @@ void serve_client(int cfd) if (line.empty()) continue; std::string reply = dispatch_request(line); reply.push_back('\n'); - if (::write(cfd, reply.data(), reply.size()) < 0) return; + // Never a bare write(): a client that hangs up between its request and our + // reply raises SIGPIPE, whose default action kills the process — so closing + // a socket mid-call would take the GUI with it. +#ifdef MSG_NOSIGNAL + if (::send(cfd, reply.data(), reply.size(), MSG_NOSIGNAL) < 0) return; +#else + if (::write(cfd, reply.data(), reply.size()) < 0) return; // SO_NOSIGPIPE set at accept +#endif } } } @@ -2137,16 +2162,33 @@ void server_thread(std::string sock_path) sockaddr_un addr{}; addr.sun_family = AF_UNIX; std::strncpy(addr.sun_path, sock_path.c_str(), sizeof(addr.sun_path) - 1); - if (::bind(sfd, reinterpret_cast(&addr), sizeof(addr)) < 0) { + // The socket is the full CAD command surface, including import_step on absolute paths. + // It lands in a world-writable directory by default (/tmp), so its access control is + // its file mode and nothing else — leaving that to the ambient umask means any local + // process may drive the modeller. umask around bind() makes it 0600 with no window in + // which a wider mode exists; the chmod afterwards covers platforms that do not apply + // umask to sockets. + const mode_t old_umask = ::umask(0177); + const int bind_rc = ::bind(sfd, reinterpret_cast(&addr), sizeof(addr)); + ::umask(old_umask); + if (bind_rc < 0) { BOOST_LOG_TRIVIAL(error) << "MCP: bind() failed on " << sock_path; ::close(sfd); return; } + if (::chmod(sock_path.c_str(), S_IRUSR | S_IWUSR) < 0) { + BOOST_LOG_TRIVIAL(error) << "MCP: cannot restrict " << sock_path << " to the owner; refusing to listen"; + ::close(sfd); ::unlink(sock_path.c_str()); return; + } if (::listen(sfd, 1) < 0) { BOOST_LOG_TRIVIAL(error) << "MCP: listen() failed"; ::close(sfd); return; } BOOST_LOG_TRIVIAL(info) << "MCP control listening on " << sock_path; for (;;) { int cfd = ::accept(sfd, nullptr, nullptr); if (cfd < 0) continue; +#if !defined(MSG_NOSIGNAL) && defined(SO_NOSIGPIPE) + const int on = 1; // macOS/BSD equivalent of MSG_NOSIGNAL + ::setsockopt(cfd, SOL_SOCKET, SO_NOSIGPIPE, &on, sizeof(on)); +#endif serve_client(cfd); ::close(cfd); } diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index a9e4ccbcaa..ccd54c671d 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -8145,3 +8145,75 @@ TEST_CASE("a body carries its own name, through recompute and the recipe", "[Cad CHECK(fresh.bodies[0].has_user_name); CHECK(fresh.bodies[0].user_name == "Bracket"); } + +// A feature reference is a reference whatever feature holds it. +// +// remove_feature()/move_feature() used to remap Extrude::sketch_ref and a Mate's two +// connectors, and nothing else — so every later consumer of a Sketch (Revolve, Sweep, Loft, +// Rib, Pattern-on-curve, the Surface* family) kept an index that the erase had just +// invalidated. The failure is quiet by construction: a shifted index still names a real +// feature, recompute() succeeds, and what comes out is built from the wrong profile. +// +// The volume is the assertion. Two different profiles go in; deleting the unused feature in +// front of them must leave the SAME solid behind, which it only can if the reference moved +// with its target. +TEST_CASE("deleting a feature remaps the references of every consumer, not just Extrude", + "[CadDocument]") +{ + using namespace Slic3r; + const SketchPlane xy = SketchPlane::XY(); + + // A rectangle 10x10 centred at v = 15, revolved 360 deg about the plane X axis. + auto rect_at = [&](double v0) { + CadFeature sk; + sk.type = CadFeatureType::Sketch; + sk.plane = xy; + sk.profile.points = { Vec2d(-5, v0 - 5), Vec2d(5, v0 - 5), + Vec2d(5, v0 + 5), Vec2d(-5, v0 + 5) }; + sk.profile.closed = true; + return sk; + }; + + SECTION("Revolve::sketch_ref survives the deletion of a feature in front of it") { + CadDocument doc; + doc.features.push_back(rect_at(40.0)); // f0: a decoy profile, never consumed + doc.features.push_back(rect_at(15.0)); // f1: the real profile + doc.add_revolve(1, 360.0, /*axis=X*/0, false, BooleanMode::New, "Rev"); + REQUIRE(doc.recompute()); + const double before = double(doc.display_mesh.volume()); + REQUIRE(before == Approx(9424.78).epsilon(0.05)); + + // f0 is consumed by nothing, so this deletes exactly one feature and shifts f1 to 0. + REQUIRE(doc.remove_feature(0)); + REQUIRE(doc.features.size() == 2); + REQUIRE(doc.features[1].type == CadFeatureType::Revolve); + REQUIRE(doc.features[1].sketch_ref == 0); // followed its target + REQUIRE(doc.error.empty()); + REQUIRE(double(doc.display_mesh.volume()) == Approx(before).epsilon(1e-6)); + } + + SECTION("move_feature swaps a Revolve's reference too") { + CadDocument doc; + doc.features.push_back(rect_at(40.0)); // f0 + doc.features.push_back(rect_at(15.0)); // f1 -> consumed + doc.add_revolve(1, 360.0, 0, false, BooleanMode::New, "Rev"); + REQUIRE(doc.recompute()); + const double before = double(doc.display_mesh.volume()); + + REQUIRE(doc.move_feature(0, 1)); // f0 and f1 trade places + REQUIRE(doc.features[2].sketch_ref == 0); // the profile is now at 0 + REQUIRE(double(doc.display_mesh.volume()) == Approx(before).epsilon(1e-6)); + } + + SECTION("deleting a Sketch cascades to a Revolve that consumes it") { + CadDocument doc; + doc.features.push_back(rect_at(15.0)); + doc.add_revolve(0, 360.0, 0, false, BooleanMode::New, "Rev"); + REQUIRE(doc.recompute()); + + // The Revolve has no profile without this Sketch, exactly as an Extrude would not: + // it goes with it rather than being left pointing at nothing. + REQUIRE(doc.remove_feature(0)); + REQUIRE(doc.features.empty()); + } +}