Closed Bug 936327 Opened 6 years ago Closed 6 years ago

Heap-use-after-free in mozilla::dom::workers::WorkerPrivate::RunExpiredTimeouts

Categories

(Core :: DOM: Workers, defect)

x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 + verified
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.2 --- unaffected

People

(Reporter: inferno, Assigned: khuey)

References

Details

(4 keywords, Whiteboard: [asan])

Crash Data

Attachments

(2 files)

Attached file Testcase
Need to install Jesse's fuzzPriv extension - https://www.squarefree.com/extensions/domFuzzLite3.xpi

==8180==ERROR: AddressSanitizer: heap-use-after-free on address 0x615001b24689 at pc 0x7fa5ce2758f6 bp 0x7fa5b2cfa6b0 sp 0x7fa5b2cfa6a8
READ of size 1 at 0x615001b24689 thread T59 (DOM Worker)
    #0 0x7fa5ce2758f5 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:1394
    #1 0x7fa5ce274663 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:420
    #2 0x7fa5ce297495 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:482
    #3 0x7fa5ce298249 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:513
    #4 0x7fa5cdf9f597 in JS_CallFunctionValue(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::Value*) js/src/jsapi.cpp:4920
    #5 0x7fa5c9d781c6 in mozilla::dom::workers::WorkerPrivate::RunExpiredTimeouts(JSContext*) dom/workers/WorkerPrivate.cpp:4963
    #6 0x7fa5c9d6a340 in mozilla::dom::workers::WorkerRunnable::Run() dom/workers/WorkerPrivate.cpp:1827
    #7 0x7fa5c9d71e3f in mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext*) dom/workers/WorkerPrivate.cpp:3756
    #8 0x7fa5c9d5487e in (anonymous namespace)::WorkerThreadRunnable::Run() dom/workers/RuntimeService.cpp:956
    #9 0x7fa5cc5d78cc in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:622
    #10 0x7fa5cc5036f1 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:251
    #11 0x7fa5cc5d53f5 in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:250
    #12 0x7fa5d3dd8227 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:204
    #13 0x43bf60 in __asan::AsanThread::ThreadStart(unsigned long) _asan_rtl_
    #14 0x7fa5d857ee99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
    #15 0x7fa5d768d3fc in
0x615001b24689 is located 9 bytes inside of 56-byte region [0x615001b24680,0x615001b246b8)
freed by thread T59 (DOM Worker) here:
    #0 0x433c25 in __interceptor_free _asan_rtl_
    #1 0x7fa5ce18c450 in js_free objdir-ff-asan/js/src/../../dist/include/js/Utility.h:165
    #2 0x7fa5ce18c450 in js::SweepScriptData(JSRuntime*) js/src/jsscript.cpp:1537
    #3 0x7fa5ce05d111 in EndSweepPhase js/src/jsgc.cpp:3997
    #4 0x7fa5ce05d111 in IncrementalCollectSlice(JSRuntime*, long, JS::gcreason::Reason, js::JSGCInvocationKind) js/src/jsgc.cpp:4442
    #5 0x7fa5ce05456c in GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:4568
    #6 0x7fa5ce04ae23 in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:4712
previously allocated by thread T59 (DOM Worker) here:
    #0 0x433d85 in malloc _asan_rtl_
    #1 0x7fa5ce186ccf in js_malloc objdir-ff-asan/js/src/../../dist/include/js/Utility.h:142
    #2 0x7fa5ce186ccf in malloc_ js/src/vm/Runtime.h:604
    #3 0x7fa5ce186ccf in js::SharedScriptData::new_(js::ExclusiveContext*, unsigned int, unsigned int, unsigned int) js/src/jsscript.cpp:1439
Thread T59 (DOM Worker) created by T0 here:
    #0 0x414c9e in __interceptor_pthread_create _asan_rtl_
    #1 0x7fa5d3dd42c8 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:444
    #2 0x7fa5d3dd3e17 in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:527
Shadow bytes around the buggy address:
  0x0c2a8035c880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
  0x0c2a8035c890: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c8a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c8b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c8c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2a8035c8d0: fd[fd]fd fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c2a8035c8e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c8f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c900: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c910: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2a8035c920: 00 00 00 00 00 00 00 00 00 00 fa fa fa fa fa fa
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
==8180==ABORTING
Tentatively blaming bug 928312 since it modified the way we hold timeouts.
Can you look at this, Kyle?

