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)

x86
macOS
defect
Not set
normal

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)

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
Attachment #8539850 - Flags: review?(ajones) → review+
IIUC, this only affects branches with asynchronous MP4, which is nightly + aurora. Do I have this right?
Flags: needinfo?(ajones)
Attachment #8539851 - Flags: review?(ajones) → review+
Flags: needinfo?(ajones)
(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.
Flags: needinfo?(ajones)
(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)
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.
Attachment #8539912 - Flags: review?(ajones) → review+
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?
Ralph, Anthony in comment #6 marked 36 as unaffected and you request an uplift to 36. Who is correct?
Thanks
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

IIUC we're uplifting all the MSE stuff to 36.
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.
Attachment #8539850 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 1119456
Group: core-security
You need to log in before you can comment on or make changes to this bug.