servo / gecko-media

Firefox's media playback stack in a stand alone Rust crate
Mozilla Public License 2.0
6 stars 9 forks source link

PlayerEventSink::update_current_images is unsound #112

Closed nox closed 6 years ago

nox commented 6 years ago

Its argument images contains Planar values which include a raw u8 pointer, which means we can pass bad values in there and probably make gecko-media dereference dangling pointers.

cpearce commented 6 years ago

Note that the PlanarYCbCrImage struct's Clone impl increments a C++ reference count on the pixel data, and its Drop impl decrements the reference count on the pixel data, which is supposed to ensure the pointer doesn't become dangling.

However, I have since realised that we override the ref count when we shutdown. So if we have a pointer to the planar data held while the Player object shuts down, we could end up deallocating the pixel data Rust code could end up derefing a dangling pointer. Since the compositor and the Player objects can live on different threads, this could potentially be an issue.

We don't see this in the example player, because the shutdown is initiated after the compositor's render loop has exited.

Fixing the lifetimes is complicated by the fact that C++ code will pass over the frame queue to the client multiple times; so we will call update_current_images() several times with the different PlanarYCbCrImage instances which referencing the same FrameIDs. So we have to be careful to allow the pixel data to be shared by multiple frame objects.

We may be able to do this by having the pixel data stored in Vec's allocated by Rust, and have some sort of indirection layer that allows them to be shared between frames and allows the C++ code to refer to them by some sort of handle (i.e. the opposite of what we currently do, which is have the frame's memory allocated by C++ code).

cpearce commented 6 years ago

This should be resolved by PR 131.