Closed Bug 1146213 Opened 9 years ago Closed 9 years ago

Crash [@ js::NativeObject::setSlot] with off-thread compilation

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla42
Tracking Status
firefox39 --- wontfix
firefox40 + verified
firefox41 + verified
firefox42 + verified
firefox43 --- verified
firefox-esr31 --- unaffected
firefox-esr38 40+ fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: decoder, Assigned: jonco)

Details

(4 keywords, Whiteboard: [jsbugmon:update][post-critsmash-triage][adv-main40+][adv-esr38.2+])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision b8e628af0b5c (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2):

setGCCallback({
  action: "majorGC",
});
schedulegc(this)
gcslice(3)
var lfGlobal = newGlobal();
lfGlobal.offThreadCompileScript("");
lfGlobal.runOffThreadScript();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::NativeObject::setSlot (this=0x4b4b4b4b4b4b4b4b, slot=1, value=...) at js/src/vm/NativeObject.h:785
#0  js::NativeObject::setSlot (this=0x4b4b4b4b4b4b4b4b, slot=1, value=...) at js/src/vm/NativeObject.h:785
#1  0x0000000000894375 in setReservedSlot (v=..., index=1, this=<optimized out>) at js/src/vm/NativeObject.h:851
#2  js::ScriptSourceObject::initFromOptions (cx=cx@entry=0x17376c0, source=..., source@entry=..., options=...) at js/src/jsscript.cpp:1388
#3  0x0000000000529599 in js::ParseTask::finish (this=this@entry=0x1810600, cx=cx@entry=0x17376c0) at js/src/vm/HelperThreads.cpp:240
#4  0x0000000000567eaf in js::GlobalHelperThreadState::finishParseTask (this=<optimized out>, maybecx=maybecx@entry=0x17376c0, rt=rt@entry=0x1715260, token=token@entry=0x1810600) at js/src/vm/HelperThreads.cpp:938
#5  0x0000000000833094 in JS::FinishOffThreadScript (maybecx=maybecx@entry=0x17376c0, rt=rt@entry=0x1715260, token=token@entry=0x1810600) at js/src/jsapi.cpp:3854
#6  0x000000000041b58c in runOffThreadScript (cx=cx@entry=0x17376c0, argc=<optimized out>, vp=0x7fffffffc2a8) at js/src/shell/js.cpp:3552
#7  0x000000000055d175 in CallJSNative (args=..., native=0x41b4f0 <runOffThreadScript(JSContext*, unsigned int, jsval*)>, cx=0x17376c0) at js/src/jscntxtinlines.h:235
#8  js::Invoke (cx=cx@entry=0x17376c0, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:502
#9  0x000000000055e56a in js::Invoke (cx=cx@entry=0x17376c0, thisv=..., fval=..., argc=0, argv=<optimized out>, rval=...) at js/src/vm/Interpreter.cpp:558
#10 0x00000000008e7495 in js::DirectProxyHandler::call (this=this@entry=0x16f70c0 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x17376c0, proxy=..., proxy@entry=..., args=...) at js/src/proxy/DirectProxyHandler.cpp:77
#11 0x00000000008f38f1 in js::CrossCompartmentWrapper::call (this=0x16f70c0 <js::CrossCompartmentWrapper::singleton>, cx=0x17376c0, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:288
#12 0x0000000000908ee7 in js::Proxy::call (cx=cx@entry=0x17376c0, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:391
#13 0x0000000000908fca in js::proxy_Call (cx=cx@entry=0x17376c0, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:703
#14 0x000000000055d330 in CallJSNative (args=..., native=0x908f80 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, cx=0x17376c0) at js/src/jscntxtinlines.h:235
#15 js::Invoke (cx=0x17376c0, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:495
#16 0x000000000055808d in Interpret (cx=0x17376c0, state=...) at js/src/vm/Interpreter.cpp:2600
#17 0x000000000055ce7d in js::RunScript (cx=cx@entry=0x17376c0, state=...) at js/src/vm/Interpreter.cpp:452
#18 0x0000000000562bb0 in js::ExecuteKernel (cx=cx@entry=0x17376c0, script=script@entry=..., scopeChainArg=..., thisv=..., type=type@entry=js::EXECUTE_GLOBAL, evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x0) at js/src/vm/Interpreter.cpp:655
#19 0x00000000005647ec in js::Execute (cx=cx@entry=0x17376c0, script=script@entry=..., scopeChainArg=..., rval=rval@entry=0x0) at js/src/vm/Interpreter.cpp:695
#20 0x0000000000832b73 in ExecuteScript (cx=cx@entry=0x17376c0, obj=..., scriptArg=..., rval=rval@entry=0x0) at js/src/jsapi.cpp:4091
#21 0x0000000000832bf6 in JS_ExecuteScript (cx=cx@entry=0x17376c0, scriptArg=..., scriptArg@entry=...) at js/src/jsapi.cpp:4113
#22 0x00000000004059e5 in RunFile (compileOnly=false, file=0x180cf00, filename=<optimized out>, cx=0x17376c0) at js/src/shell/js.cpp:466
#23 Process (cx=cx@entry=0x17376c0, filename=<optimized out>, forceTTY=forceTTY@entry=false) at js/src/shell/js.cpp:597
#24 0x00000000004133e6 in ProcessArgs (op=0x7fffffffd9b0, cx=0x17376c0) at js/src/shell/js.cpp:5738
#25 Shell (envp=<optimized out>, op=0x7fffffffd9b0, cx=0x17376c0) at js/src/shell/js.cpp:6004
#26 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:6346
rax	0x7fffffffb8f0	140737488337136
rbx	0x17376c0	24344256
rcx	0x17376c0	24344256
rdx	0x7fffffffb850	140737488336976
rsi	0x1	1
rdi	0x4b4b4b4b4b4b4b4b	5425512962855750475
rbp	0x4b4b4b4b4b4b4b4b	5425512962855750475
rsp	0x7fffffffb7e0	140737488336864
r8	0x180ff98	25231256
r9	0x5c	92
r10	0x7ffff5671000	140737310560256
r11	0x0	0
r12	0x1	1
r13	0x7fffffffb840	140737488336960
r14	0x7fffffffb850	140737488336976
r15	0x0	0
rip	0x43f3a5 <js::NativeObject::setSlot(unsigned int, JS::Value const&)+21>
=> 0x43f3a5 <js::NativeObject::setSlot(unsigned int, JS::Value const&)+21>:	mov    0x8(%rdi),%rax
   0x43f3a9 <js::NativeObject::setSlot(unsigned int, JS::Value const&)+25>:	mov    0x10(%rax),%eax


