Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly parse primitive type enum #233

Merged
merged 2 commits into from
Nov 27, 2023
Merged

Explicitly parse primitive type enum #233

merged 2 commits into from
Nov 27, 2023

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Oct 26, 2023

Instead of blindly casting int to enum, we handle the values defined in the JSON spec in a switch case.

This may seem redundant, but this fixes a UBSAN error on invalid glTF files, and it's literally the only enum where we rely on the enum values matching glTF values (everything else is either string based in glTF, or using integers that aren't small and sequential so we convert it with a switch). So this is more consistent and a little safer; this also ensures that the values actually conform to the max_enum member.

zeux added 2 commits October 25, 2023 20:10
Instead of blindly casting int to enum, we handle the values defined in
the JSON spec in a switch case.

This may seem redundant, but this fixes a UBSAN error on invalid glTF
files, and it's literally the only enum where we rely on the enum values
matching glTF values (everything else is either string based in glTF, or
using integers that aren't small and sequential so we convert it with a
switch). So this is more consistent and a little safer; this also
ensures that the values actually conform to the max_enum member.
zeux added a commit to zeux/meshoptimizer that referenced this pull request Oct 26, 2023
This is necessary to fix UBSAN crashes on invalid inputs, and it is the
only enum that was directly casted from int.

The fix is submitted upstream: jkuhlmann/cgltf#233
@jkuhlmann jkuhlmann merged commit 3ef90fe into jkuhlmann:master Nov 27, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants