Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

PlayerEventSink::update_current_images is unsound #112

Closed
nox opened this issue Nov 29, 2017 · 2 comments
Closed

PlayerEventSink::update_current_images is unsound #112

nox opened this issue Nov 29, 2017 · 2 comments

Comments

@nox
Copy link
Contributor

nox commented Nov 29, 2017

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
Copy link
Contributor

cpearce commented Dec 4, 2017

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).

bors-servo pushed a commit that referenced this issue Dec 19, 2017
Ensure video frames are stored in Rust allocated memory

To resolve the problems in issue #112 and to remove the potential use-after-free which can happen if GeckoMedia shuts down and deallocates video frames before the compositor is finished painting video frames, switch to having the video frames stored in Rust allocated Vec<u8>s which are ref counted. This means we won't deallocate the memory until the last Rust or C++ user of the data is destroyed.
@cpearce
Copy link
Contributor

cpearce commented Dec 19, 2017

This should be resolved by PR 131.

@cpearce cpearce closed this as completed Dec 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants