955911 - Heap-use-after-free in mozilla::MediaStream::ChangeExplicitBlockerCount

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Abhishek Arya]

Attachment: Testcase

==25265==ERROR: AddressSanitizer: heap-use-after-free on address 0x614000109790 at pc 0x7f93a8898469 bp 0x7f9389bd6310 sp 0x7f9389bd6308
READ of size 8 at 0x614000109790 thread T36 (MediaStreamGrph)
    #0 0x7f93a8898468 in mozilla::MediaStream::ChangeExplicitBlockerCount(int)::Message::Run() content/media/MediaStreamGraph.cpp:1656
    #1 0x7f93a886f10e in mozilla::MediaStreamGraphImpl::RunThread() content/media/MediaStreamGraph.cpp:1127
    #2 0x7f93a8899529 in mozilla::(anonymous namespace)::MediaStreamGraphInitThreadRunnable::Run() content/media/MediaStreamGraph.cpp:1350
    #3 0x7f93a4f7abcb in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:634
    #4 0x7f93a4e56ae1 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
    #5 0x7f93a57733e1 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:301
    #6 0x7f93a56e38d3 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:226
    #7 0x7f93a4f779a3 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:258
    #8 0x7f93b1dd7407 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:205
    #9 0x43bf60 in __asan::AsanThread::ThreadStart(unsigned long) _asan_rtl_
    #10 0x7f93b6367e99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
    #11 0x7f93b54763fc in ?? /build/buildd/eglibc-2.15/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112
0x614000109790 is located 336 bytes inside of 392-byte region [0x614000109640,0x6140001097c8)
freed by thread T36 (MediaStreamGrph) here:
    #0 0x433c25 in __interceptor_free _asan_rtl_
    #1 0x7f93a888f5af in nsTArray_Impl<nsRefPtr<mozilla::MediaStream>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned int, unsigned int) content/media/MediaStreamGraph.h:291
previously allocated by thread T0 here:
    #0 0x433d85 in malloc _asan_rtl_
    #1 0x7f93af3f1408 in moz_xmalloc memory/mozalloc/mozalloc.cpp:52
Thread T36 (MediaStreamGrph) created by T0 here:
    #0 0x414c9e in __interceptor_pthread_create _asan_rtl_
    #1 0x7f93b1dd3378 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:445
    #2 0x7f93b1dd2ec7 in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:528
Shadow bytes around the buggy address:
  0x0c28800192a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c28800192b0: fd fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa
  0x0c28800192c0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c28800192d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c28800192e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c28800192f0: fd fd[fd]fd fd fd fd fd fd fa fa fa fa fa fa fa
  0x0c2880019300: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c2880019310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2880019320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2880019330: 00 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa
  0x0c2880019340: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap right redzone:    fb
  Freed heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==25265==ABORTING

Comment 1

[Abhishek Arya]

Attachment: Audio file - bzip2 split part 1

Comment 2

[Abhishek Arya]

Attachment: Audio file - bzip2 split part 2

Comment 3

[Andrew McCreight [:mccr8]]

roc, do you have any suggestions for who might be able to look at this?  Thanks.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

padenot maybe

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

abhishek: What version did that ASAN report come from?

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

abhishek: What version did that ASAN report come from?
(lost the needinfo in a collision with roc)

Comment 7

[Abhishek Arya]

