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

[destro4evr-rgb]

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):

const cgltf_accessor* srcMatrices = srcSkin.inverse_bind_matrices;
FixedCapacityVector<mat4f> 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):

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.

--- 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<mat4f> 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),