Skip to content

Handle Draco zero faces and short point mappings - #1448

Merged
j9liu merged 1 commit into
CesiumGS:mainfrom
bjornblissing:fix/draco-malformed-mesh-decoding
Sep 24, 2026
Merged

j9liu merged 1 commit into
CesiumGS:mainfrom
bjornblissing:fix/draco-malformed-mesh-decoding

Conversation

@bjornblissing

Copy link
Copy Markdown
Contributor

Description

Hardens Draco mesh decoding against malformed data that can otherwise trigger
undefined behavior or leave stale compressed accessors in the decoded model.

The change handles two cases in CesiumGltfReader/src/decodeDraco.cpp:

  • A Draco mesh with zero faces no longer attempts to access its first face while
    constructing the decoded index buffer. The zero-length buffer and zero-count
    index accessor are retained, and index copying is skipped safely.
  • A non-identity Draco attribute mapping that contains fewer entries than the
    mesh has points is rejected before Draco is indexed by point number. A warning
    is emitted and the corresponding attribute is removed from the primitive,
    rather than copying out-of-bounds data or leaving the original compressed
    accessor advertised after the Draco extension is removed.

This keeps the resulting glTF model internally consistent even when decoding
crafted or corrupted Draco bitstreams.

Issue number or link

N/A

Author checklist

  • I have submitted a Contributor License Agreement (only needed once).
  • I have done a full self-review of my code.
  • I have updated CHANGES.md with a short summary of my change (for user-facing changes).
  • I have added or updated unit tests to ensure consistent code coverage as necessary.
  • I have updated the documentation as necessary.

Testing plan

Exercise Draco decoding with malformed or edge-case meshes covering:

  1. A mesh that decodes successfully but contains zero faces. Verify that
    decoding does not access a nonexistent face and produces a zero-count index
    accessor with an empty buffer.
  2. A mesh containing a non-identity attribute mapping shorter than the decoded
    point count. Verify that decoding emits a warning and removes the affected
    attribute from the primitive instead of reading beyond the mapping.
  3. A normal Draco mesh with identity and complete attribute mappings. Verify
    that valid decoded attributes and indices are still copied as before.

The changes are defensive checks in the Draco decoding path; no user-facing
format or API changes are introduced.

@j9liu j9liu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bjornblissing for the PR! I just have two small comments that are doc-related.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth mentioning these fixes in CHANGES.md, e.g., something like "fixed potential out-of-bounds access while decoding Draco-compressed glTFs."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a note in the changes.md

Comment on lines +238 to +239
if (!pAttribute->is_mapping_identity() &&
pAttribute->indices_map_size() < pMesh->num_points()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this comment be correct? I'm not familiar with how Draco works, but a small explainer would help:

Suggested change
if (!pAttribute->is_mapping_identity() &&
pAttribute->indices_map_size() < pMesh->num_points()) {
// For explicit index mapping, the remapped indices must match the
// number of points in the mesh.
if (!pAttribute->is_mapping_identity() &&
pAttribute->indices_map_size() < pMesh->num_points()) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment explaining the check.

@j9liu j9liu added this to the October 2026 Release milestone Sep 21, 2026
A crafted or corrupted Draco bitstream can decode to a mesh with
zero faces or to a non-identity attribute mapping that covers fewer
points than the mesh contains. Taking the address of the first face
of an empty mesh is undefined behaviour and aborts under
_GLIBCXX_ASSERTIONS, and the mapping is indexed by point index
without any bounds check inside Draco, so a short mapping reads out
of bounds and copies arbitrary memory into the decoded attribute.

Keep the model consistent when data is skipped: a zero-face mesh
produces a valid zero-count index accessor and only the copy is
skipped, and an attribute with a short mapping is removed from the
primitive with a warning, so consumers cannot read an accessor that
still describes compressed data after the Draco extension is
stripped.
@bjornblissing
bjornblissing force-pushed the fix/draco-malformed-mesh-decoding branch from b50c112 to 32ba13e Compare September 24, 2026 17:24

@j9liu j9liu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bjornblissing !

@j9liu
j9liu merged commit 50019ff into CesiumGS:main Sep 24, 2026
14 checks passed
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.

2 participants