More mp4 demuxer races (MP4Reader::Seek)

RESOLVED FIXED in Firefox 36

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Bobby Holley (On Leave Until June 11th), Assigned: Bobby Holley (On Leave Until June 11th))

Tracking

({sec-high})

unspecified
mozilla37
x86
Mac OS X
sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 disabled, firefox36 fixed, firefox37 fixed, firefox-esr31 unaffected)

Details

Attachments

(4 attachments)

Sigh.

0   libsystem_kernel.dylib        	0x00007fff87048282 __pthread_kill + 10
1   libsystem_c.dylib             	0x00007fff8fa5fb73 abort + 129
2   libnss3.dylib                 	0x000000010133c7ad PR_Assert + 109
3   libnss3.dylib                 	0x000000010136cc18 PR_AssertCurrentThreadOwnsLock + 104 (ptsynch.c:227)
4   XUL                           	0x00000001015c5549 mozilla::OffTheBooksMutex::AssertCurrentThreadOwns() const + 25
5   XUL                           	0x00000001015c5525 mozilla::Monitor::AssertCurrentThreadOwns() const + 21
6   XUL                           	0x00000001015b3400 mp4_demuxer::MoofParser::Moofs() + 32
7   XUL                           	0x0000000101596b8f mp4_demuxer::SampleIterator::Get() + 95
8   XUL                           	0x0000000101596f91 mp4_demuxer::SampleIterator::Seek(long long) + 65
9   XUL                           	0x000000010159afcd mp4_demuxer::MP4Demuxer::SeekAudio(long long) + 109
10  XUL                           	0x0000000104298ca2 mozilla::MP4Reader::Seek(long long, long long, long long, long long) + 626
11  XUL                           	0x00000001041ab2a0 mozilla::MediaSourceReader::AttemptSeek() + 544
12  XUL                           	0x00000001041ab73c mozilla::MediaSourceReader::Seek(long long, long long, long long, long long) + 492
13  XUL                           	0x00000001040c1caa mozilla::MediaDecoderStateMachine::DecodeSeek() + 1114
14  XUL                           	0x000000010410f17a nsRunnableMethodImpl<void (mozilla::MediaDecoderStateMachine::*)(), void, true>::Run() + 154
15  XUL                           	0x00000001040e7389 mozilla::MediaTaskQueue::Runner::Run() + 633
16  XUL                           	0x0000000101702a27 nsThreadPool::Run() + 967
17  XUL                           	0x0000000101702b1c non-virtual thunk to nsThreadPool::Run() + 28
18  XUL                           	0x00000001016ff488 nsThread::ProcessNextEvent(bool, bool*) + 2088
19  XUL                           	0x0000000101758697 NS_ProcessNextEvent(nsIThread*, bool) + 151
20  XUL                           	0x0000000101d82d91 mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) + 961
21  XUL                           	0x0000000101d1a105 MessageLoop::RunInternal() + 117
22  XUL                           	0x0000000101d1a015 MessageLoop::RunHandler() + 21
23  XUL                           	0x0000000101d19fbd MessageLoop::Run() + 45
24  XUL                           	0x00000001016fd916 nsThread::ThreadFunc(void*) + 358
25  libnss3.dylib                 	0x000000010137203f _pt_root + 463 (ptthread.c:215)
26  libsystem_pthread.dylib       	0x00007fff8a7c82fc _pthread_body + 131
27  libsystem_pthread.dylib       	0x00007fff8a7c8279 _pthread_start + 176
28  libsystem_pthread.dylib       	0x00007fff8a7c64b1 thread_start + 13
Flags: needinfo?(bobbyholley)
Created attachment 8541811 [details] [diff] [review]
Part 1 - Rename mIndexMonitor to mDemuxerMonitor. v1
Attachment #8541811 - Flags: review?(ajones)
Created attachment 8541812 [details] [diff] [review]
Part 2 - Hold the demuxer monitor in MP4Reader::Seek. v1
Attachment #8541812 - Flags: review?(ajones)
Created attachment 8541813 [details] [diff] [review]
Part 3 - Hold the demuxer lock while accessing metadata. v1
Attachment #8541813 - Flags: review?(ajones)
Created attachment 8541814 [details] [diff] [review]
Part 4 - Assert that we hold the monitor at all the MP4Demuxer API entry points. v1
Attachment #8541814 - Flags: review?(ajones)
Comment on attachment 8541813 [details] [diff] [review]
Part 3 - Hold the demuxer lock while accessing metadata. v1

Review of attachment 8541813 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/fmp4/MP4Reader.cpp
@@ +413,5 @@
> +  Microseconds duration;
> +  {
> +    MonitorAutoLock lock(mDemuxerMonitor);
> +    duration = mDemuxer->Duration();
> +  }

you already own the lock.

@@ +421,5 @@
>    }
>  
>    *aInfo = mInfo;
>    *aTags = nullptr;
>  

