gltfio: run cgltf_validate() (and honor its result) in release builds
- Dominant language
- C++
- Stars
- 20.5k
- Forks
- 2.3k
- Avg merge
- 2d 16h
- Merged PRs (30d)
- 74
Description
### 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:
```cpp
#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:
```cpp
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
8d16ad88 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.
Contributor guide
Assessment
This issue has not been assessed yet.