Skip to content

gltfio: run cgltf_validate() (and honor its result) in release builds #10073

@f0r

Description

@f0r

Summary

gltfio validates loaded glTF/GLB assets only in debug builds, so release/RelWithDebInfo builds
(the shipped configurations — the gltfio-android AAR, the WASM build, and apps built in release)
load assets without validation. Malformed or non-conformant assets are therefore not checked before
the buffer-upload and accessor-unpack paths, which can lead to crashes or out-of-bounds access on
bad input.

Details

Two spots:

  1. libs/gltfio/src/Utility.cpp, loadCgltfBuffers()cgltf_validate() is guarded by
    #ifndef NDEBUG, so it runs only in debug builds:

    #ifndef NDEBUG
        if (cgltf_validate((cgltf_data*) gltf) != cgltf_result_success) {
            slog.e << "Failed cgltf validation." << io::endl;
            return false;
        }
    #endif
  2. libs/gltfio/src/ResourceLoader.cpp, loadResources() — the return value of
    loadCgltfBuffers() is ignored, so even when it does fail the load continues:

    utility::loadCgltfBuffers(gltf, pImpl->mGltfPath.c_str(), pImpl->mUriDataCache);

As a result, accessor / bufferView / sparse-index bounds are not enforced in release before
uploadBuffers(), cgltf_accessor_unpack_floats(), and getOrCreateTexture() use file-derived
offsets, sizes, and indices. These are the same upload/unpack paths the recent hardening (commits
8d16ad8 and d41cfac) noted it did not cover (standard vertex/index upload, meshopt, texture,
sparse).

Possible approaches

  1. Run cgltf_validate() in all build configurations. Simplest and complete, but stricter than
    today's release behavior (would reject some non-conformant assets that currently load) and adds a
    validation pass per load.
  2. A minimal validator that checks only the bounds-relevant conditions (accessor bounds, sparse
    index < count, meshopt/bufferView bounds), skipping the strict structural/stride checks — lower
    chance of rejecting real-world assets; also handles sparse globally (it's unpacked at several
    sites).
  3. Targeted per-path bounds checks with graceful skip/clamp + warning, matching the style of the
    recent ResourceLoader hardening. No behavior change for valid assets.

Regardless of approach, I'd suggest also: honoring loadCgltfBuffers()'s return value, applying the
check to both the standard and extended-algorithm paths, and setting mResourcesLoaded after a
successful load (so a failed load can be retried).

I have each approach prototyped and verified (a malformed asset is rejected/handled, a valid model
still loads), plus a unit test following the existing FilaflatSecurityTest precedent. Happy to send
a PR in whichever form you prefer.

Metadata

Metadata

Labels

gltfSpecific to glTF supportsecurity

Type

No fields configured for Bug.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions