CesiumGS / cesium-native

Apache License 2.0
432 stars 212 forks source link

Fixed crash when loading glTFs with data uri images #753

Closed lilleyse closed 11 months ago

lilleyse commented 12 months ago

I ran into this issue when loading the FeatureIdTexture sample in cesium-omniverse, which has data uri images.

The crash happens when both decodeDataUrls and decodeEmbeddedImages are true. Data uris are decoded first, and then image.uri is reset. decodeEmbeddedImages isn't aware of that and tries to decode the image from a non-existent buffer view.

For now I just guard against the accidental second decode by checking that the decoded data already exists. There could be a cleaner solution but I think that would require more refactoring.

kring commented 11 months ago

Thanks @lilleyse. This change looks reasonable enough, but it's not obvious to me where the crash would be coming from. An invalid buffer should be handled gracefully. At worst I'd expect a log message about being unable to decode the (0-byte) image. Can you tell me how to reproduce the crash? Or if you can easily make it into a test, that would be even better!

lilleyse commented 11 months ago

I added a unit test, which should also make it easier to see what's happening.

The image decode works fine, the problem is that since the uri gets cleared it tries to load the image from a buffer view instead, which doesn't exist. This PR basically returns early if the decoded data already exists.

kring commented 11 months ago

Thanks for adding the test, but I'm still confused about the crash. I reverted both GltfReader.cpp and decodeDataUrls.cpp to the version in main and ran the new test. It fails because there are misleading errors reported, which is definitely a problem, but there's no crash. Maybe it only shows up on Linux or something? But it's not obvious to me why it should crash at all, which is why I asked. There could be a deeper problem.

In any case, your change looks good. Merging!

lilleyse commented 11 months ago

Ah my bad, not actually a crash, just an unexpected error.