Note also that a similar stack is showing up in massively increased volume on crash stats, in bug 937191: "This occurs mostly on Windows, with a several crashes on Mac.  Most interesting is that nearly all of these crashes happen a matter of seconds after the browser has been up for 1 hour."
Flags: needinfo?(khuey)
Blocks: 928312
Assignee: nobody → khuey
Flags: needinfo?(khuey)
Attached patch PatchSplinter Review
I didn't manage to produce a crash with Abhishek's test case, and I didn't want to do an ASAN build, but from reading the test case I was able to figure out what's going on.  I forgot to HoldJSObjects on the global, so we never actually trace it.  The test case sets a timer (thus adding a js function that we need to trace) and then fires memory pressure (forcing a GC).  Our timer function gets GCd out from underneath us and the when the timer fires we have a bad time.
Attachment #830668 - Flags: review?(bent.mozilla)
Attachment #830668 - Attachment description: diff.diff → Patch
Whiteboard: [asan]
Crash Signature: [@ js::CompartmentChecker::fail(JSCompartment*, JSCompartment*)]
Keywords: topcrash-win
Comment on attachment 830668 [details] [diff] [review]
Patch

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

::: dom/bindings/BindingUtils.h
@@ +2323,5 @@
>      NS_WARNING("Failed to set proto");
>      return nullptr;
>    }
>  
> +  mozilla::HoldJSObjects(aObject);

It's kinda unfortunate that the Hold and Drop are spread out over seemingly unrelated files... I guess we don't have too many global object types so maybe it's fine, but is there no way to move the Drop here somehow?

::: dom/workers/test/test_timeoutTracing.html
@@ +22,5 @@
> +    // begin
> +    worker.onmessage = null;
> +
> +    // 10 seconds should be enough to crash.
> +    window.setTimeout(function(event) { ok(true, "Didn't crash!"); SimpleTest.finish(); }, 10000);

This seems kinda long, and our tests are already slow enough (hellooooo, xhrtimeouts...). Can you shorten this up? I bet all you need to do is a 1s timeout after the memory pressure notification, right?
Attachment #830668 - Flags: review?(bent.mozilla) → review+
(In reply to ben turner [:bent] (needinfo? encouraged) from comment #5)
> Comment on attachment 830668 [details] [diff] [review]
> Patch
> 
> Review of attachment 830668 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/BindingUtils.h
> @@ +2323,5 @@
> >      NS_WARNING("Failed to set proto");
> >      return nullptr;
> >    }
> >  
> > +  mozilla::HoldJSObjects(aObject);
> 
> It's kinda unfortunate that the Hold and Drop are spread out over seemingly
> unrelated files... I guess we don't have too many global object types so
> maybe it's fine, but is there no way to move the Drop here somehow?

Not easily.  I thought about creating a base class for globals but the problem is that if we call HoldJSObjects in the ctor it will not be fully constructed.

> ::: dom/workers/test/test_timeoutTracing.html
> @@ +22,5 @@
> > +    // begin
> > +    worker.onmessage = null;
> > +
> > +    // 10 seconds should be enough to crash.
> > +    window.setTimeout(function(event) { ok(true, "Didn't crash!"); SimpleTest.finish(); }, 10000);
> 
> This seems kinda long, and our tests are already slow enough (hellooooo,
> xhrtimeouts...). Can you shorten this up? I bet all you need to do is a 1s
> timeout after the memory pressure notification, right?

I dropped it to 1 (and the interval timer to 20 ms).
The crash this was backed out for is the trace hook not being able to handle null.  This was intermittent because we have to end up GCing during a test that calls setTimeout/setInterval with a string.  I added this to the test for this bug and added a null check.
https://hg.mozilla.org/mozilla-central/rev/82f5dee1c0ad

If bug 928312 landed for Gecko 28, why is status-b2g-v1.2 (Gecko 26) set to affected?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
I guess I'm kind of late to the party on this one, but for what it's worth, every crash report I've seen for this signature has a yasearch@yandex.ru extension installed.
This was trunk only so I think we can open it in a couple days.
Flags: sec-bounty? → sec-bounty+
Group: core-security
Confirmed crash in FF28, 2013-01-12.
Verified fixed in ASan build of FF28, 2013-01-18.
Status: RESOLVED → VERIFIED
No longer depends on: 975052
Depends on: 975052
You need to log in before you can comment on or make changes to this bug.