Closed
Bug 1114383
Opened 9 years ago
Closed 9 years ago
Poor synchronization of mp4 demuxer leads to race-y UAF
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
Tracking | Status | |
---|---|---|
firefox34 | --- | unaffected |
firefox35 | --- | unaffected |
firefox36 | --- | fixed |
firefox37 | --- | fixed |
firefox-esr31 | --- | unaffected |
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: csectype-uaf)
Attachments
(3 files)
790 bytes,
patch
|
ajones
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
16.16 KB,
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
5.68 KB,
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
My buffering patches in bug 1109437 bounced because they were triggering an infrequent crash in the MP4 demuxer on OSX 10.8 debug. Some diagnostic patches led me to conclude that our nsTArray was getting realloced out from under us in mp4_demuxer::Index::GetEndCompositionIfBuffered, which led me to suspect that our synchronization for demuxer access wasn't holding water. I wrote a patch to assert that we're holding the index monitor whenever we access MoofParser::mMoofs. Sure enough, I asserted with the following stack on a decode thread: 16 libnss3.dylib 0x000000010136cc18 PR_AssertCurrentThreadOwnsLock + 104 17 XUL 0x00000001015c5b89 mozilla::OffTheBooksMutex::AssertCurrentThreadOwns() const + 25 (Mutex.h:96) 18 XUL 0x00000001015c5b65 mozilla::Monitor::AssertCurrentThreadOwns() const + 21 (Monitor.h:49) 19 XUL 0x00000001015b2490 mp4_demuxer::MoofParser::Moofs() + 32 (MoofParser.h:216) 20 XUL 0x000000010159627f mp4_demuxer::SampleIterator::Get() + 95 (Index.cpp:143) 21 XUL 0x0000000101595bd6 mp4_demuxer::SampleIterator::GetNext() + 54 (Index.cpp:88) 22 XUL 0x000000010159aa9a mp4_demuxer::MP4Demuxer::DemuxAudioSample() + 106 (mp4_demuxer.cpp:188) 23 XUL 0x000000010428295b mozilla::MP4Reader::PopSample(mp4_demuxer::TrackType) + 91 (MP4Reader.cpp:637) 24 XUL 0x00000001042824dc mozilla::MP4Reader::Update(mp4_demuxer::TrackType) + 700 (MP4Reader.cpp:586) 25 XUL 0x00000001042846e1 nsRunnableMethodImpl<void (mozilla::MP4Reader::*)(mp4_demuxer::TrackType), mp4_demuxer::TrackType, true>::Run() + 161 (nsThreadUtils.h:363) 26 XUL 0x00000001040d1d89 mozilla::MediaTaskQueue::Runner::Run() + 633 (MediaTaskQueue.cpp:237) 27 XUL 0x0000000101700d67 nsThreadPool::Run() + 967 (nsThreadPool.cpp:220) 28 XUL 0x0000000101700e5c non-virtual thunk to nsThreadPool::Run() + 28 (nsThreadPool.cpp:234) 29 XUL 0x00000001016fd7c8 nsThread::ProcessNextEvent(bool, bool*) + 2088 (nsThread.cpp:836) 30 XUL 0x0000000101756a27 NS_ProcessNextEvent(nsIThread*, bool) + 151 (nsThreadUtils.cpp:265) 31 XUL 0x0000000101d7fe33 mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) + 691 (MessagePump.cpp:339) 32 XUL 0x0000000101d175a5 MessageLoop::RunInternal() + 117 (message_loop.cc:234) 33 XUL 0x0000000101d174b5 MessageLoop::RunHandler() + 21 (message_loop.cc:227) 34 XUL 0x0000000101d1745d MessageLoop::Run() + 45 (message_loop.cc:200) 35 XUL 0x00000001016fbc56 nsThread::ThreadFunc(void*) + 358 (nsThread.cpp:355) 36 libnss3.dylib 0x000000010137203f _pt_root + 463 37 libsystem_pthread.dylib 0x00007fff97d6b2fc _pthread_body + 131 38 libsystem_pthread.dylib 0x00007fff97d6b279 _pthread_start + 176 39 libsystem_pthread.dylib 0x00007fff97d694b1 thread_start + 13
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8539850 -
Flags: review?(ajones)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8539851 -
Flags: review?(ajones)
Assignee | ||
Comment 3•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6f40ecb2f01
Updated•9 years ago
|
Attachment #8539850 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 4•9 years ago
|
||
IIUC, this only affects branches with asynchronous MP4, which is nightly + aurora. Do I have this right?
Flags: needinfo?(ajones)
Updated•9 years ago
|
Attachment #8539851 -
Flags: review?(ajones) → review+
Updated•9 years ago
|
Flags: needinfo?(ajones)
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #4) > IIUC, this only affects branches with asynchronous MP4, which is nightly + > aurora. Do I have this right?
Flags: needinfo?(ajones)
This only affects nightly because previously PopSample used MPEG4Extractor rather than Index.
status-firefox34:
--- → unaffected
status-firefox35:
--- → unaffected
status-firefox36:
--- → unaffected
status-firefox37:
--- → affected
Flags: needinfo?(ajones)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #3) > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a6f40ecb2f01 This was orange because it turns out that the demuxer calls back out into media code while the demuxer monitor is held, and then code tries to get the decoder monitor. Trying a very simple hack of releasing the monitor during the calls that I see: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e93019c2bf33 Anthony - is there any reason why we use a special index monitor here, and not the decoder monitor? 2 monitors makes it really easy to deadlock.
Flags: needinfo?(ajones)
I'm happy to reduce the number of monitors. The reason I didn't was because we have separate video and audio monitors already so I would've needed to lock both to calculate the buffered ranges.
Flags: needinfo?(ajones)
Assignee | ||
Comment 9•9 years ago
|
||
Summarizing from IRC discussion: I tried to switch things over to the decoder monitor, but that didn't work because the read also bogarts the lock while waiting on gMediaCache->GetReentrantMonitor(). So I think the best thing to do is just to leave a separate monitor, and manually unlock it before calling out to read.
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8539912 -
Flags: review?(ajones)
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e6011e5730f9
Updated•9 years ago
|
Attachment #8539912 -
Flags: review?(ajones) → review+
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Bobby Holley (Busy with media, don't ask for DOM/JS/XPConnect things) from comment #11) > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e6011e5730f9 Green. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/810bbcb9bf7e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c7ac8ef924d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/73a8a6927bde
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/810bbcb9bf7e https://hg.mozilla.org/mozilla-central/rev/1c7ac8ef924d https://hg.mozilla.org/mozilla-central/rev/73a8a6927bde
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 14•9 years ago
|
||
Comment on attachment 8539850 [details] [diff] [review] Part 1 - Acquire the index monitor in MP4Reader::PopSample. v1 Approval Request Comment [Feature/regressing bug #]: MSE [User impact if declined]: Less consistent testing, users more likely to see Flash video. [Describe test coverage new/current, TBPL]: Landed on m-c. [Risks and why]: Regression risk is moderate as this affects the normal video playback path. However, it adds better locking to this should help us find bugs in that code. [String/UUID change made/needed]: None. This requests serves for all patches in this bug.
Attachment #8539850 -
Flags: approval-mozilla-aurora?
Comment 15•9 years ago
|
||
Ralph, Anthony in comment #6 marked 36 as unaffected and you request an uplift to 36. Who is correct? Thanks
Flags: needinfo?(giles)
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #15) > Ralph, Anthony in comment #6 marked 36 as unaffected and you request an > uplift to 36. Who is correct? > Thanks IIUC we're uplifting all the MSE stuff to 36.
Comment 17•9 years ago
|
||
We need this change in Aurora too. Sorry for the confusion.
Flags: needinfo?(giles)
(In reply to Sylvestre Ledru [:sylvestre] from comment #15) > Ralph, Anthony in comment #6 marked 36 as unaffected and you request an > uplift to 36. Who is correct? > Thanks We're uplifting everything but at the time I marked this bug, the bug that caused the issue hadn't been uplifted.
Updated•9 years ago
|
Attachment #8539850 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9f38ae4c01b6 https://hg.mozilla.org/releases/mozilla-aurora/rev/a2b38504a3e9 https://hg.mozilla.org/releases/mozilla-aurora/rev/9ccde85dfd82
Updated•9 years ago
|
status-firefox-esr31:
--- → unaffected
Updated•9 years ago
|
Group: core-security
Updated•7 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•