From 855c1b51ac4e71efc89477cee218f8f094b56a4a Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Mon, 31 Aug 2026 01:47:45 -0400 Subject: [PATCH] fix: size the destination row before migrating per-variant values update_values_from_multi_to_multi_2 iterates the destination PRINTER's variant list while writing into a row taken from the destination PRINT preset. Those two lengths are maintained independently -- print_extruder_variant against printer_extruder_variant -- and Tab::load_current_preset() runs the migration before the print preset is re-selected for the new printer. Opening a project saved on a single-variant printer and switching to a seven-variant one therefore wrote six elements past the end of a one-element vector. The corruption stays silent until the next allocation, so the abort surfaces somewhere unrelated and the backtrace points at innocent code. Size the row to the variant count before indexing it. Every write is then in range, and the result carries one value per destination variant, which is what the callers consume. Pad with nil rather than a copied value: set_to_index() skips nil entries, so a variant the object has no opinion about keeps tracking the print preset instead of being pinned to another variant's number. The same shape -- a count from one array indexing another -- appears twice more in this file. update_values_from_multi_to_multi has three of these writes protected only by assert(idx < old_count), and NDEBUG is defined for every non-Debug configuration, so those guards are absent from shipping builds. update_values_from_single_to_multi has the read half. Both are bounded here; leaving them would fix one third of one defect. Source reads are bounded too. is_nil(size_t) indexes values[idx] without checking, so an index past the end was undefined behaviour on that side as well. Where the row already matches the variant list -- every case that was not corrupting the heap -- the resize is a no-op and the output is unchanged. Fixes #15455 Co-Authored-By: Claude Opus 5 (1M context) --- src/libslic3r/PrintConfig.cpp | 57 +++++++++++-- .../test_config_variant_expansion.cpp | 81 +++++++++++++++++++ 2 files changed, 131 insertions(+), 7 deletions(-) diff --git a/src/libslic3r/PrintConfig.cpp b/src/libslic3r/PrintConfig.cpp index 3f977647ce..634164a005 100644 --- a/src/libslic3r/PrintConfig.cpp +++ b/src/libslic3r/PrintConfig.cpp @@ -10238,6 +10238,10 @@ int DynamicPrintConfig::update_values_from_single_to_multi(DynamicPrintConfig& m for (int index = 0; index < variant_count; index++) { + //variant_count is the variant column width, src_opt the value array; + //they disagree when the source was authored at a different width + if (index >= (int)src_opt->values.size()) + break; if (opt->values[index] > src_opt->values[index]) opt->values[index] = src_opt->values[index]; } @@ -10255,6 +10259,8 @@ int DynamicPrintConfig::update_values_from_single_to_multi(DynamicPrintConfig& m for (int index = 0; index < variant_count; index++) { + if (index >= (int)src_opt->values.size()) + break; if (opt->values[index].value > src_opt->values[index].value) opt->values[index] = src_opt->values[index]; } @@ -10449,6 +10455,10 @@ int DynamicPrintConfig::update_values_from_multi_to_multi(DynamicPrintConfig& ne for(auto idx : variant_indices){ assert(idx < old_count); + //the counts come from the variant columns, the arrays from the options; + //they disagree when a config was authored at a different variant width + if (idx >= old_count || new_variant_index >= (int)opt->values.size()) + continue; if (old_values[idx] < opt->values[new_variant_index]) opt->values[new_variant_index] = old_values[idx]; } @@ -10479,6 +10489,10 @@ int DynamicPrintConfig::update_values_from_multi_to_multi(DynamicPrintConfig& ne for(auto idx : variant_indices){ assert(idx < old_count); + //the counts come from the variant columns, the arrays from the options; + //they disagree when a config was authored at a different variant width + if (idx >= old_count || new_variant_index >= (int)opt->values.size()) + continue; if (old_values[idx] < opt->values[new_variant_index]) opt->values[new_variant_index] = old_values[idx]; } @@ -10509,6 +10523,8 @@ int DynamicPrintConfig::update_values_from_multi_to_multi(DynamicPrintConfig& ne for(auto idx : variant_indices){ assert(idx < old_count); + if (idx >= old_count || new_variant_index >= (int)opt->values.size()) + continue; if (old_values[idx]) //enabled opt->values[new_variant_index] = old_values[idx]; } @@ -10549,6 +10565,15 @@ int DynamicPrintConfig::update_values_from_multi_to_multi_2(const std::vectorkeys(); for(auto& key : keys){ if(key_sets.find(key) == key_sets.end()) @@ -10564,7 +10589,13 @@ int DynamicPrintConfig::update_values_from_multi_to_multi_2(const std::vectoroption(key); auto src_values = opt->values; - auto dst_values = dst_config.option(key) ->values; + const auto* dst_opt = dst_config.option(key); + if(!dst_opt){ + BOOST_LOG_TRIVIAL(warning) << __FUNCTION__ << boost::format(", Line %1%: %2% missing from destination config")%__LINE__%key; + break; + } + auto dst_values = dst_opt->values; + dst_values.resize(dst_variant_count, ConfigOptionFloatsNullable::nil_value()); for(size_t dst_idx =0; dst_idx < same_variant_indices.size(); ++dst_idx){ auto& indices = same_variant_indices[dst_idx]; if(indices.empty()) @@ -10572,7 +10603,7 @@ int DynamicPrintConfig::update_values_from_multi_to_multi_2(const std::vector::max(); for(auto idx : indices){ - if(opt && idx < opt->values.size() && !opt->is_nil(idx)){ + if(idx < (int)opt->values.size() && !opt->is_nil(idx)){ has_value = true; target_value = std::min(target_value, src_values[idx]); } @@ -10588,7 +10619,13 @@ int DynamicPrintConfig::update_values_from_multi_to_multi_2(const std::vectoroption(key); auto src_values = opt->values; - auto dst_values = dst_config.option(key) ->values; + const auto* dst_opt = dst_config.option(key); + if(!dst_opt){ + BOOST_LOG_TRIVIAL(warning) << __FUNCTION__ << boost::format(", Line %1%: %2% missing from destination config")%__LINE__%key; + break; + } + auto dst_values = dst_opt->values; + dst_values.resize(dst_variant_count, ConfigOptionFloatsOrPercentsNullable::nil_value()); for(size_t dst_idx =0; dst_idx < same_variant_indices.size(); ++dst_idx){ auto& indices = same_variant_indices[dst_idx]; if(indices.empty()) @@ -10596,7 +10633,7 @@ int DynamicPrintConfig::update_values_from_multi_to_multi_2(const std::vectoris_nil(idx)){ + if(idx < (int)opt->values.size() && !opt->is_nil(idx)){ has_value = true; target_value = src_values[idx].value < target_value.value ? src_values[idx] : target_value; } @@ -10612,15 +10649,21 @@ int DynamicPrintConfig::update_values_from_multi_to_multi_2(const std::vectoroption(key); auto src_values = opt->values; - auto dst_values = dst_config.option(key) ->values; + const auto* dst_opt = dst_config.option(key); + if(!dst_opt){ + BOOST_LOG_TRIVIAL(warning) << __FUNCTION__ << boost::format(", Line %1%: %2% missing from destination config")%__LINE__%key; + break; + } + auto dst_values = dst_opt->values; + dst_values.resize(dst_variant_count, ConfigOptionBoolsNullable::nil_value()); for(size_t dst_idx =0; dst_idx < same_variant_indices.size(); ++dst_idx){ auto indices = same_variant_indices[dst_idx]; if(indices.empty()) continue; bool has_value = false; - bool target_value; + bool target_value = false; for(auto idx : indices){ - if(opt && !opt->is_nil(idx)){ + if(idx < (int)opt->values.size() && !opt->is_nil(idx)){ has_value = true; target_value = src_values[idx]; break; diff --git a/tests/libslic3r/test_config_variant_expansion.cpp b/tests/libslic3r/test_config_variant_expansion.cpp index dd7de2af44..28901702cc 100644 --- a/tests/libslic3r/test_config_variant_expansion.cpp +++ b/tests/libslic3r/test_config_variant_expansion.cpp @@ -479,3 +479,84 @@ TEST_CASE("update_values_to_printer_extruders_for_multiple_filaments resolves pe REQUIRE(config.option("filament_self_index")->values == std::vector({1, 2})); } } + +// update_values_from_multi_to_multi_2 walks the DESTINATION PRINTER's variant list while writing +// into a row taken from the destination PRINT preset, whose arrays are sized to its own +// print_extruder_variant. Those two widths disagree until the print preset is re-selected for the +// new printer -- Tab::load_current_preset() runs this migration first -- so a project authored on +// a single-variant printer, opened and switched to a wider one, wrote past the end of the row. +TEST_CASE("update_values_from_multi_to_multi_2 sizes the destination row to the variant count", + "[Config][VariantExpansion]") +{ + const std::vector src_variants{"Direct Drive Standard"}; + const std::vector dst_variants{"Direct Drive Standard", "Direct Drive High Flow", + "Direct Drive Standard", "Direct Drive High Flow"}; + const std::set keys{"outer_wall_speed"}; + + // The per-object override as authored on the single-variant printer. + const auto object_override = [] { + DynamicPrintConfig c; + c.option("outer_wall_speed", true)->values = {42.}; + return c; + }; + + SECTION("a row narrower than the variant list is grown, not overrun") { + DynamicPrintConfig object_config = object_override(); + DynamicPrintConfig dst; + dst.option("outer_wall_speed", true)->values = {200.}; + + REQUIRE(object_config.update_values_from_multi_to_multi_2(src_variants, dst_variants, dst, keys) == 0); + + const auto& out = object_config.option("outer_wall_speed")->values; + REQUIRE(out.size() == dst_variants.size()); + // Both "Direct Drive Standard" columns match the source variant, so they take the override. + CHECK(out[0] == Catch::Approx(42.)); + CHECK(out[2] == Catch::Approx(42.)); + // The High Flow columns have no matching source variant: nil, so the destination keeps + // tracking the print preset rather than being pinned to another variant's value. + CHECK(std::isnan(out[1])); + CHECK(std::isnan(out[3])); + } + + // The regression guard: where the row already matches the variant list -- every case that was + // not corrupting the heap -- the resize is a no-op and the output is unchanged. + SECTION("a correctly sized row is untouched") { + DynamicPrintConfig object_config = object_override(); + DynamicPrintConfig dst; + dst.option("outer_wall_speed", true)->values = {200., 500., 210., 510.}; + + REQUIRE(object_config.update_values_from_multi_to_multi_2(src_variants, dst_variants, dst, keys) == 0); + + const auto& out = object_config.option("outer_wall_speed")->values; + REQUIRE(out.size() == 4); + CHECK(out[0] == Catch::Approx(42.)); // matched -> override + CHECK(out[1] == Catch::Approx(500.)); // unmatched -> preset value preserved + CHECK(out[2] == Catch::Approx(42.)); + CHECK(out[3] == Catch::Approx(510.)); + } + + // is_nil(idx) indexes values[idx] with no bounds check, so a source shorter than its own + // variant list read out of range before the guard was added. + SECTION("a source shorter than its variant list is read in range") { + DynamicPrintConfig object_config = object_override(); // one value... + DynamicPrintConfig dst; + dst.option("outer_wall_speed", true)->values = {200., 500.}; + + REQUIRE(object_config.update_values_from_multi_to_multi_2( + {"Direct Drive Standard", "Direct Drive Standard"}, // ...but two source variants + {"Direct Drive Standard", "Direct Drive High Flow"}, dst, keys) == 0); + + const auto& out = object_config.option("outer_wall_speed")->values; + REQUIRE(out.size() == 2); + CHECK(out[0] == Catch::Approx(42.)); + CHECK(out[1] == Catch::Approx(500.)); + } + + SECTION("an empty destination variant list is refused") { + DynamicPrintConfig object_config = object_override(); + DynamicPrintConfig dst; + dst.option("outer_wall_speed", true)->values = {200.}; + + CHECK(object_config.update_values_from_multi_to_multi_2(src_variants, {}, dst, keys) == -1); + } +}