google / google/filament

gltfio: heap OOB read in updateBoneMatrices() - asset->mSkins desync when createSkins() skips validation failures

Open
#10,198 1 comment 0 reactions 0 assignees View on GitHub
gltf security
Dominant language
C++
Stars
20.5k
Forks
2.3k
Avg merge
2d 14h
Merged PRs (30d)
83

Description

### Summary

`ResourceLoader::createSkins()` uses `continue` to skip `emplace_back` whenever an `inverse_bind_matrices` accessor fails validation (wrong type, count too small, missing buffer view, buffer bounds exceeded). `AssetLoader::importSkins()` runs independently and always pushes exactly `skins_count` entries into `instance->mSkins` via `resize()`. When any skin is skipped, `asset->mSkins.size()` ends up less than `skins_count` while `instance->mSkins.size()` stays at `skins_count`.

`Animator::updateBoneMatrices()` then iterates `instance->mSkins` and indexes `asset->mSkins` by a monotonically-incrementing counter — `asset->mSkins[skinIndex++]` — with no bounds check. When the counter reaches the shortened `asset->mSkins.size()`, it reads past the end of the heap allocation.

I originally submitted this as PR #10189 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` — `createSkins()` (~line 239):**

```cpp
const cgltf_accessor* srcMatrices = srcSkin.inverse_bind_matrices;
FixedCapacityVector inverseBindMatrices(srcSkin.joints_count);
if (srcMatrices) {
if (srcMatrices->type != cgltf_type_mat4 ||
srcMatrices->component_type != cgltf_component_type_r_32f) {
LOG(WARNING) << "Cannot copy inverse bind matrices, unsupported accessor type.";
continue; // skips emplace_back — asset->mSkins ends up short
}
if (srcMatrices->count < srcSkin.joints_count) {
LOG(WARNING) << "Cannot copy inverse bind matrices, accessor count is too small.";
continue; // same
}
if (!srcMatrices->buffer_view || ...) {
LOG(WARNING) << "Cannot copy inverse bind matrices, missing buffer view or buffer data.";
continue; // same
}
if (offsetInView > viewSize || requiredBytes > viewSize - offsetInView) {
LOG(WARNING) << "Cannot copy inverse bind matrices, accessor data exceeds buffer view bounds.";
continue; // same
}
if (!srcMatrices->buffer_view->data) {
LOG(WARNING) << "Cannot copy inverse bind matrices, compressed buffer data is null.";
continue; // same
}
if (totalOffset > bufferSize || requiredBytes > bufferSize - totalOffset) {
LOG(WARNING) << "Cannot copy inverse bind matrices, accessor data exceeds buffer bounds.";
continue; // same — 6 paths in total all skip emplace_back
}
memcpy(...);
}
// emplace_back only reached when ALL checks pass
FFilamentAsset::Skin skin{ .name = ..., .inverseBindMatrices = ... };
asset->mSkins.emplace_back(std::move(skin));
```

**`libs/gltfio/src/Animator.cpp` — `updateBoneMatrices()` (~line 616):**

```cpp
assert_invariant(instance->mSkins.size() == asset->mSkins.size()); // no-op in release
size_t skinIndex = 0;
for (const auto& skin : instance->mSkins) {
const auto& assetSkin = asset->mSkins[skinIndex++]; // OOB when skinIndex >= asset->mSkins.size()
...
}
```

### Impact

Heap OOB read triggered during `Animator::updateBoneMatrices()` when any skin's `inverse_bind_matrices` accessor fails the type or bounds checks in `createSkins()`. A crafted `.glb` with a skin using a VEC3 accessor instead of MAT4 passes `cgltf_validate()` cleanly and triggers the desync on load.

### Suggested fix

The fix is to wrap all validation inside an immediately-invoked lambda so that on any failure the code falls through to `emplace_back` with identity matrices rather than skipping it, keeping `asset->mSkins` index-aligned with `instance->mSkins`. A bounds guard in `updateBoneMatrices()` acts as a secondary safety net.

