1092025 - Potential UAF in MediaSourceReader::ReadMetadata() mAudioTrack->Decoders()[0]->GetReader()

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

MediaSourceReader::ReadMetadata() is called on the decode task queue.

It calls mAudioTrack->Decoders()[0]->GetReader() without holding the decoder
monitor.

Decoders() returns TrackBuffer::mInitializedDecoders, the length of which may
be modified concurrently in TrackBuffer::RegisterDecoder() on the
initialization task queue TrackBuffer::mTaskQueue (with the decoder monitor
held).

Disabled in RELEASE_BUILD

Comment 1

[Karl Tomlinson (:karlt)]

Possible solutions might include one of:

* Setting mAudioReader in MediaSourceReader::OnTrackBufferConfigured().

* Holding the lock.

* Using the decode task queue to replace TrackBuffer::mTaskQueue.

Comment 2

[Bobby Holley (:bholley)]

See also bug 1113282, which improves this code but doesn't deal with the fundamental problem. I'll see if I can have a look at this sometime next week.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #2)
> See also bug 1113282, which improves this code but doesn't deal with the
> fundamental problem. I'll see if I can have a look at this sometime next
> week.

Oh wait I see - this is about MediaSourceReader's ReadMetaData implementation, not the call made in TrackBuffer. The TrackBuffer stuff is sane-ish AFAICT.

It also looks like MediaSourceReader::ReadMetaData doesn't actually do anything expensive or blocking, so it should be fine to just hold the lock. I'll attach a patch.

Also, can someone explain to me why we have this separate task queue for initialization? Is this something we can get rid of now after the refactoring in bug 1104964, or is it necessary for the blocking ReadMetaData call?

Comment 4

[Bobby Holley (:bholley)]

Attachment: Hold the lock for the entire duration of MediaSourceReader::ReadMetaData. v1

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=797d32cde2f4

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8539430 [details] [diff] [review]
Hold the lock for the entire duration of MediaSourceReader::ReadMetaData. v1

Looks like Karl is back, so moving the review to him.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3)
> Also, can someone explain to me why we have this separate task queue for
> initialization? Is this something we can get rid of now after the
> refactoring in bug 1104964, or is it necessary for the blocking ReadMetaData
> call?

I assumed it was to allow media from one SourceBuffer (or even the same SourceBuffer) to continue to play while ReadMetadata blocked waiting for JS to provide the rest of the initialization segment and the first frame.  It no longer waits for the first frame, and WebMReader::ReadMetadata should no longer block waiting for the end of the init segment, but there may still be an issue with MP4Reader and partial init segments.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #5)
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=797d32cde2f4

For some reason this try run didn't trigger any jobs:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=70bd3c18ebc0

Comment 9

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/811adfa8d698

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/811adfa8d698

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 8539430 [details] [diff] [review]
Hold the lock for the entire duration of MediaSourceReader::ReadMetaData. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing; users more like to see Flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: This does affect normal video playback, but addresses a race, so any regressions it might cause are better than the alternative.
[String/UUID change made/needed]: none.

Comment 12

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dbc18960a3a1