Closed
Bug 1146213
Opened 9 years ago
Closed 9 years ago
Crash [@ js::NativeObject::setSlot] with off-thread compilation
Categories
(Core :: JavaScript Engine, defect)
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)
5.71 KB,
text/plain
|
Details | |
7.57 KB,
patch
|
bhackett1024
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•9 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 1•9 years ago
|
||
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.
Updated•9 years ago
|
Group: javascript-core-security
Comment 3•9 years ago
|
||
Can you please look at this Terrence? It has been sitting around for 3 months. Thanks.
Flags: needinfo?(terrence)
Comment 4•9 years ago
|
||
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)
Comment 5•9 years ago
|
||
(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)
Updated•9 years ago
|
Flags: needinfo?(terrence)
Comment 6•9 years ago
|
||
Flags: needinfo?(terrence) → needinfo?(bhackett1024)
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
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?
Updated•9 years ago
|
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-v2.2r:
--- → unaffected
status-b2g-master:
--- → affected
status-firefox40:
--- → affected
status-firefox41:
--- → affected
status-firefox42:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → affected
tracking-firefox-esr38:
--- → ?
Flags: in-testsuite?
Assignee | ||
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/679e80ff389c
https://hg.mozilla.org/mozilla-central/rev/679e80ff389c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 14•9 years ago
|
||
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?
Comment 15•9 years ago
|
||
Scratch that, a test was included with the attached patch but wasn't landed with the fix.
Comment 16•9 years ago
|
||
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)
Updated•9 years ago
|
Comment 17•9 years ago
|
||
(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)
Updated•9 years ago
|
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+
Updated•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
Clearing my ni. Al already marked the uplift approvals.
Flags: needinfo?(lmandel)
Comment 19•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/460c89e00f5c
Keywords: checkin-needed
Comment 21•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr38/rev/df2615c6d073
Flags: needinfo?(jimb)
Updated•9 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-critsmash-triage]
Updated•9 years ago
|
Whiteboard: [jsbugmon:update][post-critsmash-triage] → [jsbugmon:update][post-critsmash-triage][adv-main40+][adv-esr38.2+]
Reporter | ||
Updated•9 years ago
|
Status: RESOLVED → VERIFIED
status-firefox43:
--- → verified
Reporter | ||
Comment 22•9 years ago
|
||
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
Updated•9 years ago
|
Group: javascript-core-security
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•