gltfio: heap OOB read in uploadBuffers() - accessor count vs morphTargetCount mismatch
- Dominant language
- C++
- Stars
- 20.5k
- Forks
- 2.3k
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 74
Description
### Summary
`uploadBuffers()` in `libs/gltfio/src/ResourceLoader.cpp` allocates `floatsData` with a capacity derived from `accessor->count` (the number of vertices the morph-target accessor actually covers), then passes `slot.morphTargetCount` — the **full mesh vertex count** — as the element count to `setPositionsAt()`. When a glTF asset provides a morph-target accessor whose `count` is less than the mesh vertex count, `setPositionsAt()` reads past the end of the heap allocation.
I originally submitted this as PR #10172 with a fix, but on July 10th all non-maintainer `libs/gltfio` PRs were closed by the repository automation. The bot asked me to open an issue instead, so I am filing it here as requested.
### Affected code
`libs/gltfio/src/ResourceLoader.cpp`
**No-buffer_view path (~line 434):**
```cpp
if (accessor->type == cgltf_type_vec3) {
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
(const float3*)floatsData, slot.morphTargetCount, slot.morphTargetOffset);
// ^^^^^^^^^^^^^^^^ should be accessor->count
}
else {
assert_invariant(accessor->type == cgltf_type_vec4);
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
(const float4*)floatsData, slot.morphTargetCount, slot.morphTargetOffset);
// ^^^^^^^^^^^^^^^^ should be accessor->count
}
```
`floatsData` is allocated for `accessor->count` elements, but `slot.morphTargetCount` (the full mesh vertex count) is passed as the read count. When `accessor->count < slot.morphTargetCount`, `setPositionsAt` reads past the allocation.
**requiresPacking path (~line 628):**
```cpp
if (accessor->type == cgltf_type_vec3) {
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
(const float3*) floatsData,
slot.morphTargetCount, // wrong count — should be safeCount
slot.morphTargetOffset);
} else {
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
(const float4*) data, // wrong pointer — should be floatsData
slot.morphTargetBuffer->getVertexCount(), // wrong count — should be safeCount
slot.morphTargetOffset);
}
```
Both branches are wrong: vec3 passes `slot.morphTargetCount` instead of `safeCount`, and vec4 additionally passes the raw packed buffer pointer `data` instead of the unpacked `floatsData`.
### Impact
Heap OOB read during asset loading when a morph-target accessor declares a `count` smaller than the mesh vertex count. `cgltf_validate()` does not enforce that morph-target accessor counts match the base mesh vertex count, so such a file passes all validation.
### Suggested fix
```diff
--- a/libs/gltfio/src/ResourceLoader.cpp
+++ b/libs/gltfio/src/ResourceLoader.cpp
@@ -432,12 +432,12 @@ inline bool uploadBuffers(FFilamentAsset* asset, Engine& engine,
if (accessor->type == cgltf_type_vec3) {
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
- (const float3*)floatsData, slot.morphTargetCount, slot.morphTargetOffset);
+ (const float3*)floatsData, accessor->count, slot.morphTargetOffset);
}
else {
assert_invariant(accessor->type == cgltf_type_vec4);
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
- (const float4*)floatsData, slot.morphTargetCount, slot.morphTargetOffset);
+ (const float4*)floatsData, accessor->count, slot.morphTargetOffset);
}
@@ -626,11 +626,11 @@ inline bool uploadBuffers(FFilamentAsset* asset, Engine& engine,
if (accessor->type == cgltf_type_vec3) {
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
(const float3*) floatsData,
- slot.morphTargetCount,
+ safeCount,
slot.morphTargetOffset);
} else {
slot.morphTargetBuffer->setPositionsAt(engine, slot.bufferIndex,
- (const float4*) data, slot.morphTargetBuffer->getVertexCount(),
+ (const float4*) floatsData, safeCount,
slot.morphTargetOffset);
}
```
Contributor guide
Research direction
Start in libs/gltfio/src/ResourceLoader.cpp at uploadBuffers(), focusing on the no-buffer_view and requiresPacking paths around lines 434 and 628. Trace how floatsData, accessor->count, safeCount, and the morph-target buffer calls relate. Done means both paths use the allocated unpacked data with a count that cannot exceed it; exercise loading a morph-target asset whose accessor count is below the mesh vertex count.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- computer-graphics
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 70/100