google / filament

Filament is a real-time physically based rendering engine for Android, iOS, Windows, Linux, macOS, and WebGL2
https://google.github.io/filament/
Apache License 2.0
17.35k stars 1.83k forks source link

Fix use-after-free of a std::mutex in PresentCallable. #7934

Closed ullerrm closed 1 week ago

ullerrm commented 1 week ago

If a user is using setFrameScheduledCallback(), managing the provided PresentCallable during engine shutdown is tricky -- we'll likely get a final frame scheduled when we flush the engine's work queue, but the PresentCallable will schedule the final CAMetalDrawable to be released on main thread afterwards, even if we call present(false) to skip it. If the swap chain is destroyed before that main queue block gets executed, the mutex presenting that drawable will no longer exist, causing a crash.

To make things easier, store the std::mutex in a shared_ptr, so that a PresentCallable can safely outlive the FSwapChain instance that created it and clean itself up afterwards.

Alternatives considered, all of which seem unfortunate:

pixelflinger commented 1 week ago

@bejado can we find a better solution for this? I really don't like having heap allocations for this and I'm not a fan of the mutex in the first place.

bejado commented 1 week ago

Unfortunately CAMetalLayer is not thread-safe with respect to drawable allocation:

https://github.com/google/filament/blob/cc8a3ed96b311c265395a0eecb262637e8c5829e/filament/backend/src/metal/MetalHandles.mm#L178-L180

So, the only alternative to using a mutex I imagine would be to always acquire/release drawables on the main thread. But, that has performance implications and potential deadlock hazards I need to think through. For now I think this is the best fix; using shared_ptr is a pattern we use elsewhere in the backend to ensure certain objects outlast the destruction of a handle.