Skip to content

Make the Draco bufferView bounds check overflow-safe - #1449

Merged
j9liu merged 2 commits into
CesiumGS:mainfrom
bjornblissing:fix/draco-bounds-check-overflow
Sep 24, 2026
Merged

j9liu merged 2 commits into
CesiumGS:mainfrom
bjornblissing:fix/draco-bounds-check-overflow

Conversation

@bjornblissing

@bjornblissing bjornblissing commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Description

decodeBufferViewToDracoMesh (CesiumGltfReader/src/decodeDraco.cpp) validated a Draco bufferView against its owning buffer with:

bufferView.byteOffset + bufferView.byteLength > buffer.cesium.data.size()

Both byteOffset and byteLength come directly from untrusted JSON as signed 64-bit integers. Their sum can overflow and wrap around to a negative or small value, which would pass the > check even though the bufferView actually extends beyond (or entirely outside) the buffer. Draco would then be handed a span pointing outside the buffer's allocation.

The fix rewrites the check to avoid the overflow-prone addition entirely: it first confirms byteOffset doesn't exceed the buffer size, then compares byteLength against the remaining space (bufferSize - byteOffset), which cannot overflow since byteOffset <= bufferSize is already established at that point.

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

  1. Construct a glTF model with a KHR_draco_mesh_compression bufferView whose byteOffset and byteLength are both large, valid-looking int64_t values that sum to an overflowed (negative or wrapped) result. Before the fix, the bounds check incorrectly passes and Draco is given an out-of-bounds span. After the fix, the check correctly rejects it and emits the "Draco bufferView extends beyond its buffer." warning.
  2. Verify normal, in-bounds bufferViews (including byteOffset == 0 and a bufferView that exactly spans the whole buffer) still decode successfully with no false-positive warnings.
  3. Verify a bufferView that legitimately extends beyond the buffer (without overflow) is still correctly rejected, matching prior behavior.

This is a targeted arithmetic fix in the bounds-check path with no user-facing format or API changes.

Both operands come straight from JSON, so adding them could overflow and
wrap negative, letting the guard pass and handing Draco a span pointing
outside the buffer.

@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 ! Can you fix the formatting by using npm run format on the base repo folder? Then this will be ready to go.

Cover the overflow-safe bounds check on Draco bufferViews: an
offset near INT64_MAX combined with a byteLength must be rejected
rather than wrapping past the guard and handing Draco a span
outside the buffer. Without the accompanying fix, this reproduces
as a crash instead of a warning.
@bjornblissing
bjornblissing force-pushed the fix/draco-bounds-check-overflow branch from b02d92c to da5ebba Compare September 24, 2026 17:28

@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 ab279bc into CesiumGS:main Sep 24, 2026
14 checks passed
@j9liu j9liu added this to the October 2026 Release milestone Sep 24, 2026
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