```diff
--- a/libs/gltfio/src/Animator.cpp
+++ b/libs/gltfio/src/Animator.cpp
@@ -616,6 +616,9 @@ void AnimatorImpl::updateBoneMatrices(FFilamentInstance* instance) {
assert_invariant(instance->mSkins.size() == asset->mSkins.size());
size_t skinIndex = 0;
for (const auto& skin : instance->mSkins) {
+ if (UTILS_UNLIKELY(skinIndex >= asset->mSkins.size())) {
+ break;
+ }
const auto& assetSkin = asset->mSkins[skinIndex++];
size_t njoints = skin.joints.size();
boneMatrices.resize(njoints);
--- a/libs/gltfio/src/ResourceLoader.cpp
+++ b/libs/gltfio/src/ResourceLoader.cpp
@@ -239,37 +239,41 @@ inline void createSkins(cgltf_data const* gltf, bool normalize,
const cgltf_accessor* srcMatrices = srcSkin.inverse_bind_matrices;
FixedCapacityVector inverseBindMatrices(srcSkin.joints_count);
- if (srcMatrices) {
+
+ // Validate and copy the inverse bind matrices. On any failure we log a warning and fall
+ // through to emplace_back with identity matrices. The skin entry must always be pushed so
+ // that asset->mSkins stays index-aligned with instance->mSkins, which is built separately
+ // by importSkins() without these checks and always has exactly skins_count entries.
+ [&]() {
+ if (!srcMatrices) return;
if (srcMatrices->type != cgltf_type_mat4 ||
srcMatrices->component_type != cgltf_component_type_r_32f) {
LOG(WARNING) << "Cannot copy inverse bind matrices, unsupported accessor type.";
- continue;
+ return;
}
if (srcMatrices->count < srcSkin.joints_count) {
LOG(WARNING) << "Cannot copy inverse bind matrices, accessor count is too small.";
- continue;
+ return;
}
if (!srcMatrices->buffer_view ||
- (!srcMatrices->buffer_view->has_meshopt_compression &&
- (!srcMatrices->buffer_view->buffer || !srcMatrices->buffer_view->buffer->data))) {
+ (!srcMatrices->buffer_view->has_meshopt_compression &&
+ (!srcMatrices->buffer_view->buffer ||
+ !srcMatrices->buffer_view->buffer->data))) {
LOG(WARNING) << "Cannot copy inverse bind matrices, missing buffer view or buffer data.";
- continue;
+ return;
}
if (offsetInView > viewSize || requiredBytes > viewSize - offsetInView) {
LOG(WARNING) << "Cannot copy inverse bind matrices, accessor data exceeds buffer view bounds.";
- continue;
+ return;
}
if (!srcMatrices->buffer_view->data) {
LOG(WARNING) << "Cannot copy inverse bind matrices, compressed buffer data is null.";
- continue;
+ return;
}
if (totalOffset > bufferSize || requiredBytes > bufferSize - totalOffset) {
LOG(WARNING) << "Cannot copy inverse bind matrices, accessor data exceeds buffer bounds.";
- continue;
+ return;
}
memcpy((uint8_t*) inverseBindMatrices.data(), (const void*) srcBuffer, requiredBytes);
- }
+ }();
+
FFilamentAsset::Skin skin{
.name = std::move(name),
.inverseBindMatrices = std::move(inverseBindMatrices),
```

Contributor guide

Open the contributing guide

Research direction

Start in libs/gltfio/src/ResourceLoader.cpp at createSkins() and libs/gltfio/src/Animator.cpp at updateBoneMatrices(), tracing how asset-mSkins and instance-mSkins are populated and indexed. Exercise the malformed .glb case described in the issue and verify that invalid inverse bind matrices do not desynchronize the skin arrays or cause an out-of-bounds read.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
computer-graphics, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.