884459 - Heap-use-after-free in nsTArray_base<nsTArrayInfallibleAllocator>::Length()

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Tested on:

OS: Ubuntu 12.04

Firefox: ASAN debug-build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-dbg-asan/1371553271/


To reproduce you might need to hit refresh few times after opening the repro-file in browser.

ASAN-report:

==22795== ERROR: AddressSanitizer: heap-use-after-free on address 0x7fc6b99799c0 at pc 0x7fc6d324094a bp 0x7fc6a80861f0 sp 0x7fc6a80861e8
READ of size 4 at 0x7fc6b99799c0 thread T23
    #0 0x7fc6d3240949 in nsTArray_base<nsTArrayInfallibleAllocator>::Length() const /builds/slave/m-cen-l64-dbg-asan-00000000000/build/obj-firefox/toolkit/xre/../../dist/include/nsTArray.h:363
    #1 0x7fc6d33400b8 in nsTArray_base<nsTArrayInfallibleAllocator>::IsEmpty() const /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../../dist/include/nsTArray.h:368
    #2 0x7fc6d467450a in mozilla::MediaStreamGraphImpl::PrepareUpdatesToMainThreadState(bool) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/media/MediaStreamGraph.cpp:913
    #3 0x7fc6d4675d49 in mozilla::MediaStreamGraphImpl::RunThread() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/media/MediaStreamGraph.cpp:1124
    #4 0x7fc6d4684e88 in mozilla::(anonymous namespace)::MediaStreamGraphThreadRunnable::Run() /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/media/MediaStreamGraph.cpp:1223
    #5 0x7fc6d656bb3b in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/xpcom/threads/nsThread.cpp:626
.
.
.
freed by thread T0 here:
    #0 0x43b3c5 in __interceptor_realloc ??:0
    #1 0x7fc6de3cc4ba in moz_xrealloc /builds/slave/m-cen-l64-dbg-asan-00000000000/build/memory/mozalloc/mozalloc.cpp:86
    #2 0x7fc6d3325013 in nsTArray_base<nsTArrayInfallibleAllocator>::EnsureCapacity(unsigned int, unsigned int) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../dist/include/nsTArray-inl.h:167
    #3 0x7fc6d467c529 in nsAutoPtr<mozilla::ControlMessage>* nsTArray_Impl<nsAutoPtr<mozilla::ControlMessage>, nsTArrayInfallibleAllocator>::AppendElements<mozilla::ControlMessage*>(mozilla::ControlMessage* const*, unsigned int) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/../../dist/include/nsTArray.h:1044
    #4 0x7fc6d4677a9c in mozilla::MediaStreamGraphImpl::AppendMessage(mozilla::ControlMessage*) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/content/media/MediaStreamGraph.cpp:1470
    #5 0x7fc6d5d95ef0 in mozilla::dom::AudioParamBinding::setValueAtTime(JSContext*, JS::Handle<JSObject*>, mozilla::dom::AudioParam*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-dbg-asan-00000000000/build/obj-firefox/dom/bindings/AudioParamBinding.cpp:118
.
.
.

Comment 1

[Paul Adenot (:padenot)]

Attachment: patch

This fixes it.

Comment 2

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

Comment on attachment 764775 [details] [diff] [review]
patch

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

::: content/media/MediaStreamGraph.cpp
@@ +1468,5 @@
>    }
>  
> +  {
> +    MonitorAutoLock lock(mMonitor);
> +    mCurrentTaskMessageQueue.AppendElement(aMessage);

mCurrentTaskMessageQueue is supposed to be main-thread only, so why would this be necessary?

Comment 3

[Paul Adenot (:padenot)]

Well, |mCurrentTaskMessageQueue.IsEmpty()| is called on the MSG thread in MediaStreamGraphImpl::PrepareUpdatesToMainThreadState, which is called from MediaStreamGraphImpl::RunThread. Maybe this is our bug, then, I don't know MSG well enough yet to be sure.

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#919

Comment 4

[(no longer active)]

That's a bug that I've put in then <http://hg.mozilla.org/mozilla-central/rev/6e264b512357> :(

Not sure how best to fix it though...

Comment 5

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

Well, I reviewed it...

I think we just don't need to check !mCurrentTaskMessageQueue.IsEmpty(). It doesn't matter if there are pending messages from the main thread to the graph.

Comment 6

[(no longer active)]

If that works, then great!  I don't remember why I added that check, could be that was just a mistake.

Comment 7

[Paul Adenot (:padenot)]

Attachment: patch

This is green locally on content/media/test and content/media/webaudio/test.

Comment 8

[(no longer active)]

Comment on attachment 766043 [details] [diff] [review]
patch

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

OK, I wrote this code, so, r=me.  :-)  Thanks!

Comment 9

[(no longer active)]

Please land the test case as a crashtest alongside with the patch.  Also, since this only happens on m-c you don't need sec-approval.  It would be amazing if you can land this before the uplift (Monday.)

Comment 10

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97ff13b79e53

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/97ff13b79e53

Comment 12

[Paul Adenot (:padenot)]

Somehow this did not get RESOLVED FIXED during the m-i -> m-c merge.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

No longer tracking for FF23