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

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[Abhishek Arya]

Attachment: 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

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Tentatively blaming bug 928312 since it modified the way we hold timeouts.

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7c0c133cc2

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4631a830ba92 for GC crashes in https://tbpl.mozilla.org/php/getParsedLog.php?id=30510540&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=30509684&tree=Mozilla-Inbound

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82f5dee1c0ad

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 12

[(Away)]

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 [e-mail] extension installed.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This was trunk only so I think we can open it in a couple days.

Comment 15

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

Confirmed crash in FF28, 2013-01-12.
Verified fixed in ASan build of FF28, 2013-01-18.