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

[MODELS] Fix crash with some GLTF models #2867

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Conversation

JeffM2501
Copy link
Contributor

When a buffer is read using cgltf_load_buffer_base64, the data is a raw void*, so we can't call cgltf_free. cgltf_free assumes the data is a cgltf_data structure and will try to free the various buffers in that structure.
This causes a crash with some files (attached)
house.zip

This PR calls a normal free for the data, so it's properly deallocated so it won't crash.
I could not find a pure free method in cgltf, so I just called raylib's free since it looks like that is what is installed for cgltf's allocators anyway.

@raysan5 raysan5 merged commit 116603e into raysan5:master Jan 19, 2023
@raysan5
Copy link
Owner

raysan5 commented Jan 19, 2023

@JeffM2501 This issue could be a bit tricky...

data should be freed by the same allocation library used to allocate it. In this case, cgltf calls cgltf_default_alloc() that calls CGLTF_MALLOC that rmodels map to RL_CALLOC, so, calling MemFree() that calls RL_FREE end-up using the right allocators pair... BUT there could be some situation, like user setting up custom allocation/free pairs on raylib, that could let to undesired effects.

Clearly cgltf_free() can not be used to unload data. Ideally, cgltf_load_buffer_base64() should provide a unload function equivalent... In the meantime, I'm merging this fix.

Related to jkuhlmann/cgltf#200

@JeffM2501
Copy link
Contributor Author

Yeah, I was hoping they exposed a proper free, but I didn't see one, so yeah it's tricky.

@JeffM2501 JeffM2501 deleted the gltf_free branch January 19, 2023 20:28
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