harden binary parser against integer underflow and unsafe type punning

src/MeshGroup.cpp

@@ -3,6 +3,8 @@
 
 #include "MeshGroup.h"
 
+#include <array>
+#include <cstring>
 #include <filesystem>
 #include <fstream>
 #include <limits>
@@ -179,30 +181,53 @@ bool loadMeshSTL_ascii(Mesh* mesh, const char* filename, const Matrix4x3D& matri
 bool loadMeshSTL_binary(Mesh* mesh, const char* filename, const Matrix4x3D& matrix, const std::vector<Point2F>* uv_coordinates = nullptr)
 {
     FILE* f = fopen(filename, "rb");
+    if (f == nullptr)
+    {
+        return false;
+    }
 
     fseek(f, 0L, SEEK_END);
-    long long file_size = ftell(f); // The file size is the position of the cursor after seeking to the end.
+    const long long file_size = ftell(f); // Cursor position at EOF == file size, or -1 on error.
     rewind(f); // Seek back to start.
-    size_t face_count = (file_size - 80 - sizeof(uint32_t)) / 50; // Subtract the size of the header. Every face uses exactly 50 bytes.
 
-    char buffer[80];
+    // A binary STL is an 80-byte header, a uint32 triangle count, then exactly 50 bytes per triangle.
+    // Reject files too small to even hold the header (or for which ftell() returned -1) before the size
+    // arithmetic, so the unsigned face-count computation below cannot underflow into a huge value and
+    // drive a wild reserve()/read loop.
+    constexpr long long header_size = 80 + static_cast<long long>(sizeof(uint32_t));
+    constexpr size_t bytes_per_face = 50; // Normal (3 floats) + 3 vertices (9 floats) + 2-byte attribute.
+    if (file_size < header_size)
+    {
+        fclose(f);
+        return false;
+    }
+    const size_t face_count = static_cast<size_t>(file_size - header_size) / bytes_per_face;
+
+    std::array<char, 80> header;
     // Skip the header
-    if (fread(buffer, 80, 1, f) != 1)
+    if (fread(header.data(), header.size(), 1, f) != 1)
     {
         fclose(f);
         return false;
     }
 
-    uint32_t reported_face_count;
-    // Read the face count. We'll use it as a sort of redundancy code to check for file corruption.
-    if (fread(&reported_face_count, sizeof(uint32_t), 1, f) != 1)
+    uint32_t reported_face_count = 0;
+    // Read the header's triangle count and cross-check it against the size-derived count to surface
+    // corrupt or malformed files. We keep parsing the size-derived count (some exporters append trailing
+    // data), so a mismatch only warns rather than rejecting the file.
+    if (fread(&reported_face_count, sizeof(reported_face_count), 1, f) != 1)
     {
         fclose(f);
         return false;
     }
     if (reported_face_count != face_count)
     {
-        spdlog::warn("Face count reported by file ({}) is not equal to actual face count ({}). File could be corrupt!", reported_face_count, face_count);
+        const uint64_t expected_size = static_cast<uint64_t>(header_size) + static_cast<uint64_t>(reported_face_count) * bytes_per_face;
+        spdlog::warn(
+            "STL triangle count in header ({}) is inconsistent with the file size (header implies {} bytes, file is {} bytes). File could be corrupt!",
+            reported_face_count,
+            expected_size,
+            file_size);
     }
 
     // For each face read:
@@ -214,16 +239,22 @@ bool loadMeshSTL_binary(Mesh* mesh, const char* filename, const Matrix4x3D& matr
     size_t vertex_index = 0;
     for (size_t i = 0; i < face_count; i++)
     {
-        if (fread(buffer, 50, 1, f) != 1)
+        std::array<std::byte, bytes_per_face> face_record;
+        if (fread(face_record.data(), face_record.size(), 1, f) != 1)
         {
             fclose(f);
             return false;
         }
-        float* v = reinterpret_cast<float*>(buffer) + 3;
-
-        Point3LL v0 = matrix.apply(Point3F(v[0], v[1], v[2]).toPoint3d());
-        Point3LL v1 = matrix.apply(Point3F(v[3], v[4], v[5]).toPoint3d());
-        Point3LL v2 = matrix.apply(Point3F(v[6], v[7], v[8]).toPoint3d());
+        // Decode the 12 little-endian floats with memcpy instead of aliasing a float* over the byte
+        // buffer, removing pointer-alignment and strict-aliasing assumptions while preserving the
+        // existing layout: floats 3..11 (bytes 12..47) are the three vertices; floats 0..2 are the
+        // discarded normal.
+        std::array<float, 12> floats;
+        std::memcpy(floats.data(), face_record.data(), sizeof(floats));
+
+        Point3LL v0 = matrix.apply(Point3F(floats[3], floats[4], floats[5]).toPoint3d());
+        Point3LL v1 = matrix.apply(Point3F(floats[6], floats[7], floats[8]).toPoint3d());
+        Point3LL v2 = matrix.apply(Point3F(floats[9], floats[10], floats[11]).toPoint3d());
 
         // Handle UV coordinates if provided
         if (uv_coordinates && vertex_index + 2 < uv_coordinates->size())

tests/CMakeLists.txt

@@ -13,6 +13,7 @@ set(TESTS_SRC_BASE
         GCodeTemplateResolverTest
         InfillTest
         LayerPlanTest
+        MeshGroupTest
         PathOrderOptimizerTest
         PathOrderMonotonicTest
         TimeEstimateCalculatorTest

tests/MeshGroupTest.cpp

@@ -0,0 +1,115 @@
+// Copyright (c) 2024 UltiMaker
+// CuraEngine is released under the terms of the AGPLv3 or higher
+
+#include "MeshGroup.h" // The unit under test: loadMeshIntoMeshGroup -> binary STL loader.
+
+#include <array>
+#include <cstdint>
+#include <cstring>
+#include <filesystem>
+#include <fstream>
+#include <vector>
+
+#include <gtest/gtest.h>
+
+#include "Application.h" // To set up a slice so that settings can be requested.
+#include "Slice.h"
+#include "settings/Settings.h"
+#include "utils/Matrix4x3D.h"
+
+// NOLINTBEGIN(*-magic-numbers)
+namespace cura
+{
+
+class MeshGroupTest : public testing::Test
+{
+public:
+    void SetUp() override
+    {
+        Application::getInstance().startThreadPool();
+        Application::getInstance().current_slice_ = std::make_shared<Slice>(1);
+    }
+
+    static void appendFloat(std::vector<uint8_t>& bytes, const float value)
+    {
+        std::array<uint8_t, sizeof(float)> raw{};
+        std::memcpy(raw.data(), &value, sizeof(float));
+        bytes.insert(bytes.end(), raw.begin(), raw.end());
+    }
+
+    //! Build a minimal binary STL: 80-byte header, \p reported_count as the uint32 triangle count, then
+    //! one 50-byte record per triangle (each triangle is 9 vertex floats; the normal is zeroed).
+    static std::vector<uint8_t> makeBinaryStl(const uint32_t reported_count, const std::vector<std::array<float, 9>>& triangles)
+    {
+        std::vector<uint8_t> bytes(80, 0x00);
+        std::array<uint8_t, sizeof(uint32_t)> count_raw{};
+        std::memcpy(count_raw.data(), &reported_count, sizeof(uint32_t));
+        bytes.insert(bytes.end(), count_raw.begin(), count_raw.end());
+        for (const std::array<float, 9>& triangle : triangles)
+        {
+            for (int normal = 0; normal < 3; ++normal)
+            {
+                appendFloat(bytes, 0.0F);
+            }
+            for (const float coord : triangle)
+            {
+                appendFloat(bytes, coord);
+            }
+            bytes.push_back(0); // 2-byte attribute
+            bytes.push_back(0);
+        }
+        return bytes;
+    }
+
+    static std::string writeTempFile(const std::string& name, const std::vector<uint8_t>& bytes)
+    {
+        const std::filesystem::path path = std::filesystem::temp_directory_path() / name;
+        std::ofstream out(path, std::ios::binary | std::ios::trunc);
+        out.write(reinterpret_cast<const char*>(bytes.data()), static_cast<std::streamsize>(bytes.size()));
+        out.close();
+        return path.string();
+    }
+};
+
+// Regression: a binary STL smaller than the 84-byte header must be rejected instead of underflowing the
+// triangle-count arithmetic into a gigantic reserve() (which previously aborted the process).
+TEST_F(MeshGroupTest, BinaryStlSmallerThanHeaderIsRejected)
+{
+    Settings& settings = Application::getInstance().current_slice_->scene.settings;
+    MeshGroup mesh_group;
+    // 50 bytes, not starting with "solid", so it is routed to the binary loader.
+    const std::string path = writeTempFile("curaengine_short.stl", std::vector<uint8_t>(50, 0x01));
+    EXPECT_FALSE(loadMeshIntoMeshGroup(&mesh_group, path.c_str(), Matrix4x3D(), settings));
+    std::filesystem::remove(path);
+}
+
+// Regression: a header triangle count that disagrees with the file size must not change what is parsed.
+// The loader trusts the size-derived count and reads no out-of-bounds data.
+TEST_F(MeshGroupTest, BinaryStlWithInconsistentHeaderCountStillLoads)
+{
+    Settings& settings = Application::getInstance().current_slice_->scene.settings;
+    MeshGroup mesh_group;
+    const std::vector<std::array<float, 9>> triangles = {
+        { 0, 0, 0, 10, 0, 0, 0, 10, 0 }, // well-separated, non-degenerate
+        { 0, 0, 20, 10, 0, 20, 0, 10, 20 },
+    };
+    const std::string path = writeTempFile("curaengine_badcount.stl", makeBinaryStl(9999, triangles)); // header lies: 9999
+    ASSERT_TRUE(loadMeshIntoMeshGroup(&mesh_group, path.c_str(), Matrix4x3D(), settings));
+    ASSERT_EQ(mesh_group.meshes.size(), size_t(1));
+    EXPECT_EQ(mesh_group.meshes.back().faces_.size(), size_t(2)); // size-derived count, not the bogus header count
+    std::filesystem::remove(path);
+}
+
+// Behaviour preservation: the memcpy-based decoder still loads the real binary STL fixture.
+TEST_F(MeshGroupTest, BinaryStlFixtureLoads)
+{
+    Settings& settings = Application::getInstance().current_slice_->scene.settings;
+    MeshGroup mesh_group;
+    const std::string path = std::filesystem::path(__FILE__).parent_path().append("testModel.stl").string();
+    ASSERT_TRUE(loadMeshIntoMeshGroup(&mesh_group, path.c_str(), Matrix4x3D(), settings));
+    ASSERT_EQ(mesh_group.meshes.size(), size_t(1));
+    EXPECT_GT(mesh_group.meshes.back().faces_.size(), size_t(0));
+}
+
+} // namespace cura
+// NOLINTEND(*-magic-numbers)