1114383 - Poor synchronization of mp4 demuxer leads to race-y UAF

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

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

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Acquire the index monitor in MP4Reader::PopSample. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Assert that access to mMoofs goes through a monitor. v1

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

IIUC, this only affects branches with asynchronous MP4, which is nightly + aurora. Do I have this right?

Comment 5

[Bobby Holley (:bholley)]

(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?

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

This only affects nightly because previously PopSample used MPEG4Extractor rather than Index.

Comment 7

[Bobby Holley (:bholley)]

(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.

Comment 8

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Unlock the demuxer monitor before calling out via MP4Stream. v1

Comment 11

[Bobby Holley (:bholley)]

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

Comment 12

[Bobby Holley (:bholley)]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/810bbcb9bf7e
https://hg.mozilla.org/mozilla-central/rev/1c7ac8ef924d
https://hg.mozilla.org/mozilla-central/rev/73a8a6927bde

Comment 14

[Ralph Giles (:rillian)]

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.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Ralph, Anthony in comment #6 marked 36 as unaffected and you request an uplift to 36. Who is correct?
Thanks

Comment 16

[Bobby Holley (:bholley)]

(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

[Ralph Giles (:rillian)]

We need this change in Aurora too. Sorry for the confusion.

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(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.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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