1132342 - race conditions in TrackBuffer::mInitializationPromise

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Karl Tomlinson (:karlt)]

AFAICS TrackBuffer::mInitializationPromise.RejectIfExists() may be called on
the main thread via SourceBuffer::Abort() while also being called on an
initialization task queue thread from InitializeDecoder().

This could lead to a null deref if MediaPromiseHolder::mPromise has been
cleared, or use-after-free calling Reject() or Release() on a deleted mPromise.

Perhaps the RejectIfExists() can be proxied to the main thread.

Comment 1

[Jean-Yves Avenard [:jya]]

Only change required is TrackBuffer::AbortAppendData() to hold the monitor before calling reject.

if abort() got to run first, InitializeDecoder will immediately exit , after that it holds the lock.

The only other time reject can be called is in TrackBuffer::AppendData and this can never be called while TrackBuffer::InitializeDecoder hasn't completed.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Handle race should operation be aborted while reading metadata

Implement solution discussed earlier. Detect if abort() was called while the trackbuffer was in the middle of calling readmetadata and initalizing decoders

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8565766 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

> TrackBuffer::AbortAppendData()
> {
>   DiscardCurrentDecoder();
>   // The SourceBuffer would have disconnected its promise.
>   // However we must ensure that the MediaPromiseHolder handle all pending
>   // promises.
>+  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
>   mInitializationPromise.RejectIfExists(NS_ERROR_ABORT, __func__);
> }

The early exit on (mCurrentDecoder != aDecoder) below is enough to make this
unnecessary, right?  DiscardCurrentDecoder() unsets mCurrentDecoder in the
monitor (on the main thread), so testing mCurrentDecoder in the monitor on the
initialization queue is sufficient to avoid using mInitializationPromise at
the wrong time.

>+  if (mCurrentDecoder != aDecoder || mInitializationPromise.IsEmpty()) {

Is the mInitializationPromise.IsEmpty() test required?
Please remove if not.
I assume we use Reject() instead of RejectIfExists() below to clarify that it will exist and there is no race.
mInitializationPromise is Ensure()d outside of the monitor in AppendData(), so
I feel uncomfortable testing it on this thread.

We have just marked the resource not-ended.  If aDecoder != mCurrentDecoder,
then I guess it should be ended?  Should the AppendData() call above instead be after this (mCurrentDecoder != aDecoder) early return?

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #3)
> I assume we use Reject() instead of RejectIfExists() below to clarify that
> it will exist and there is no race.

I mean, I assume we *could* use ...

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Handle race should operation be aborted while reading metadata

applying comments. You're right, we could be using Reject() safely in place of RejectIfExists; as we are guaranteed that if we reach that point the promise holder holds a promise reference. Also, the only place where the promise would be created (in AppendData), will only ever be called until the initialization process have completed; or been cancelled (in which case InitializeDecoder would have iterrupted earlier).

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 8566918 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

> TrackBuffer::AbortAppendData()
> {
>   DiscardCurrentDecoder();
>   // The SourceBuffer would have disconnected its promise.
>   // However we must ensure that the MediaPromiseHolder handle all pending
>   // promises.
>+  ReentrantMonitorAutoEnter mon(mParentDecoder->GetReentrantMonitor());
>   mInitializationPromise.RejectIfExists(NS_ERROR_ABORT, __func__);
> }

Please remove this change now that mutually exclusive access to mInitializationPromise is protected by the state of mCurrentDecoder, which is always modified in the monitor (on main thread), and read off-main-thread in the monitor.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c2b291548ad

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Does this need to land on any other branches?

Comment 9

[Jean-Yves Avenard [:jya]]

Yes, including beta 37

Comment 10

[Ralph Giles (:rillian)]

Comment on attachment 8566918 [details] [diff] [review]
Handle race should operation be aborted while reading metadata

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Crashes with video playback, possible exploitation by malicious content.
[Describe test coverage new/current, TreeHerder]: Landed on m-c.
[Risks and why]: This is a small and straightforward fix, preventing null dereference and adding a lock. It's had two weeks to setting on central, so regression risk is low.
[String/UUID change made/needed]: None

Comment 11

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/803ed9fc9507

Comment 12

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19443ff644bd