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

RESOLVED FIXED in Firefox 24

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: attekett, Assigned: padenot)

Tracking

(4 keywords)

Trunk
mozilla24
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox22 disabled, firefox23- disabled, firefox24+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main24-])

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
Posted file 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
.
.
.
Blocks: webaudio
Severity: normal → critical
OS: Linux → All
Version: unspecified → Trunk

Updated

6 years ago
Assignee: nobody → paul
Posted patch patch (obsolete) — Splinter Review
This fixes it.
Attachment #764775 - Flags: review?(roc)
Flags: sec-bounty?
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?
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

6 years ago
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...
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

6 years ago
If that works, then great!  I don't remember why I added that check, could be that was just a mistake.
Posted patch patchSplinter Review
This is green locally on content/media/test and content/media/webaudio/test.
Attachment #766043 - Flags: review?(roc)
Attachment #764775 - Attachment is obsolete: true
Attachment #764775 - Flags: review?(roc)

Comment 8

6 years ago
Comment on attachment 766043 [details] [diff] [review]
patch

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

OK, I wrote this code, so, r=me.  :-)  Thanks!
Attachment #766043 - Flags: review?(roc) → review+

Comment 9

6 years ago
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.)
Somehow this did not get RESOLVED FIXED during the m-i -> m-c merge.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Blocks: 885956
Flags: sec-bounty? → sec-bounty+
Duplicate of this bug: 885956
Whiteboard: [adv-main24-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.