From 2976213bc04248596b75a9160b1963e6719ace58 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:54:57 -0400 Subject: [PATCH 1/2] test(MeshIO): failing tests for three PLY reader/writer robustness bugs Red phase. Each of these makes a malformed or unusual file produce a wrong result instead of an error: - UnknownElementSignedListCount_Throws: PLY allows a signed list-count type. A 0xFF count byte read as char is -1, which as an unsigned byte count wraps to a small negative seek that skips nothing, so the element's payload is parsed as vertex data. Returns two junk vertices, no error. - VertexElementWithListProperty_Throws (+ _ASCII_): the binary vertex reader sizes each record by summing scalar widths and the ASCII reader indexes tokens by position, so neither handles a list property on the vertex element. The first vertex reads correctly and the rest are garbage. - WriteFailureInFinalFlush_Throws: write_ply checks the stream while the tail is still buffered. Isolated with RLIMIT_FSIZE at 0 and a mesh smaller than the stream buffer, so nothing is written until close and only the flush fails - the stream is still good when the existing check runs. POSIX-only; there is no portable way to provoke it. Two guard tests pass already, and exist so the fixes cannot be over-strict: a well-formed unknown element still skips correctly, and a genuinely huge count still fails. Found by code review of #28. All three predate that branch. Co-Authored-By: Claude Opus 5 --- tests/src/TestMeshIO.cpp | 233 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index 1994eaa..72f4a1a 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -1,10 +1,16 @@ #include #include +#include +#include #include #include #include +#if defined(__unix__) || defined(__APPLE__) +#include +#endif + #include "educelab/core/io/MeshIO.hpp" #include "educelab/core/io/MeshIO_OBJ.hpp" #include "educelab/core/io/MeshIO_PLY.hpp" @@ -1998,6 +2004,233 @@ TEST_F(PLYTest, MalformedPropertyLine_Throws) // PLY: face record with texcoord list count beyond the per-face safety cap // should throw rather than attempting to allocate an unbounded buffer. +//------------------------------------------------------------------------------ +// Reader robustness: a malformed or unusual file must produce an error, never +// a silently wrong mesh. These are the worst failure mode for a reader — the +// caller gets geometry back and has no way to know it is garbage. +//------------------------------------------------------------------------------ + +TEST_F(PLYTest, UnknownElementSignedListCount_Throws) +{ + // PLY permits a signed list-count type. A count byte of 0xFF read as + // `char` is -1, which as an unsigned byte-count is astronomically large; + // multiplied by the element width it wraps back around to a small negative + // number, and a seek of that size skips nothing at all. The reader then + // parses this element's payload as vertex data. + // + // There are enough bytes left for that to succeed, so without a bound the + // read returns two vertices of junk and reports no error. + const auto path = ply("unknown_signed_count"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_little_endian 1.0\n" + << "element blob 1\n" + << "property list char double junk\n" + << "element vertex 2\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "element face 0\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + const uint8_t count = 0xFF; // -1 as char + f.write(reinterpret_cast(&count), 1); + const double junk[3] = {-9.5, -8.5, -7.5}; + f.write(reinterpret_cast(junk), sizeof(junk)); + const float verts[6] = {1.f, 2.f, 3.f, 4.f, 5.f, 6.f}; + f.write(reinterpret_cast(verts), sizeof(verts)); + } + + Mesh3f dst; + EXPECT_THROW(read_ply(path, dst), std::runtime_error); +} + +TEST_F(PLYTest, UnknownElementOversizeListCount_Throws) +{ + // The same guard from the other side: a count that is genuinely huge + // rather than negative. This already failed before the bound existed, but + // only by running off the end of the file, so it is pinned here to make + // sure it now fails on the count itself. + const auto path = ply("unknown_huge_count"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_little_endian 1.0\n" + << "element blob 1\n" + << "property list uint double junk\n" + << "element vertex 1\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "element face 0\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + const uint32_t count = 0xFFFFFFFFu; + f.write(reinterpret_cast(&count), 4); + const float verts[3] = {1.f, 2.f, 3.f}; + f.write(reinterpret_cast(verts), sizeof(verts)); + } + + Mesh3f dst; + EXPECT_THROW(read_ply(path, dst), std::runtime_error); +} + +TEST_F(PLYTest, UnknownElementValidListCount_SkipsCorrectly) +{ + // The bound must not break the case it guards. A well-formed unknown + // element is skipped and the vertices that follow read correctly. + const auto path = ply("unknown_valid_count"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_little_endian 1.0\n" + << "element blob 1\n" + << "property list char double junk\n" + << "element vertex 2\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "element face 0\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + const uint8_t count = 3; + f.write(reinterpret_cast(&count), 1); + const double junk[3] = {-9.5, -8.5, -7.5}; + f.write(reinterpret_cast(junk), sizeof(junk)); + const float verts[6] = {1.f, 2.f, 3.f, 4.f, 5.f, 6.f}; + f.write(reinterpret_cast(verts), sizeof(verts)); + } + + Mesh3f dst; + read_ply(path, dst); + ASSERT_EQ(dst.num_vertices(), 2u); + EXPECT_NEAR(dst.vertex(0)[0], 1.f, 1e-6f); + EXPECT_NEAR(dst.vertex(0)[1], 2.f, 1e-6f); + EXPECT_NEAR(dst.vertex(0)[2], 3.f, 1e-6f); + EXPECT_NEAR(dst.vertex(1)[0], 4.f, 1e-6f); + EXPECT_NEAR(dst.vertex(1)[1], 5.f, 1e-6f); + EXPECT_NEAR(dst.vertex(1)[2], 6.f, 1e-6f); +} + +TEST_F(PLYTest, VertexElementWithListProperty_Throws) +{ + // The binary vertex reader sizes each record by summing its properties' + // element widths, which is only correct when every property is a single + // scalar. A list property occupies a count plus N values, so the record + // size comes out short, every read is misaligned, and vertices after the + // first are garbage — with no error. + // + // Neither the binary nor the ASCII vertex path interprets a list property, + // so the file is refused rather than half-read. + const auto path = ply("vertex_list_prop"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_little_endian 1.0\n" + << "element vertex 2\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "property list uchar int extra\n" + << "element face 0\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + for (int vi = 0; vi < 2; ++vi) { + const float v[3] = { + static_cast(3 * vi + 1), + static_cast(3 * vi + 2), + static_cast(3 * vi + 3)}; + f.write(reinterpret_cast(v), sizeof(v)); + const uint8_t n = 1; + const int32_t val = 7; + f.write(reinterpret_cast(&n), 1); + f.write(reinterpret_cast(&val), 4); + } + } + + Mesh3f dst; + EXPECT_THROW(read_ply(path, dst), std::runtime_error); +} + +TEST_F(PLYTest, VertexElementWithListProperty_ASCII_Throws) +{ + // Same declaration, ASCII. The ASCII vertex loop indexes tokens by + // property position, which a list property also breaks, so it is refused + // for the same reason. + const auto path = ply("vertex_list_prop_ascii"); + { + std::ofstream f(path); + f << "ply\n" + << "format ascii 1.0\n" + << "element vertex 2\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "property list uchar int extra\n" + << "element face 0\n" + << "property list uchar int vertex_indices\n" + << "end_header\n" + << "1 2 3 1 7\n" + << "4 5 6 1 7\n"; + } + + Mesh3f dst; + EXPECT_THROW(read_ply(path, dst), std::runtime_error); +} + +// A write failure that happens only in the final flush. +// +// write_ply checks the stream while the tail of the data may still be +// buffered; the flush happens when the ofstream is destroyed, and a failure +// there is swallowed, so write_ply returns normally on an incomplete file. +// +// Isolating that needs the failure to occur at close and nowhere earlier. With +// RLIMIT_FSIZE at 0 every write to the file fails, and with a mesh smaller than +// the stream buffer no write is attempted until close — so the stream is still +// good when write_ply's check runs, and only the flush fails. +// +// POSIX-only: there is no portable way to provoke this. +#if defined(__unix__) || defined(__APPLE__) +TEST_F(PLYTest, WriteFailureInFinalFlush_Throws) +{ + // Exceeding RLIMIT_FSIZE raises SIGXFSZ, which by default kills the + // process; ignore it so the write reports EFBIG instead. + struct Guard { + rlimit saved{}; + void (*prev_sigxfsz)(int){nullptr}; + bool active{false}; + Guard() + { + if (::getrlimit(RLIMIT_FSIZE, &saved) != 0) { + return; + } + prev_sigxfsz = std::signal(SIGXFSZ, SIG_IGN); + rlimit zero{0, saved.rlim_max}; + active = ::setrlimit(RLIMIT_FSIZE, &zero) == 0; + } + ~Guard() + { + if (active) { + (void)::setrlimit(RLIMIT_FSIZE, &saved); + } + if (prev_sigxfsz != nullptr) { + (void)std::signal(SIGXFSZ, prev_sigxfsz); + } + } + } guard; + if (!guard.active) { + GTEST_SKIP() << "cannot lower RLIMIT_FSIZE in this environment"; + } + + // A triangle is a few hundred bytes — comfortably inside the stream + // buffer, so nothing reaches the filesystem before close. + const auto src = make_triangle(); + const auto path = ply("flush_fails"); + EXPECT_THROW(write_ply(path, src), std::runtime_error); +} +#endif + TEST_F(PLYTest, ExcessiveTexcoordCount_Throws) { const auto path = ply("excessive_texcoord"); From aa618a17404ce188ce2d57056a66eec1ad28561b Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:56:14 -0400 Subject: [PATCH 2/2] fix(MeshIO): make malformed PLY input an error instead of a wrong mesh Three cases where read_ply or write_ply produced a wrong result rather than failing. All predate #28 and are independent of binary write and endianness. 1. Unknown-element list skip had no bound on the element count. PLY permits a signed count type, and a 0xFF count byte read as char is -1: as an unsigned byte total that wraps to a negative seek, which skips nothing, so the element's payload was parsed as vertex data. The count is now bounded before it is multiplied. kMaxFaceVertices and kMaxFaceListLength move from duplicated function-local constants in the two face helpers to namespace scope so all three skip sites share one bound. 2. A list property on the vertex element was silently misread. The binary reader sizes each record by summing its properties' scalar widths and the ASCII reader indexes tokens by property position, so neither accounts for a count plus N values: the first vertex read correctly and every later one came from the wrong offset. Such a file is now refused. 3. write_ply checked the stream while the tail of the data was still buffered. The final flush happens when the ofstream is destroyed and its failure is swallowed, so a write that failed only at flush time returned normally on an incomplete file. All three tiers now close before checking. Found by code review of #28. Co-Authored-By: Claude Opus 5 --- include/educelab/core/io/MeshIO_PLY.hpp | 59 +++++++++++++++++++++---- 1 file changed, 50 insertions(+), 9 deletions(-) diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index e2228f2..97171bb 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -437,6 +437,19 @@ auto read_ply_prop_from_buf(const char* buf, PLYType type) -> DestT // Face record parsing helpers // ------------------------------------------------------------------------- +// Bounds on a list property's element count. Legitimate values are small. +// Unknown list properties are skipped rather than interpreted, but the count +// still governs how many bytes are advanced, so it needs a bound there too: +// PLY permits a signed count type, and a negative count converted to an +// unsigned byte total wraps to a seek that skips nothing, leaving the reader +// misaligned inside the element it meant to step over. + +/** @brief Largest @c vertex_indices count @ref read_ply will accept */ +constexpr std::size_t kMaxFaceVertices = 256; + +/** @brief Largest count @ref read_ply will accept for any other list */ +constexpr std::size_t kMaxFaceListLength = 1024; + /** @brief Parse one binary face record from @p file. * * Populates @p face with vertex indices and, when @p load_texcoords is @@ -452,13 +465,6 @@ inline void read_ply_face_binary( std::vector& face, std::vector& texcoords) { - // Cap any list property inside a face record. Legitimate values are small: - // vertex_indices is capped at 256 corners, texcoord is 2 floats per corner - // (<= 512). Unknown list properties are skipped but the count still governs - // how many bytes we read — without a cap, a hostile file can ask us to - // advance an unbounded number of bytes. - constexpr std::size_t kMaxFaceVertices = 256; - constexpr std::size_t kMaxFaceListLength = 1024; face.clear(); texcoords.clear(); for (const auto& prop : elem.props) { @@ -538,8 +544,6 @@ inline void read_ply_face_ascii( std::vector& face, std::vector& texcoords) { - constexpr std::size_t kMaxFaceVertices = 256; - constexpr std::size_t kMaxFaceListLength = 1024; face.clear(); texcoords.clear(); std::size_t ti = 0; @@ -716,6 +720,14 @@ void read_ply_impl( } const auto count = read_ply_binary_prop(file, prop.list_count_type); + // Bound before multiplying. A negative count from a signed count type + // arrives here as a huge unsigned value, and the byte total would wrap + // to a negative seek that skips nothing at all. + if (count > kMaxFaceListLength) { + throw std::runtime_error( + "read_ply: list property count " + to_string(count) + + " exceeds maximum of " + to_string(kMaxFaceListLength)); + } file.ignore( static_cast(count * ply_type_bytes(prop.type))); }; @@ -738,6 +750,20 @@ void read_ply_impl( std::vector tokens; for (const auto& elem : hdr.elements) { if (elem.name == "vertex") { + // Neither vertex path can interpret a list property: the binary + // reader sizes each record by summing its properties' scalar + // widths, and the ASCII reader indexes tokens by property + // position. A list occupies a count plus N values, so both would + // read the first vertex correctly and every later one from the + // wrong offset. Refuse the file rather than return garbage. + for (const auto& p : elem.props) { + if (p.is_list) { + throw std::runtime_error( + "read_ply: list property '" + p.name + + "' on the vertex element is not supported"); + } + } + // Pre-compute binary vertex record layout so the inner loop makes // one file.read() per vertex (O(vertices)) instead of one read // per property per vertex (O(properties × vertices)). @@ -1129,6 +1155,11 @@ void write_ply( file, buf, mesh, static_cast*>(nullptr), has_normals, has_colors); + // Close before checking. The stream may still hold buffered data at this + // point; the final flush happens when `file` is destroyed, and a failure + // there would be swallowed, so write_ply would return normally on an + // incomplete file. close() performs that flush and records its failure. + file.close(); if (!file) { throw std::runtime_error( "write_ply: I/O error while writing file: " + path.string()); @@ -1170,6 +1201,11 @@ void write_ply( detail::write_ply_header(file, mesh, "", true, has_normals, has_colors); detail::write_ply_data(file, buf, mesh, &uvmap, has_normals, has_colors); + // Close before checking. The stream may still hold buffered data at this + // point; the final flush happens when `file` is destroyed, and a failure + // there would be swallowed, so write_ply would return normally on an + // incomplete file. close() performs that flush and records its failure. + file.close(); if (!file) { throw std::runtime_error( "write_ply: I/O error while writing file: " + path.string()); @@ -1218,6 +1254,11 @@ void write_ply( file, mesh, texture_path.string(), true, has_normals, has_colors); detail::write_ply_data(file, buf, mesh, &uvmap, has_normals, has_colors); + // Close before checking. The stream may still hold buffered data at this + // point; the final flush happens when `file` is destroyed, and a failure + // there would be swallowed, so write_ply would return normally on an + // incomplete file. close() performs that flush and records its failure. + file.close(); if (!file) { throw std::runtime_error( "write_ply: I/O error while writing file: " + path.string());