From 5ac1d05fcbb0c5b32a1f59c82c01a40952290c7b Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Thu, 3 Sep 2026 00:55:58 -0400 Subject: [PATCH] Stop three IMEX changes from altering non-IMEX behaviour None of these came from the review; they were found auditing the branch for the same class of leak review comment 6 identified. physical_extruder_map was normalised through effective_physical_extruder_map on every Print::apply(), so any printer with more than one nozzle and no authored map got the identity [0,1,...,n-1] where the single-element {0} default belongs. The key carries two readings: the IMEX paths index it by logical extruder and need one entry per extruder, while the inherited BBL paths read it through the clamping get_at(), for which {0} means "everything is physical 0". Deriving unconditionally imposed the IMEX reading on profiles that mean the other one, changing the config block line and the {first_tools} / {first_filaments} / {curr_physical_extruder_id} placeholders for multi-nozzle non-IMEX printers. Gated on is_imex; every consumer needing the per-extruder form is already IMEX-gated, and profiles with an authored map of the right length are unaffected either way. That gating unmasked a latent out-of-bounds read: WipeTower's M104/M109 emitters index m_physical_extruder_map by tool with no bounds check, which reads past the end of the single-element default on a multi-nozzle machine. Upstream's bug, from the BambuStudio wipe tower sync, previously hidden because the map was being widened for everyone. Now bounds-checked, falling back to the tool's own index -- the form GCodeProcessor already uses for the same map. Preset::save() and get_preset_differed_for_save() carried a branch storing the full vector whenever a child and its parent had different lengths. It was written against a set_with_nil that threw on mismatched sizes; upstream #13035 replaced that with a tolerant version that keeps the child vector verbatim and nil-marks only the overlapping range, and that fix was already in the tree when this branch was rebased. Left in, it defeated the delta encoding for every user printer preset whose extruder count differs from its parent's: a 7-extruder profile inheriting a single-extruder base wrote out all of its per-variant retraction keys as literals, including ones identical to the parent, pinning them against future vendor updates while the UI still reported the preset as inheriting. Removed; the two save loops are now identical to upstream. Note this only affects new saves -- presets already written keep their frozen values until re-saved. Co-Authored-By: Claude Opus 5 (1M context) --- src/libslic3r/GCode/WipeTower.cpp | 13 +++++++++++-- src/libslic3r/Preset.cpp | 12 ------------ src/libslic3r/PrintApply.cpp | 14 +++++++++++++- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/src/libslic3r/GCode/WipeTower.cpp b/src/libslic3r/GCode/WipeTower.cpp index 5834937169..790c0d9d73 100644 --- a/src/libslic3r/GCode/WipeTower.cpp +++ b/src/libslic3r/GCode/WipeTower.cpp @@ -1350,7 +1350,7 @@ public: buffer += "M400\n"; buffer += "M104"; if (target_extruder != -1) - buffer += (" T" + std::to_string(m_physical_extruder_map[target_extruder])); + buffer += (" T" + std::to_string(physical_extruder_for(target_extruder))); buffer += " S" + std::to_string(target_temp) + " N0"; // N0 means the gcode is generated by slicer if (!comment.empty()) buffer += " ;" + comment; buffer += '\n'; @@ -1362,7 +1362,7 @@ public: { std::string buffer = "M109"; if (target_extruder != -1) - buffer += (" T" + std::to_string(m_physical_extruder_map[target_extruder])); + buffer += (" T" + std::to_string(physical_extruder_for(target_extruder))); buffer += " S" + std::to_string(target_temp) + " N0"; // N0 means the gcode is generated by slicer if (!comment.empty()) buffer += " ;" + comment; buffer += '\n'; @@ -1382,6 +1382,15 @@ public: void set_multi_nozzle_group_result(const MultiNozzleUtils::LayeredNozzleGroupResult *multi_nozzle_group_result) { m_multi_nozzle_group_result = multi_nozzle_group_result; } void set_physical_extruder_map(const std::vector &physical_extruder_map) { m_physical_extruder_map = physical_extruder_map; } + // physical_extruder_map defaults to the single element {0} and is only widened to one entry + // per extruder on IMEX printers, so indexing it by tool is out of range on any other + // multi-extruder machine. Fall back to the tool's own index, matching the bounds-checked + // form GCodeProcessor uses for the same map. + int physical_extruder_for(int tool) const + { + return tool >= 0 && tool < (int) m_physical_extruder_map.size() ? m_physical_extruder_map[tool] : tool; + } + private: std::string set_normal_acceleration() { std::vector accelerations = m_is_first_layer ? m_first_layer_normal_accelerations : m_normal_accelerations; diff --git a/src/libslic3r/Preset.cpp b/src/libslic3r/Preset.cpp index e3ac3a75df..211de76a6c 100644 --- a/src/libslic3r/Preset.cpp +++ b/src/libslic3r/Preset.cpp @@ -733,12 +733,6 @@ void Preset::save(DynamicPrintConfig* parent_config) ConfigOptionVectorBase* opt_vec_inherit = static_cast(parent_config->option(option)); if (opt_vec_src->size() == 1) opt_dst->set(opt_src); - else if (opt_vec_src->size() != opt_vec_inherit->size()) { - // Size mismatch (e.g. new multi-extruder profile inheriting from a - // single-extruder base): nil-delta encoding requires matching sizes, - // so fall back to storing the full vector. - opt_dst->set(opt_src); - } else if (key_set1->find(option) != key_set1->end()) { opt_vec_dst->set_with_nil(opt_vec_src, opt_vec_inherit, 1); } @@ -1905,12 +1899,6 @@ Preset* PresetCollection::get_preset_differed_for_save(Preset& preset) ConfigOptionVectorBase* opt_vec_inherit = static_cast(parent_preset->config.option(option)); if (opt_vec_src->size() == 1) opt_dst->set(opt_src); - else if (opt_vec_src->size() != opt_vec_inherit->size()) { - // Size mismatch (e.g. new multi-extruder profile inheriting from a - // single-extruder base): nil-delta encoding requires matching sizes, - // so fall back to storing the full vector. - opt_dst->set(opt_src); - } else if (key_set1->find(option) != key_set1->end()) { opt_vec_dst->set_with_nil(opt_vec_src, opt_vec_inherit, 1); } diff --git a/src/libslic3r/PrintApply.cpp b/src/libslic3r/PrintApply.cpp index 3d1fc8f16c..579c5e5747 100644 --- a/src/libslic3r/PrintApply.cpp +++ b/src/libslic3r/PrintApply.cpp @@ -1313,7 +1313,19 @@ Print::ApplyStatus Print::apply(const Model &model, DynamicPrintConfig new_full_ // Fill in physical_extruder_map when the printer profile has not authored one. It has one // entry per logical extruder, so it is sized from the nozzle count -- not from // printer_extruder_id, which is indexed by variant slot. - { + // + // IMEX printers only. physical_extruder_map carries two readings in this tree (see + // IMEXHelpers.hpp): the IMEX paths need one entry per logical extruder, the inherited BBL + // paths read it through the clamping get_at(), which collapses every logical extruder to + // physical 0 while the profile default is the single-element {0}. Deriving the identity + // unconditionally would impose the IMEX reading on printers that mean the other one and + // change what they emit -- {first_tools}/{first_filaments}/{first_non_support_*} and + // {most_used_physical_extruder_id}/{curr_physical_extruder_id} in custom start G-code, plus + // the `; physical_extruder_map =` line in the G-code config block -- for every multi-nozzle + // profile that never authored a map. Non-IMEX printers therefore keep the config value + // exactly as it arrives, which is what upstream slices with. + const auto* is_imex_opt = new_full_config.option("is_imex"); + if (is_imex_opt && is_imex_opt->value) { auto* pem = new_full_config.option("physical_extruder_map", true); if (pem) { pem->values = effective_physical_extruder_map(pem, extruder_count).values;