Marking s-s because this looks like a use-after-free.
Keywords: sec-high
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/4f64a711181d
user:        Steve Fink
date:        Tue Jan 13 14:01:42 2015 -0800
summary:     Bug 1117768 - Fix assertion in AutoStopVerifyingBarriers and add tests, r=terrence

This iteration took 146.429 seconds to run.
Needinfo from sfink based on comment 1.
Flags: needinfo?(sphink)
Group: javascript-core-security
Can you please look at this Terrence? It has been sitting around for 3 months. Thanks.
Flags: needinfo?(terrence)
I was able to reproduce this easily. It is incredibly unlikely to be Steve's patch and is likely just tweaked timing. It seems that we are doing a GC while a finished parseTask is in the finished list, but before it has been copied to a RootedScript on the stack. I'm not sure how this is supposed to work normally, so it's hard to say what the correct solution is.

Brian, do you know where parseTask->script is supposed to be marked from, or what invariant is supposed to keep GC from happening while it is kept in the parseFinishedList?
Flags: needinfo?(terrence)
Flags: needinfo?(sphink)
Flags: needinfo?(bhackett1024)
(In reply to Terrence Cole [:terrence] from comment #4)
> I was able to reproduce this easily. It is incredibly unlikely to be Steve's
> patch and is likely just tweaked timing. It seems that we are doing a GC
> while a finished parseTask is in the finished list, but before it has been
> copied to a RootedScript on the stack. I'm not sure how this is supposed to
> work normally, so it's hard to say what the correct solution is.
> 
> Brian, do you know where parseTask->script is supposed to be marked from, or
> what invariant is supposed to keep GC from happening while it is kept in the
> parseFinishedList?

Off thread parse tasks have their own zones, for which usedByExclusiveThread is set.  This prevents the zones from being collected by the GC, whether they are in active use by another thread or just sitting on the parseFinishedList.

In GlobalHelperThreadState::finishParseTask we call LeaveParseTaskZone, which will clear the bit on the off thread zone and make it available for collection, and then (without doing anything that can actually trigger GC) we call MergeCompartments, which moves the contents of that off thread zone into another zone which can also be collected.

Immediately after MergeCompartments, parseTask->finish() is called, which does some things that can GC (cx->compartment()->wrap()) without the parse task's script being properly rooted.  So I think that's the bug, but I don't know what those wrap calls are there for or whether they will actually be wrapping anything in this testcase.  So if you can post the stack trace of the last GC that would be really helpful.
Flags: needinfo?(bhackett1024)
Flags: needinfo?(terrence)
Flags: needinfo?(terrence) → needinfo?(bhackett1024)
That backtrace has the GC occurring under the AutoFinishGC thing, which was added by bug 1137341 (which landed not long before this bug appeared, though I don't know why bisection is blaming a bug that landed even earlier).  That AutoFinishGC is in the wrong place, as we shouldn't be doing any GC'ing after calling LeaveParseTaskZone.

The wrapping issue also seems to be a bug though it isn't what's causing the crash here.  That looks like a regression from bug 1031636.

I would like to be able to needinfo jonco and jimb for this, but bugzilla refuses to let me --- if I needinfo them then bugzilla whines about them not being on the cc list, and if I try to add them to the cc list it whines about not knowing about some mysterious user named 'other'.
Flags: needinfo?(bhackett1024)
Setting needinfo? as per comment 7.
Flags: needinfo?(jimb)
Flags: needinfo?(jcoppeard)
I moved the merging code into a separate method and made it finish any ongoing GC at the beginning, before calling LeaveParseTaskZone().

I had to make a slight change to the assertions in AutoTraceSession, used via AutoPrepareForTracing in MergeCompartments().  Previously this asserted that allocation was allowed to prove that a GC can only happen where we are able to allocate.  I moved this assertion elsewhere in the GC so that AutoTraceSession can be used in regions where we've already disallowed allocation.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Attachment #8634180 - Flags: review?(bhackett1024)
Comment on attachment 8634180 [details] [diff] [review]
bug1146213-finish-gc-before-parse-merge

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

::: js/src/jit-test/tests/gc/bug1146213.js
@@ +16,5 @@
> +schedulegc(this)
> +gcslice(3)
> +var lfGlobal = newGlobal();
> +lfGlobal.offThreadCompileScript("");
> +lfGlobal.runOffThreadScript();

This testcase shouldn't go in until this bug is fixed everywhere.

::: js/src/vm/HelperThreads.cpp
@@ +962,5 @@
> +                                                   JSCompartment* dest)
> +{
> +    // It's not safe to GC finished merging the contents of the parse task's
> +    // compartment into the destination compartment.  Finish any ongoing
> +    // incremental GC first and assert that no allocation can occur.

This comment is garbled.
Attachment #8634180 - Flags: review?(bhackett1024) → review+
Comment on attachment 8634180 [details] [diff] [review]
bug1146213-finish-gc-before-parse-merge

[Security approval request comment]

Sorry, I forgot to get approval before landing this, again :(

How easily could an exploit be constructed based on the patch?

Difficult as it relies on the timing of GC and off thread parsing.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

Back to FF38.

If not all supported branches, which bug introduced the flaw?

Bug 1137341.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

It shouldn't be hard to backport this.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #8634180 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/679e80ff389c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8634180 [details] [diff] [review]
bug1146213-finish-gc-before-parse-merge

Approval Request Comment
[Feature/regressing bug #]: Bug 1137341
[User impact if declined]: A sec-high in the wild.
[Describe test coverage new/current, TreeHerder]: Green on m-c and included a test when it landed.
[Risks and why]: Low risk per comment 11.
[String/UUID change made/needed]: None
Attachment #8634180 - Flags: approval-mozilla-esr38?
Attachment #8634180 - Flags: approval-mozilla-beta?
Attachment #8634180 - Flags: approval-mozilla-aurora?
Scratch that, a test was included with the attached patch but wasn't landed with the fix.
abillings - This sec-high landed accidentally on m-c without sec approval. Do you have time to review for sec approval today so that we can consider taking the fix in beta7?
Flags: needinfo?(abillings)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #16)
> abillings - This sec-high landed accidentally on m-c without sec approval.
> Do you have time to review for sec approval today so that we can consider
> taking the fix in beta7?

We should take this on all affected branches, especially now that it has gone into trunk.
Flags: needinfo?(abillings) → needinfo?(lmandel)
Attachment #8634180 - Flags: sec-approval?
Attachment #8634180 - Flags: sec-approval+
Attachment #8634180 - Flags: approval-mozilla-esr38?
Attachment #8634180 - Flags: approval-mozilla-esr38+
Attachment #8634180 - Flags: approval-mozilla-beta?
Attachment #8634180 - Flags: approval-mozilla-beta+
Attachment #8634180 - Flags: approval-mozilla-aurora?
Attachment #8634180 - Flags: approval-mozilla-aurora+
Clearing my ni. Al already marked the uplift approvals.
Flags: needinfo?(lmandel)
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main40+][adv-esr38.2+]
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx40
JSBugMon: This bug has been automatically verified fixed on Fx41
JSBugMon: This bug has been automatically verified fixed on Fx42
Group: javascript-core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.