diff --git a/src/libslic3r/Format/bbs_3mf.cpp b/src/libslic3r/Format/bbs_3mf.cpp index d34a7b84b3..e3d673dd20 100644 --- a/src/libslic3r/Format/bbs_3mf.cpp +++ b/src/libslic3r/Format/bbs_3mf.cpp @@ -1633,7 +1633,7 @@ void PlateData::parse_filament_info(GCodeProcessorResult *result) } while (it != m_plater_data.end()) { - if (it->first > m_plater_data.size()) + if (it->first <= 0 || static_cast(it->first) > m_plater_data.size()) { add_error("invalid plate index"); return false; @@ -2316,7 +2316,7 @@ void PlateData::parse_filament_info(GCodeProcessorResult *result) } while (it != m_plater_data.end()) { - if (it->first > m_plater_data.size()) + if (it->first <= 0 || static_cast(it->first) > m_plater_data.size()) { add_error("invalid plate index"); return false; diff --git a/src/libslic3r/Model.cpp b/src/libslic3r/Model.cpp index 5f7bfaaaec..1c6086dd80 100644 --- a/src/libslic3r/Model.cpp +++ b/src/libslic3r/Model.cpp @@ -3750,7 +3750,11 @@ void FacetsAnnotation::set_triangle_from_string(int triangle_id, const std::stri m_data.bitstream.insert(m_data.bitstream.end(), bool(dec & (1 << i))); } - m_data.update_used_states(bitstream_start_idx); + if (!m_data.update_used_states(bitstream_start_idx)) { + BOOST_LOG_TRIVIAL(warning) << __FUNCTION__ << ": dropping malformed paint data of triangle " << triangle_id; + m_data.bitstream.resize(bitstream_start_idx); + m_data.triangles_to_split.pop_back(); + } } bool FacetsAnnotation::equals(const FacetsAnnotation &other) const diff --git a/src/libslic3r/TriangleSelector.cpp b/src/libslic3r/TriangleSelector.cpp index b47004fca5..3dfea4c0c7 100644 --- a/src/libslic3r/TriangleSelector.cpp +++ b/src/libslic3r/TriangleSelector.cpp @@ -1778,6 +1778,13 @@ TriangleSelector::TriangleSplittingData TriangleSelector::serialize() const { return out.data; } +// A split code keeps the split side (one split) or the kept side (two splits) in its upper two +// bits, where 3 is not a side. The value is ignored for a three-side split. +static bool split_code_valid(int code) +{ + return (code & 0b11) == 3 || (code >> 2) != 3; +} + void TriangleSelector::deserialize(const TriangleSplittingData &data, bool needs_reset, EnforcerBlockerType max_ebt, @@ -1812,11 +1819,12 @@ void TriangleSelector::deserialize(const TriangleSplittingData &data, for (auto [triangle_id, ibit] : data.triangles_to_split) { assert(triangle_id < int(m_triangles.size())); - assert(ibit < int(data.bitstream.size())); - auto next_nibble = [&data, &ibit = ibit]() { + // Set when the bitstream runs out or holds an impossible split before this triangle's tree is complete. + bool corrupt = false; + auto next_nibble = [&data, &ibit = ibit, &corrupt]() { int n = 0; - for (int i = 0; i < 4; ++ i) - n |= data.bitstream[ibit ++] << i; + if (! data.read_nibble(ibit, n)) + corrupt = true; return n; }; // Decode a leaf state stored behind the "11" prefix: one nibble of (state-3) for states @@ -1835,6 +1843,10 @@ void TriangleSelector::deserialize(const TriangleSplittingData &data, bool is_split = num_of_children != 0; // Only valid if not is_split. auto state = is_split ? EnforcerBlockerType::NONE : ((code & 0b1100) == 0b1100 ? decode_leaf_state() : EnforcerBlockerType(code >> 2)); + if (is_split && ! split_code_valid(code)) + corrupt = true; + if (corrupt) + break; // BBS if (state == to_delete_filament) @@ -1849,7 +1861,7 @@ void TriangleSelector::deserialize(const TriangleSplittingData &data, } // Only valid if is_split. - int special_side = code >> 2; + int special_side = num_of_split_sides == 3 ? 0 : code >> 2; // Take care of the first iteration separately, so handling of the others is simpler. if (parents.empty()) { @@ -1904,47 +1916,55 @@ void TriangleSelector::deserialize(const TriangleSplittingData &data, if (parents.empty()) break; } + + if (corrupt) { + // Every split above allocated all of its children, so the partial tree unwinds cleanly. + BOOST_LOG_TRIVIAL(warning) << __FUNCTION__ << ": malformed paint data, dropping paint of triangle " << triangle_id; + undivide_triangle(triangle_id); + m_triangles[triangle_id].set_state(EnforcerBlockerType::NONE); + } } } -void TriangleSelector::TriangleSplittingData::update_used_states(const size_t bitstream_start_idx) { - assert(bitstream_start_idx < this->bitstream.size()); - assert(!this->bitstream.empty() && this->bitstream.size() != bitstream_start_idx); - assert((this->bitstream.size() - bitstream_start_idx) % 4 == 0); +bool TriangleSelector::TriangleSplittingData::update_used_states(const size_t bitstream_start_idx) { + int ibit = static_cast(bitstream_start_idx); + uint64_t states = 0; + do { + // Walk one triangle's tree depth-first, counting the nodes still to be read; a split node adds its children. + for (int pending_nodes = 1; pending_nodes > 0; --pending_nodes) { + int code; + if (!this->read_nibble(ibit, code)) + return false; - if (this->bitstream.empty() || this->bitstream.size() == bitstream_start_idx) - return; + if (const int num_of_split_sides = code & 0b11; num_of_split_sides != 0) { + if (!split_code_valid(code)) + return false; + pending_nodes += num_of_split_sides + 1; + continue; + } - size_t nibble_idx = bitstream_start_idx; - - auto read_next_nibble = [&data_bitstream = std::as_const(this->bitstream), &nibble_idx]() -> uint8_t { - assert(nibble_idx + 3 < data_bitstream.size()); - uint8_t code = 0; - for (size_t bit_idx = 0; bit_idx < 4; ++bit_idx) - code |= data_bitstream[nibble_idx++] << bit_idx; - return code; - }; - - while (nibble_idx < this->bitstream.size()) { - const uint8_t code = read_next_nibble(); - - if (const bool is_split = (code & 0b11) != 0; is_split) - continue; - - uint8_t facet_state; - if ((code & 0b1100) == 0b1100) { - // Leaf behind the "11" prefix: one nibble of (state-3), or 0b1111 + (state-18). - const uint8_t nibble = read_next_nibble(); - facet_state = nibble == 0b1111 ? uint8_t(read_next_nibble() + 18) : uint8_t(nibble + 3); - } else { - facet_state = code >> 2; + int facet_state = code >> 2; + if (facet_state == 0b11) { + // Leaf behind the "11" prefix: one nibble of (state-3), or 0b1111 + (state-18). + int nibble; + if (!this->read_nibble(ibit, nibble)) + return false; + facet_state = nibble + 3; + if (nibble == 0b1111) { + if (!this->read_nibble(ibit, nibble)) + return false; + facet_state = nibble + 18; + } + } + states |= uint64_t(1) << facet_state; } - assert(facet_state < this->used_states.size()); - if (facet_state >= this->used_states.size()) - continue; + } while (static_cast(ibit) < this->bitstream.size()); - this->used_states[facet_state] = true; - } + // The leaf encoding tops out at state 33, so every state fits the 64-bit mask. + for (size_t state_idx = 0; state_idx < std::min(this->used_states.size(), 64); ++state_idx) + if (states & (uint64_t(1) << state_idx)) + this->used_states[state_idx] = true; + return true; } // Lightweight variant of deserialization, which only tests whether a face of test_state exists. @@ -1956,11 +1976,12 @@ bool TriangleSelector::has_facets(const TriangleSplittingData &data, const Enfor for (const TriangleBitStreamMapping &triangle_id_and_ibit : data.triangles_to_split) { int ibit = triangle_id_and_ibit.bitstream_start_idx; - assert(ibit < int(data.bitstream.size())); - auto next_nibble = [&data, &ibit = ibit]() { + // Stop reading a triangle whose stream is truncated. + bool truncated = false; + auto next_nibble = [&data, &ibit = ibit, &truncated]() { int n = 0; - for (int i = 0; i < 4; ++ i) - n |= data.bitstream[ibit ++] << i; + if (! data.read_nibble(ibit, n)) + truncated = true; return n; }; // < 0 -> negative of a number of children @@ -1978,6 +1999,8 @@ bool TriangleSelector::has_facets(const TriangleSplittingData &data, const Enfor }; int state = num_children_or_state(); + if (truncated) + continue; if (state < 0) { // Root is split. parents_children.clear(); @@ -1985,6 +2008,8 @@ bool TriangleSelector::has_facets(const TriangleSplittingData &data, const Enfor do { if (-- parents_children.back() >= 0) { int state = num_children_or_state(); + if (truncated) + break; if (state < 0) // Child is split. parents_children.emplace_back(- state); diff --git a/src/libslic3r/TriangleSelector.hpp b/src/libslic3r/TriangleSelector.hpp index 594f710e45..2e6627039c 100644 --- a/src/libslic3r/TriangleSelector.hpp +++ b/src/libslic3r/TriangleSelector.hpp @@ -297,8 +297,20 @@ public: std::fill(used_states.begin(), used_states.end(), false); } - // Update used states based on the bitstream. It just iterated over the bitstream from the bitstream_start_idx till the end. - void update_used_states(size_t bitstream_start_idx); + // Update used states from the triangle trees stored between bitstream_start_idx and the end of the bitstream. + // Returns false and leaves used states untouched if a tree is truncated or malformed. + bool update_used_states(size_t bitstream_start_idx); + + // Read the 4-bit code at bit index ibit (LSB first) and advance ibit past it. + // Returns false without advancing when fewer than 4 bits remain. + bool read_nibble(int &ibit, int &nibble) const { + if (ibit < 0 || static_cast(ibit) + 4 > bitstream.size()) + return false; + nibble = 0; + for (int i = 0; i < 4; ++i) + nibble |= static_cast(bitstream[ibit++]) << i; + return true; + } private: friend class cereal::access; diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index 02d392140b..ce3285c7ad 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -184,16 +185,13 @@ static bool read_cad_recipe_entry(const std::string& path, std::string& out, return found; } -// Rewrites the archive at `path` with the recipe entry back under the name it had before the -// rename, which is what every project saved by an earlier build looks like on disk. Generated -// rather than checked in because a whole project archive is not frozen evidence the way a bare -// recipe blob is -- it has to be whatever today's exporter writes, with only the name aged. -// miniz cannot rename in place and open_zip_writer truncates, so the entries are held across -// the switch. -static void rename_cad_recipe_entry_to_legacy(const std::string& path) +// Rewrites the archive at `path`, letting `edit` change the name or data of each entry; returns +// whether `edit` reported a change for any of them. miniz cannot edit in place and +// open_zip_writer truncates, so the entries are held across the switch. +static bool rewrite_3mf_entries(const std::string& path, const std::function& edit) { std::vector> entries; - bool renamed = false; + bool changed = false; { mz_zip_archive zip; mz_zip_zero_struct(&zip); @@ -208,22 +206,140 @@ static void rename_cad_recipe_entry_to_legacy(const std::string& path) std::string data((size_t) st.m_uncomp_size, '\0'); if (st.m_uncomp_size > 0) REQUIRE(mz_zip_reader_extract_to_mem(&zip, i, data.data(), data.size(), 0)); - if (boost::algorithm::iequals(name, CAD_RECIPE_ENTRY)) { - name = LEGACY_CAD_RECIPE_ENTRY; - renamed = true; - } + changed |= edit(name, data); entries.emplace_back(std::move(name), std::move(data)); } close_zip_reader(&zip); } - // Without this the scenario would degrade silently into re-testing the new name if the - // exporter's constant ever moved again: every load below would still pass. - REQUIRE(renamed); Zipper out(path); for (const auto& e : entries) out.add_entry(e.first, e.second.data(), e.second.size()); out.finalize(); + return changed; +} + +// Rewrites the archive at `path` with the recipe entry back under the name it had before the +// rename, which is what every project saved by an earlier build looks like on disk. Generated +// rather than checked in because a whole project archive is not frozen evidence the way a bare +// recipe blob is -- it has to be whatever today's exporter writes, with only the name aged. +static void rename_cad_recipe_entry_to_legacy(const std::string& path) +{ + const bool renamed = rewrite_3mf_entries(path, [](std::string& name, std::string&) { + if (!boost::algorithm::iequals(name, CAD_RECIPE_ENTRY)) + return false; + name = LEGACY_CAD_RECIPE_ENTRY; + return true; + }); + // Without this the scenario would degrade silently into re-testing the new name if the + // exporter's constant ever moved again: every load below would still pass. + REQUIRE(renamed); +} + +// Replaces the first occurrence of `from` in any entry whose name ends with `suffix`. +static bool replace_in_3mf_entry(const std::string& path, const std::string& suffix, const std::string& from, const std::string& to) +{ + bool replaced = false; + rewrite_3mf_entries(path, [&](std::string& name, std::string& data) { + if (replaced || !boost::algorithm::ends_with(name, suffix)) + return false; + if (const size_t pos = data.find(from); pos != std::string::npos) { + data.replace(pos, from.size(), to); + replaced = true; + } + return replaced; + }); + return replaced; +} + +// Stores a one-plate project holding a cube whose first two facets are painted Extruder2 and +// Extruder3, which the exporter writes as paint_color="8" and paint_color="0C". +static void store_painted_cube(const std::string& path) +{ + Model model; + ModelObject* object = model.add_object(); + ModelVolume* volume = object->add_volume(make_cube(10., 10., 10.)); + object->add_instance(); + { + TriangleSelector selector(volume->mesh()); + selector.set_facet(0, EnforcerBlockerType::Extruder2); + selector.set_facet(1, EnforcerBlockerType::Extruder3); + REQUIRE(volume->mmu_segmentation_facets.set(selector)); + } + ScopedTemporaryDir backup_dir("orca_paint_src"); + model.set_backup_path(backup_dir.string()); + + DynamicPrintConfig cfg; + PlateData plate; + plate.plate_index = 0; + StoreParams sp; + sp.path = path.c_str(); + sp.model = &model; + sp.config = &cfg; + sp.strategy = SaveStrategy::Zip64 | SaveStrategy::Silence; + sp.plate_data_list.push_back(&plate); + REQUIRE(store_bbs_3mf(sp)); +} + +// Loads `path` through the BBS importer into `model`, releasing the plates it returns. The +// importer stages metadata through `backup_dir`, which has to outlive the model. +static bool load_project(const std::string& path, Model& model, const ScopedTemporaryDir& backup_dir) +{ + model.set_backup_path(backup_dir.string()); + DynamicPrintConfig config; + ConfigSubstitutionContext ctxt{ ForwardCompatibilitySubstitutionRule::Enable }; + PlateDataPtrs plates; + std::vector project_presets; + bool is_bbl_3mf = false, is_orca_3mf = false; + Semver file_version; + const bool loaded = load_bbs_3mf(path.c_str(), &config, &ctxt, &model, &plates, &project_presets, &is_bbl_3mf, + &is_orca_3mf, &file_version, nullptr, LoadStrategy::LoadModel | LoadStrategy::LoadConfig); + release_PlateData_list(plates); + return loaded; +} + +TEST_CASE("A project with a plate id below 1 fails to load", "[3mf][Regression]") +{ + const int plate_id = GENERATE(0, -1); + INFO("plater_id " << plate_id); + + ScopedTemporaryFile temp(".3mf"); + store_painted_cube(temp.string()); + { + ScopedTemporaryDir backup_dir("orca_plate_dst"); + Model model; + REQUIRE(load_project(temp.string(), model, backup_dir)); + } + + REQUIRE(replace_in_3mf_entry(temp.string(), "model_settings.config", "key=\"plater_id\" value=\"1\"", + "key=\"plater_id\" value=\"" + std::to_string(plate_id) + "\"")); + ScopedTemporaryDir backup_dir("orca_plate_dst"); + Model model; + bool loaded = true; + REQUIRE_NOTHROW(loaded = load_project(temp.string(), model, backup_dir)); + REQUIRE_FALSE(loaded); +} + +TEST_CASE("A project with malformed paint data loads without the damaged facet", "[3mf][Regression]") +{ + ScopedTemporaryFile temp(".3mf"); + store_painted_cube(temp.string()); + // Split codes with no children behind them: the stream runs out mid-tree. + REQUIRE(replace_in_3mf_entry(temp.string(), ".model", "paint_color=\"8\"", "paint_color=\"FFFFFFFFFFFFFFFF3\"")); + + ScopedTemporaryDir backup_dir("orca_paint_dst"); + Model model; + REQUIRE(load_project(temp.string(), model, backup_dir)); + REQUIRE(model.objects.size() == 1); + const ModelVolume& volume = *model.objects.front()->volumes.front(); + const auto& data = volume.mmu_segmentation_facets.get_data(); + REQUIRE_FALSE(data.used_states[size_t(EnforcerBlockerType::Extruder2)]); + REQUIRE(data.used_states[size_t(EnforcerBlockerType::Extruder3)]); + + TriangleSelector selector(volume.mesh()); + REQUIRE_NOTHROW(selector.deserialize(data)); + REQUIRE(selector.num_facets(EnforcerBlockerType::Extruder2) == 0); + REQUIRE(selector.num_facets(EnforcerBlockerType::Extruder3) == 1); } // The recipe lives only in the BBS-native backend, because that is the only one that runs: diff --git a/tests/libslic3r/test_triangle_selector.cpp b/tests/libslic3r/test_triangle_selector.cpp index fd2ab9efa8..c207ee66ad 100644 --- a/tests/libslic3r/test_triangle_selector.cpp +++ b/tests/libslic3r/test_triangle_selector.cpp @@ -3,6 +3,8 @@ #include "libslic3r/TriangleSelector.hpp" #include "libslic3r/TriangleMesh.hpp" +#include + using namespace Slic3r; // A sphere gives well over ExtruderMax original facets, so every extruder state can be assigned @@ -123,3 +125,77 @@ TEST_CASE("Extruder states match the CONST_FILAMENTS hex encoding", "[TriangleSe INFO("Hex " << c.hex << " -> extruder " << c.state); REQUIRE(TriangleSelector::has_facets(data, EnforcerBlockerType(c.state))); } + +// Pack 4-bit codes into a bitstream, least significant bit first, in the order the decoder reads them. +static std::vector pack_nibbles(const std::vector &nibbles) +{ + std::vector bitstream; + for (const int nibble : nibbles) + for (int bit = 0; bit < 4; ++bit) + bitstream.push_back((nibble >> bit) & 1); + return bitstream; +} + +TEST_CASE("A valid paint stream with nested splits round-trips bit for bit", "[TriangleSelector]") +{ + const TriangleMesh mesh = test_mesh(); + + TriangleSelector::TriangleSplittingData data; + data.triangles_to_split.emplace_back(0, 0); + // A three-side split whose children, in stream order, are: a one-side split (side 2) into two + // leaves, a two-side split (side 1) into leaves of states 20, 0 and 8, then two plain leaves. + const std::vector triangle_0 = {0b0011, + 0b1001, 0b1000, 0b0100, + 0b0110, 0b1100, 0b1111, 20 - 18, 0b0000, 0b1100, 8 - 3, + 0b1000, + 0b0100}; + data.bitstream = pack_nibbles(triangle_0); + data.triangles_to_split.emplace_back(5, int(data.bitstream.size())); + const std::vector triangle_5 = pack_nibbles({0b1100, 3 - 3}); + data.bitstream.insert(data.bitstream.end(), triangle_5.begin(), triangle_5.end()); + data.reset_used_states(); + REQUIRE(data.update_used_states(0)); + + TriangleSelector restored(mesh); + restored.deserialize(data); + + REQUIRE(restored.num_facets(EnforcerBlockerType::Extruder20) == 1); + REQUIRE(restored.num_facets(EnforcerBlockerType::Extruder3) == 1); + REQUIRE(restored.serialize() == data); +} + +TEST_CASE("A truncated or malformed paint stream drops only the damaged triangle", "[TriangleSelector][Regression]") +{ + struct Case { const char *name; std::vector nibbles; }; + const auto c = GENERATE(values({ + {"three-side split missing two children", {0b0011, 0b1000, 0b1000}}, + {"leaf missing its state nibble", {0b1100}}, + {"leaf missing its second state nibble", {0b1100, 0b1111}}, + {"splits nested past the end", {0b0011, 0xF, 0xF, 0xF, 0xF, 0xF, 0xF, 0xF, 0xF, + 0xF, 0xF, 0xF, 0xF, 0xF, 0xF, 0xF, 0xF}}, + {"one-side split of the nonexistent side 3", {0b1101, 0b1000, 0b1000}}, + })); + INFO(c.name); + + const TriangleMesh mesh = test_mesh(); + TriangleSelector intact(mesh); + intact.set_facet(0, EnforcerBlockerType::Extruder2); + + // Triangle 0 stays intact, triangle 1 carries the damaged stream. + TriangleSelector::TriangleSplittingData data = intact.serialize(); + data.triangles_to_split.emplace_back(1, int(data.bitstream.size())); + const std::vector damaged = pack_nibbles(c.nibbles); + data.bitstream.insert(data.bitstream.end(), damaged.begin(), damaged.end()); + + TriangleSelector restored(mesh); + REQUIRE_NOTHROW(restored.deserialize(data)); + // Triangle 1 unwinds completely, so the selector holds exactly the intact paint. + REQUIRE(restored.serialize() == intact.serialize()); + + REQUIRE_NOTHROW(TriangleSelector::has_facets(data, EnforcerBlockerType::Extruder3)); + + TriangleSelector::TriangleSplittingData recomputed = data; + recomputed.reset_used_states(); + REQUIRE_FALSE(recomputed.update_used_states(0)); + REQUIRE(std::none_of(recomputed.used_states.begin(), recomputed.used_states.end(), [](bool used) { return used; })); +}