fix(MeshIO): make malformed PLY input an error instead of a wrong mesh - #29
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
csparker247
added a commit
that referenced
this pull request
Sep 5, 2026
…ary-io_20260904) The manual MeshLab check was completed; the note still said it was outstanding. Records what it found and points at #29 for the robustness fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
csparker247
added a commit
that referenced
this pull request
Sep 5, 2026
…ary-io_20260904) The manual MeshLab check was completed; the note still said it was outstanding. Records what it found and points at #29 for the robustness fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Three cases where
read_plyorwrite_plyproduced a wrong result insteadof an error. All three predate #28 and are independent of binary write and
endianness — found by code review of that PR, fixed here so #28's scope stays
settled.
Branched off
develop.The bugs
1. Unbounded list count on an unknown element → wrong geometry, no error
A PLY may contain elements
read_plydoesn't understand (edge,tristrips,application-defined). It skips them by reading each list's count and jumping
that many bytes. That count had no bound.
PLY permits a signed count type —
property list char double junkis legal.A count byte of
0xFFreads as-1; as an unsigned byte total that becomes~1.8e19, multiplying wraps it back to just below zero, and the cast to
std::streamsizegives -8. A seek of -8 skips nothing, so the reader staysinside the element it meant to step over and parses that payload as vertex data.
With enough bytes remaining, nothing fails:
uchar 0xFF(255) was already safe — it runs off the end of the file andthrows. Only a signed count type reaches the silent path.
Fix: bound the count before multiplying.
kMaxFaceVerticesandkMaxFaceListLengthmove from duplicated function-local constants in the twoface helpers to namespace scope, so all three skip sites share one bound.
2. List property on the vertex element → first vertex right, rest garbage
The binary vertex reader sizes each record once by summing its properties'
scalar widths, then does one
readper vertex. A list property occupies a countplus N values, so the record size comes out short and every read after the first
is misaligned. The ASCII path has the same flaw by a different route — it
indexes tokens by property position.
Fix: refuse the file. Lists on a
vertexelement are legal but rare — listsare conventionally a face thing — and refusing cannot break a working case,
because no such file was ever read correctly.
3.
write_plyreported success on a write that failed at flushEach tier ended with
if (!file) throw;— but that ran while the tail of thedata was still in the stream buffer. The final flush happens when the
ofstreamis destroyed, and a failure there is swallowed, so a write thatfailed only at flush time returned normally on an incomplete file.
Fix:
file.close()before the check, in all three tiers.close()performsthat flush and records its failure.
Testing
Four failing tests first, then the fixes; two additional guard tests so the
fixes can't be over-strict (a well-formed unknown element still skips
correctly, and a genuinely huge count still fails).
TestMeshIO71 → 77 tests. Green in Debug and Release.Bug 3 needed care to test honestly. My first attempt passed both before and
after the fix — worse than no test, since it looks like coverage. The real test
isolates the failure to the flush and nowhere earlier:
RLIMIT_FSIZEat 0 makesevery write to the file fail, and a mesh smaller than the stream buffer means no
write is attempted until
close()— so the stream is still good when the oldcheck ran, and only the flush fails. It's
#if defined(__unix__) || defined(__APPLE__)with the limit and
SIGXFSZdisposition restored by an RAII guard, and skipsvia
GTEST_SKIPwhere the limit can't be lowered. There is no portable way toprovoke this.
Interaction with #28
#28 also hoists
kMaxFaceVertices/kMaxFaceListLengthto namespace scope(so its
validate_ply_face_listscanstatic_assertthe writer's limitsagainst the reader's caps), and it adds a
needs_swapargument to theskip_binary_proplambda touched here. Expect a conflict inMeshIO_PLY.hppon whichever merges second; the resolution is mechanical —keep the hoisted constants once, and keep both the bound and the
needs_swapargument.
If this merges first, #28 rebases and drops its duplicate hoist.