google / google/filament

gltfio: heap OOB read in uploadBuffers() - accessor count vs morphTargetCount mismatch

Open Beginner friendly
#10,197 3 comments 0 reactions 0 assignees View on GitHub
gltf security
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.