(In reply to Randell Jesup [:jesup] from comment #5)
> abhishek: What version did that ASAN report come from?

Clang revision r188888 and firefox revision 20140104081712

Comment 8

[Paul Adenot (:padenot)]

Attachment: r=

So, this happens, because we have a setup like this:

- We have a MediaElement, whose source is being captured to a MediaStream.
- We abort its loading (src is replaced or something).
- This schedules a ::Destroy on the MediaStream

#0  mozilla::MediaStream::Destroy (this=0x7fffca817f00) at /home/padenot/src/trees/mozilla-central/content/media/MediaStreamGraph.cpp:1764
#1  0x00007ffff193644f in mozilla::MediaDecoder::DecodedStreamData::~DecodedStreamData (this=0x7fffc8473260, __in_chrg=<optimized out>)
    at /home/padenot/src/trees/mozilla-central/content/media/MediaDecoder.cpp:210
#2  0x00007ffff1949ce0 in nsAutoPtr<mozilla::MediaDecoder::DecodedStreamData>::assign (this=0x7fffca904a18, newPtr=0x0) at ../../dist/include/nsAutoPtr.h:45
#3  0x00007ffff19457ed in nsAutoPtr<mozilla::MediaDecoder::DecodedStreamData>::operator= (this=0x7fffca904a18, rhs=0x0) at ../../dist/include/nsAutoPtr.h:115
#4  0x00007ffff1936879 in mozilla::MediaDecoder::DestroyDecodedStream (this=0x7fffca904980) at /home/padenot/src/trees/mozilla-central/content/media/MediaDecoder.cpp:281
#5  0x00007ffff193740a in mozilla::MediaDecoder::Shutdown (this=0x7fffca904980) at /home/padenot/src/trees/mozilla-central/content/media/MediaDecoder.cpp:462
#6  0x00007ffff1883980 in mozilla::dom::HTMLMediaElement::ShutdownDecoder (this=0x7fffcc735000)
    at /home/padenot/src/trees/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:584
#7  0x00007ffff1883a39 in mozilla::dom::HTMLMediaElement::AbortExistingLoads (this=0x7fffcc735000)
    at /home/padenot/src/trees/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:601
[...]

Then, at roughly the same time,
MediaDecoder::UpdateStreamBlockingForStateMachinePlaying gets called, and
MediaStream::ChangeExplicitBlockerCount gets queued up [1], because we are not on
the main thread, and so we can't directly send a message to the graph.

Then, in the same MessageBlock, we have
- A destroy message
- A change explicit blocker count message

hence the use after free.

[1]: http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoder.cpp#303

Comment 9

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/367027357fb5

Comment 10

[Carsten Book [:Tomcat]]

fixd on central https://hg.mozilla.org/mozilla-central/rev/367027357fb5

Comment 11

[Daniel Veditz [:dveditz]]

Paul: how far back does this bug go? A severe (or unrated) security patch needs sec-approval before landing: https://wiki.mozilla.org/Security/Bug_Approval_Process

Given the simple patch and code comments giving the triggering condition we should get this patch landed on other affected branches ASAP.

Comment 12

[Paul Adenot (:padenot)]

Daniel, I just tested current release, current beta, current aurora, and current nightly (that has the fix from this bug), using ASAN builds for each , and I can't repro. I would expect this bug to be a fallout from the recent big changes in the HTMLMediaElement/MediaStreamGraph ingration roc did a couple weeks ago.

Comment 13

[Daniel Veditz [:dveditz]]

Decoder: please run this testcase through your tinderbox-build bisector and see if we can pin down a regression range that would tell us the affected releases.

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Paul Adenot (:padenot) from comment #12)
> Daniel, I just tested current release, current beta, current aurora, and
> current nightly (that has the fix from this bug), using ASAN builds for each
> , and I can't repro. I would expect this bug to be a fallout from the recent
> big changes in the HTMLMediaElement/MediaStreamGraph ingration roc did a
> couple weeks ago.

Is there a tinderbox build that you /can/ repro on? Maybe you don't have it set up correctly. The only big changes I see from roc in MediaStreamGraph that you might be talking about is from bug 943461, but that didn't land until after this bug was filed.

Comment 15

[Daniel Veditz [:dveditz]]

The patch is in the middle of code that's quite old, would it be unsafe to just take it on old branches in any case? Or do we really need to nail down the regression range to do so safely?

Comment 16

[Paul Adenot (:padenot)]

Daniel, I didn't build myself, I grabbed the ASAN builds from tinderbox.

Comment 17

[bhavana bajaj [:bajaj]]

Waiting on confirmation if the previous releases are impacted to actually track this. Also just as an fyi, we go-to-build with one of last beta's today.

Comment 18

[Daniel Veditz [:dveditz]]

(In reply to Paul Adenot (:padenot) from comment #16)
> Daniel, I didn't build myself, I grabbed the ASAN builds from tinderbox.

I've got a windows box and can't use those ASAN builds. Comment 12 tells me which builds do NOT have the bug, but not which builds do. I assume you must have reproduced the crash at some point but you didn't say you did--it's possible you diagnosed this in the debugger without reproducing. If you didn't reproduce the crash at least once then I'm less confident in relying on your statement that Aurora didn't crash.

I'd be more confident if we could pin this down to the regressing bug, but roc's changes to this area seem to be either /after/ Abhishek reported the problem (bug 943461) or long enough ago that Aurora should have had the problem too. Probably there's some other bug you know about that I couldn't find, which is what I was asking for in comment 14

Comment 19

[Paul Adenot (:padenot)]

I'll do a mozregression run today to pinpoint exactly the changeset that introduced the crash. I could certainly repro in an ASAN build (that I built myself) 100% of the time before I fixed this, so that should not be too hard (the range being rather small).

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Without having figuring out a regression range, this should not have been checked in trunk per policy with this rating.

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 22

[bhavana bajaj [:bajaj]]

(In reply to Paul Adenot (:padenot) from comment #19)
> I'll do a mozregression run today to pinpoint exactly the changeset that
> introduced the crash. I could certainly repro in an ASAN build (that I built
> myself) 100% of the time before I fixed this, so that should not be too hard
> (the range being rather small).

NI on paul here to see if he has an update here. We have already gone to build with our final Release build for Fx27.0 but we could still uplift this on aurora if this impacts that branch.

Comment 23

[Paul Adenot (:padenot)]

Interestingly enough, I can't repro at all, using the gecko revision from comment 7, and I'm sure I could repro most of the time before.

Comment 24

[bhavana bajaj [:bajaj]]

Clearing tracking for now , please renominate if needed, in case we need to fix this on Fx28.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Does this affect Firefox 28? Did it affect 27?

Comment 26

[Abhishek Arya]

(In reply to Al Billings [:abillings] from comment #25)
> Does this affect Firefox 28? Did it affect 27?

I dont have the Firefox 28, 27 builds to verify this (i just build from trunk). Might want to check with Paul who fixed this.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Matt, can you try reproducing this on older builds so we can figure out if this affects 28 or if it affected 27?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

We mainly care about 28 here though but it needs to be a Linux ASAN build and you'll probably need to build it by hand.

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

Crashes 28, but not 27.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Thanks, Matt.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

Can we get a patch for Beta for this?

Comment 32

[Paul Adenot (:padenot)]

Al, the attached patch applies cleanly on 28.

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8359283 [details] [diff] [review]
r=

Approved for Beta (28). Get 'er in!

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/84f6530e4f9c

I talked to Paul on IRC about backports to the older branches. While the patch applies cleanly all the way down to b2g18, the changes that actually made this function buggy were introduced in 28, so the patch won't actually be doing anything useful on them.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/84f6530e4f9c

Comment 36

[u279076]

Matt, can you please evaluate whether this needs QA testing before we release Firefox 28?

Comment 37

[Matt Wobensmith [:mwobensmith][:matt:]]

Crashes FF29, 2013-12-19.
Verified fixed on FF28, FF29 2014-03-03.