also need to remove the lock   MonitorAutoLock mon(mIndexMonitor); before Update() line 424
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> you already own the lock.

No, it goes out of scope above.

> also need to remove the lock   MonitorAutoLock mon(mIndexMonitor); before
> Update() line 424

No, for the same reason.
Duh... Will crawl back into my hole :)
Blocks: 1114849
Blocks: 1114847
Blocks: 1114844
Comment on attachment 8541811 [details] [diff] [review]
Part 1 - Rename mIndexMonitor to mDemuxerMonitor. v1

Review of attachment 8541811 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review since k17e is on vacation and this is causing problems with debug builds.
Attachment #8541811 - Flags: review?(ajones) → review+
Comment on attachment 8541814 [details] [diff] [review]
Part 4 - Assert that we hold the monitor at all the MP4Demuxer API entry points. v1

Review of attachment 8541814 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, stealing review.
Attachment #8541814 - Flags: review?(ajones) → review+
Comment on attachment 8541813 [details] [diff] [review]
Part 3 - Hold the demuxer lock while accessing metadata. v1

Review of attachment 8541813 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review. r=me

::: dom/media/fmp4/MP4Reader.cpp
@@ +413,5 @@
> +  Microseconds duration;
> +  {
> +    MonitorAutoLock lock(mDemuxerMonitor);
> +    duration = mDemuxer->Duration();
> +  }

No, this is ok. The previous lock goes out of scope at the end of 'if (!mDemuxerInitialized)' at the start of the function.

@@ +421,5 @@
>    }
>  
>    *aInfo = mInfo;
>    *aTags = nullptr;
>  

Likewise.
Attachment #8541813 - Flags: review?(ajones) → review+
Comment on attachment 8541812 [details] [diff] [review]
Part 2 - Hold the demuxer monitor in MP4Reader::Seek. v1

Review of attachment 8541812 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing review. r=me with comments addressed.

Could you please put some motivation in the body of the commit message for future reference. Why is it important to take the monitor here but not on other methods?

::: dom/media/fmp4/MP4Reader.h
@@ +101,5 @@
>  
>    // Blocks until the demuxer produces an sample of specified type.
>    // Returns nullptr on error on EOS. Caller must delete sample.
>    mp4_demuxer::MP4Sample* PopSample(mp4_demuxer::TrackType aTrack);
> +  mp4_demuxer::MP4Sample* PopSampleLocked(mp4_demuxer::TrackType aTrack);

It's unfortunate you need two versions here.

Is there no way to prevent deadlock if one calls PopSample() instead of PopSampleLocked()?
Attachment #8541812 - Flags: review?(ajones) → review+
Per IRC discussion, the answer to my deadlock question was 'no'.
Blocks: 1115190
Given all the crashes that are showing up from this, it doesn't seem out of the question that something bad could happen with this, so I'm going to mark it as sec-high.  Feel free to adjust as needed.
Keywords: sec-high
https://hg.mozilla.org/mozilla-central/rev/c603370ae39d
https://hg.mozilla.org/mozilla-central/rev/5640e21c2544
https://hg.mozilla.org/mozilla-central/rev/ed1950e4c697
https://hg.mozilla.org/mozilla-central/rev/30c89bc08ce5
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox37: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Was this trunk only?
Flags: needinfo?(bobbyholley)
(In reply to Al Billings [:abillings] from comment #18)
> Was this trunk only?

Given that we're aggressively uplifting all media patches to 36 for MSE, this also affects aurora.

Speaking of which, Ralph, can you uplift this?
Flags: needinfo?(bobbyholley) → needinfo?(giles)
I was curious as to whether it affected ESR31 or not.
status-firefox36: --- → affected
(In reply to Al Billings [:abillings] from comment #20)
> I was curious as to whether it affected ESR31 or not.

No. This code is pref'd off prior to Firefox 36.
Flags: needinfo?(giles)
status-firefox35: --- → disabled
status-firefox-esr31: --- → unaffected
(In reply to Ralph Giles (:rillian) from comment #21)
> (In reply to Al Billings [:abillings] from comment #20)
> > I was curious as to whether it affected ESR31 or not.
> 
> No. This code is pref'd off prior to Firefox 36.

The NI was from comment 19.
Flags: needinfo?(giles)
Comment on attachment 8541811 [details] [diff] [review]
Part 1 - Rename mIndexMonitor to mDemuxerMonitor. v1

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Less consistent testing, sites more likely to serve flash video.
[Describe test coverage new/current, TBPL]: Landed on m-c.
[Risks and why]: Low. These changes are MSE-specific.
[String/UUID change made/needed]: None.

This request applies to all patches on this bug.
Flags: needinfo?(giles)
Attachment #8541811 - Flags: approval-mozilla-aurora?
Attachment #8541811 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
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.