Skip to content

fix: validate OBJ face indices against declared v/vt/vn counts#1199

Open
SongTonyLi wants to merge 1 commit into
google:mainfrom
SongTonyLi:fix/issue-1194-obj-decoder-oob-read
Open

fix: validate OBJ face indices against declared v/vt/vn counts#1199
SongTonyLi wants to merge 1 commit into
google:mainfrom
SongTonyLi:fix/issue-1194-obj-decoder-oob-read

Conversation

@SongTonyLi
Copy link
Copy Markdown

@SongTonyLi SongTonyLi commented May 6, 2026

Summary

Fixes #1194.

I found that ParseVertexIndices in obj_decoder.cc only checks that face indices are non-zero — it never actually checks whether they're within the range of declared v/vt/vn entries. So an OBJ file like f 1/100/1 2/100/2 3/100/3 (where only 3 vt entries exist) gets accepted by the parser, and the bogus index 100 makes it all the way through MapPointToVertexIndices into the dedup stage, where DeduplicateFormattedValues reads out of bounds at point_attribute.cc:219.

What's going on

I traced through the code and here's the flow:

  1. ParseVertexIndices parses the face line and sees vt=100. It checks index != 0 (passes) but never checks index <= num_tex_coords_ (should fail). So 100 gets through.
  2. MapPointToVertexIndices converts it to AttributeValueIndex(100 - 1) = 99 and stores it in the attribute map — no questions asked.
  3. Later, DeduplicateFormattedValues does value_map[indices_map_[i]] where indices_map_[i] is 99, but value_map only has 3 entries. Heap OOB read.

I added some instrumentation probes to confirm and saw exactly this:

  • ParseVertexIndices emitted {v:1, vt:100, vn:1} while counts were {v:3, vt:3, vn:3}
  • MapPointToVertexIndices flagged oob={v:0, vt:1, vn:0} and stored mapped index 99

Fix

I added a bounds check (indices_in_bounds lambda) in ParseVertexIndices that validates each parsed v/vt/vn index against the declared pool size before returning. It handles both positive and negative OBJ index forms (OBJ supports negative indexing like -1 for "last declared entry").

I went with fixing it here instead of at the crash site because ParseVertexIndices is where untrusted OBJ text first becomes internal indices — it already returns false on error and callers already handle that, so this felt like the natural place. A guard at DeduplicateFormattedValues would only protect that one code path while leaving bad indices in indices_map_ for anything else that reads them.

Testing

Built with ASan (clang 14, -fsanitize=address):

  • Before: ERROR: AddressSanitizer: heap-buffer-overflow — READ of size 4 in DeduplicateFormattedValues<float, 2> at point_attribute.cc:219
  • After: Failed loading the input mesh: Failed to parse vertex indices. — clean exit, no sanitizer output
  • Valgrind: ERROR SUMMARY: 0 errors from 0 contexts, all heap blocks freed
  • Sanity check: testdata/triangle.obj still encodes fine (exit 0) — valid files aren't affected

Reproducer:

cat > /tmp/crash.obj << 'OBJ'
v 0 0 0
v 1 0 0
v 0 1 0
vt 0 0
vt 0 0
vt 0 0
vn 0 0 1
vn 0 0 1
vn 0 0 1
f 1/100/1 2/100/2 3/100/3
OBJ
draco_encoder -i /tmp/crash.obj -o /tmp/out.drc

…e#1194)

ParseVertexIndices accepted any non-zero face index without checking it
against the number of declared v/vt/vn entries.  Out-of-range indices
propagated through MapPointToVertexIndices into the attribute dedup
stage, causing a heap-buffer-overflow read in
PointAttribute::DeduplicateFormattedValues.

Add bounds validation in ParseVertexIndices for position, texcoord, and
normal indices (both positive and negative OBJ forms) so the decoder
rejects malformed faces at parse time rather than crashing downstream.
@SongTonyLi SongTonyLi marked this pull request as ready for review May 6, 2026 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Out-of-bounds read in draco::PointAttribute::DeduplicateFormattedValues

1 participant