Closed Bug 1132342 Opened 9 years ago Closed 9 years ago

race conditions in TrackBuffer::mInitializationPromise

Categories

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

37 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 + fixed
firefox38 + fixed
firefox39 --- fixed
firefox-esr31 --- unaffected

People

(Reporter: karlt, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main37+])

Attachments

(1 file, 1 obsolete file)

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.
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.
Summary: race conditions in TrackBuffer::RemoveDecoder() may lead to null deref → race conditions in TrackBuffer::mInitializationPromise
Implement solution discussed earlier. Detect if abort() was called while the trackbuffer was in the middle of calling readmetadata and initalizing decoders
Attachment #8565766 - Flags: review?(karlt)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
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?
Attachment #8565766 - Flags: review?(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 ...
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).
Attachment #8566918 - Flags: review?(karlt)
Attachment #8565766 - Attachment is obsolete: true
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.
Attachment #8566918 - Flags: review?(karlt) → review+
https://hg.mozilla.org/mozilla-central/rev/8c2b291548ad
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Group: media-core-security
Does this need to land on any other branches?
Flags: needinfo?(jyavenard)
Yes, including beta 37
Flags: needinfo?(jyavenard) → needinfo?(giles)
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
Flags: needinfo?(giles)
Attachment #8566918 - Flags: approval-mozilla-beta?
Attachment #8566918 - Flags: approval-mozilla-aurora?
Attachment #8566918 - Flags: approval-mozilla-beta?
Attachment #8566918 - Flags: approval-mozilla-beta+
Attachment #8566918 - Flags: approval-mozilla-aurora?
Attachment #8566918 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main37+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.