Loading this file causes a crash.

[sol-vin]

When using Crocotile 3D to generate gltf files I found one that crashes raylib (which uses cgltf).

INFO: FILEIO: [rsrc/house.gltf] File loaded successfully
INFO: MODEL: [rsrc/house.gltf] Model basic data (glTF) loaded successfully
INFO:     > Meshes count: 1
INFO:     > Materials count: 1 (+1 default)
INFO: IMAGE: Data loaded successfully (512x512 | R8G8B8A8 | 1 mipmaps)
Invalid memory access (C0000005) at address 0xffffffffffffffff
[0x7ffa85918e17] cgltf_free at C:\Users\Ian\Documents\GitHub\raylib\src\external\cgltf.h:1767
[0x7ffa85941f71] LoadImageFromCgltfImage at C:\Users\Ian\Documents\GitHub\raylib\src\rmodels.c:4664
[0x7ffa8593dfa5] LoadGLTF at C:\Users\Ian\Documents\GitHub\raylib\src\rmodels.c:4829
[0x7ffa8590048e] LoadModel at C:\Users\Ian\Documents\GitHub\raylib\src\rmodels.c:1031
[0x7ff671cd4698] load at C:\Users\Ian\Documents\GitHub\ville\gltf_test\src\gltf_test.cr:157

File in question is attached. house.zip

[raysan5]

The crash was probably related to the attached issue. Image data loaded with cgltf_load_buffer_base64() can not be freed with cgltf_free(). It should be freed with corresponding de-allocator provided that by default fallbacks to cgltf_default_free(), that calls CGLTF_FREE().

Current solution was freeing it in raylib side with MemFree() that relies in RL_FREE(), it works as expected because CGLTF_FREE maps to RL_FREE() but not sure if there could be some potential issue if custom allocators are defined in one side or the other.

[jkuhlmann]

So, am I getting this right, it's not really a bug in cgltf, but the documentation for cgltf_load_buffer_base64() could potentially be clearer?

[raysan5]

@jkuhlmann It's not really a bug but it can be an issue for users and potential library bindings, I had a similar issue with raylib allocators.

When generating a .dll using the library, the allocators (malloc/free pair) could use a different memory context than the ones used by the user.

For example, here it is one function provided by raylib:

char *LoadFileText(const char *fileName);   // Load text data from file (read), returns a '\0' terminated string

This function loads a char array using the internal allocator, only configurable by a macro at raylib compile time (raylib does not provide callbacks for custom allocators config like cgltf).

The problem is that most users could be tempted to use this function in the following way:

char *textData = LoadFileText("game_dialog.txt");
//...
free(textData);      // WARNING: This could generate a crash if the user-side allocator does not match the one used internally by raylib!

The solution I adopted for raylib was always providing a Unload*() equivalent for all functions loading memory to avoid the allocators missmatch, that was my approach (considering most raylib users could be unexperience developers). I also exposed MemFree() calling the internall de-allocator, just in case.

char *textData = LoadFileText("game_dialog.txt");
//...
UnloadFileText(textData);      // Correct internall de-allocator used

cgltf uses a different and more professional approach. It provides the option of setting custom allocators and also cgltf_load_buffer_base64() supports the cgltf_options parameter to configure them. But, in case of a naive use of the function, it could happen the same issue:

void *data = NULL;
cgltf_options options = { 0 };
cgltf_result result = cgltf_load_buffer_base64(&options, outSize, cgltfImage->uri, &data);
//...
free(data);   // WARNING: This could generate a crash if the user-side allocator does not match the one used by cgltf

User side solution is just setting the cgltf_options allocators correctly but there could be some side-case (cgltf build inside a DLL + binding + separate compilations (lib/user-code) + unexperienced user) that code could crash.

Considering cgltf is more intended for coders with experience I think a note in the documentation could be enough.

[zeux]

cgltf_free is perhaps a little unfortunate as far as names go :D looks like this was the source of the bug, not the regular allocator mismatch? We could rename it and keep the old name for compatibility, but worth noting is that even a C compiler should warn on the call to cgltf_free given a high enough warning setting.