From 468fa3be86f56fddbecaa9d39bf1574c8e90fd28 Mon Sep 17 00:00:00 2001 From: Hanif Koh Date: Mon, 28 Sep 2026 05:59:22 +0800 Subject: [PATCH] Reject DRC Meshes Without Positions or with Invalid Face Indices load_drc dereferenced the POSITION attribute without checking that the mesh has one, and trusted the decoded face indices, which the Draco decoder does not check against the point count. Both now fail the load cleanly. A failed vertex conversion is treated the same way. The libslic3r tests link Draco so they can encode the malformed meshes in-test. --- src/libslic3r/Format/DRC.cpp | 29 ++++++++++---- tests/libslic3r/CMakeLists.txt | 5 ++- tests/libslic3r/test_drc.cpp | 71 ++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 9 deletions(-) create mode 100644 tests/libslic3r/test_drc.cpp diff --git a/src/libslic3r/Format/DRC.cpp b/src/libslic3r/Format/DRC.cpp index d489f04dba..c07655f69b 100644 --- a/src/libslic3r/Format/DRC.cpp +++ b/src/libslic3r/Format/DRC.cpp @@ -48,24 +48,37 @@ bool load_drc(const char *path, TriangleMesh *meshptr) indexed_triangle_set its; const PointAttribute *const positions = dracoMesh.GetNamedAttribute(GeometryAttribute::POSITION); + if (positions == nullptr) { + BOOST_LOG_TRIVIAL(error) << "load_drc: the mesh has no POSITION attribute"; + return false; + } size_t num_vertices = positions->size(); its.vertices.reserve(num_vertices); for (AttributeValueIndex i(0); i < num_vertices; ++ i) { float pos[3]; - positions->ConvertValue(i, 3, pos); + if (!positions->ConvertValue(i, 3, pos)) { + BOOST_LOG_TRIVIAL(error) << "load_drc: invalid vertex position"; + return false; + } its.vertices.emplace_back(pos[0], pos[1], pos[2]); } + // The Draco decoder does not check face indices against the point count. + const uint32_t num_points = dracoMesh.num_points(); size_t num_faces = dracoMesh.num_faces(); its.indices.reserve(num_faces); for (FaceIndex i(0); i < num_faces; ++ i) { - Mesh::Face face = dracoMesh.face(i); - - its.indices.emplace_back( - positions->mapped_index(face[0]).value(), - positions->mapped_index(face[1]).value(), - positions->mapped_index(face[2]).value() - ); + const Mesh::Face &face = dracoMesh.face(i); + stl_triangle_vertex_indices facet; + for (int k = 0; k < 3; ++ k) { + const size_t vertex_idx = face[k].value() < num_points ? positions->mapped_index(face[k]).value() : num_vertices; + if (vertex_idx >= num_vertices) { + BOOST_LOG_TRIVIAL(error) << "load_drc: invalid vertex index"; + return false; + } + facet[k] = static_cast(vertex_idx); + } + its.indices.emplace_back(facet); } *meshptr = TriangleMesh(std::move(its)); diff --git a/tests/libslic3r/CMakeLists.txt b/tests/libslic3r/CMakeLists.txt index 9c6dae471a..120766c516 100644 --- a/tests/libslic3r/CMakeLists.txt +++ b/tests/libslic3r/CMakeLists.txt @@ -14,6 +14,7 @@ add_executable(${_TEST_NAME}_tests test_clipper_utils.cpp test_config.cpp test_config_variant_expansion.cpp + test_drc.cpp test_toolordering_nozzle_group.cpp test_preset_bundle_loading.cpp test_preset_setting_id.cpp @@ -68,7 +69,9 @@ if (TARGET OpenVDB::openvdb) target_sources(${_TEST_NAME}_tests PRIVATE test_hollowing.cpp) endif() -target_link_libraries(${_TEST_NAME}_tests test_common libslic3r Catch2::Catch2WithMain) +# libslic3r links Draco privately; test_drc.cpp encodes its own fixtures. +find_package(Draco REQUIRED) +target_link_libraries(${_TEST_NAME}_tests test_common libslic3r draco::draco Catch2::Catch2WithMain) target_include_directories(${_TEST_NAME}_tests PRIVATE ${CMAKE_SOURCE_DIR}/src) set_property(TARGET ${_TEST_NAME}_tests PROPERTY FOLDER "tests") diff --git a/tests/libslic3r/test_drc.cpp b/tests/libslic3r/test_drc.cpp new file mode 100644 index 0000000000..917fd5c714 --- /dev/null +++ b/tests/libslic3r/test_drc.cpp @@ -0,0 +1,71 @@ +#include + +#include "libslic3r/Format/DRC.hpp" +#include "libslic3r/TriangleMesh.hpp" + +#include + +#include +#include + +#include "test_utils.hpp" + +using namespace Slic3r; + +namespace { + +// Encodes one triangle whose vertices carry a single attribute of the given type. +// The decoder accepts both a mesh without POSITION and a face index past the point count. +void write_drc_triangle(const std::string &path, draco::GeometryAttribute::Type attribute_type, uint32_t second_point) +{ + draco::Mesh mesh; + mesh.set_num_points(3); + draco::GeometryAttribute attribute; + attribute.Init(attribute_type, nullptr, 3, draco::DT_FLOAT32, false, sizeof(float) * 3, 0); + const int attribute_id = mesh.AddAttribute(attribute, true, 3); + const float points[3][3] = {{0, 0, 0}, {1, 0, 0}, {0, 1, 0}}; + for (int i = 0; i < 3; ++i) + mesh.attribute(attribute_id)->SetAttributeValue(draco::AttributeValueIndex(i), points[i]); + draco::Mesh::Face face; + face[0] = draco::PointIndex(0); + face[1] = draco::PointIndex(second_point); + face[2] = draco::PointIndex(2); + mesh.AddFace(face); + + draco::Encoder encoder; + encoder.SetEncodingMethod(draco::MESH_SEQUENTIAL_ENCODING); + draco::EncoderBuffer buffer; + REQUIRE(encoder.EncodeMeshToBuffer(mesh, &buffer).ok()); + boost::nowide::ofstream out(path, std::ios::binary); + out.write(buffer.data(), static_cast(buffer.size())); +} + +} // namespace + +TEST_CASE("A Draco triangle with positions loads", "[DRC]") +{ + ScopedTemporaryFile drc(".drc"); + write_drc_triangle(drc.string(), draco::GeometryAttribute::POSITION, 1); + + TriangleMesh mesh; + REQUIRE(load_drc(drc.string().c_str(), &mesh)); + CHECK(mesh.facets_count() == 1); +} + +TEST_CASE("A Draco mesh without a POSITION attribute fails to load", "[DRC][Regression]") +{ + ScopedTemporaryFile drc(".drc"); + write_drc_triangle(drc.string(), draco::GeometryAttribute::GENERIC, 1); + + TriangleMesh mesh; + CHECK_FALSE(load_drc(drc.string().c_str(), &mesh)); +} + +TEST_CASE("A Draco face referencing a missing point fails to load", "[DRC][Regression]") +{ + ScopedTemporaryFile drc(".drc"); + write_drc_triangle(drc.string(), draco::GeometryAttribute::POSITION, 200); + + TriangleMesh mesh; + CHECK_FALSE(load_drc(drc.string().c_str(), &mesh)); +}