zeux
commented
9 months ago This is easier to review with ignored whitespace in the diff. Also note that I am discovering these as I go; we previously never asked the question, can you meaningfully use various cgltf accessor functions on the result of cgltf_parse, and these changes are attempting to answer that :) I expect there will be a few more PRs in the coming days that fix corner cases like this.
This change attempts to flag more invalid glTF files as invalid when this likely interferes with the mesh processing by causing crashes.
Fix parsing of cgltf_size by using cgltf_json_to_size in a couple places where int was used erroneously and replacing negative numbers with 0. We would previously return -1 when the JSON token was invalid and an arbitrary negative value if a negative number was present, but this runs a high risk of an integer overflow during any of the size computation, which causes a security risk. In the future it would be nicer to use a limit for positive values as well (for example, glTF spec says that all sizes should be limited to 2^53), as with this change there is still a risk of overflow during multiplication.
Fix validation of accessors - an invalid component type or type may lead to computed accessor stride to be 0 which may invalidate all sorts of logic. Both component type and type are mandatory per glTF spec.
Make sure that indices in mesh primitives have no custom stride. This is enforced by the glTF spec, and cgltf_calc_index_bound ignores the stride, which makes it possible to craft an index buffer that passes index validity checks but results in bogus indices later. This observation may also allow us to simplify cgltf_accessor_unpack_indices in the future.
Make sure that there's at least one primitive. This is guaranteed by the spec, and without this it's possible to specify indices without vertices.
Make sure that there's at least one vertex. This is guaranteed by the spec as all accessors must have count >= 1 (for now this is checked just for attribute accessors, in the future it might make sense to move this check to accessor validation). Without this, it's possible to have an index buffer filled with zeroes (without a buffer view, accessors are zero-initialized...), so we'd never check the index bound.