Closed
Bug 891808
Opened 8 years ago
Closed 8 years ago
Assertion failure: scope == cx->global(), at ../vm/Interpreter-inl.h:285 or Crash [@ PrepareOsrTempData]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox22 | --- | unaffected |
firefox23 | + | fixed |
firefox24 | + | fixed |
firefox25 | + | fixed |
b2g18 | --- | unaffected |
b2g-v1.1hd | --- | unaffected |
b2g-v1.2 | --- | unaffected |
People
(Reporter: decoder, Assigned: h4writer)
References
Details
(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main23-])
Crash Data
Attachments
(5 files, 5 obsolete files)
331 bytes,
application/x-javascript
|
Details | |
9.06 KB,
patch
|
jandem
:
review+
lsblakk
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
475 bytes,
patch
|
Details | Diff | Splinter Review | |
1.52 KB,
text/plain
|
Details | |
7.20 KB,
patch
|
h4writer
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The following testcase asserts on mozilla-central revision 04d8c309fe72 (run with --fuzzing-safe --ion-eager): gczeal(2); function bind(f) {} function g(a, b) function V(f) bind(); for(var i=0; i<2000; i++) { g.call({}, bind(function(){})); }
Reporter | ||
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
Crash trace: Program received signal SIGSEGV, Segmentation fault. PrepareOsrTempData (jitcode=0x7ffff7e0d4c3, frame=<optimized out>, cx=<optimized out>, stub=<optimized out>, script=..., pc=<optimized out>) at js/src/ion/BaselineIC.cpp:841 841 stackFrameLocalsStart[i] = *(frame->valueSlot(i)); (gdb) bt #0 PrepareOsrTempData (jitcode=0x7ffff7e0d4c3, frame=<optimized out>, cx=<optimized out>, stub=<optimized out>, script=..., pc=<optimized out>) at js/src/ion/BaselineIC.cpp:841 #1 DoUseCountFallback (infoPtr=0x7fffffffd2a8, frame=<optimized out>, stub=<optimized out>, cx=<optimized out>) at js/src/ion/BaselineIC.cpp:902 #2 js::ion::DoUseCountFallback (cx=<optimized out>, stub=<optimized out>, frame=<optimized out>, infoPtr=0x7fffffffd2a8) at js/src/ion/BaselineIC.cpp:851 #3 0x00007ffff6bcbfe8 in ?? () #4 0x00007ffff6bb04a1 in ?? () (More stack frames follow...) (gdb) x /i $pc => 0x668e58 <js::ion::DoUseCountFallback(JSContext*, js::ion::ICUseCount_Fallback*, js::ion::BaselineFrame*, js::ion::IonOsrTempData**)+648>: mov %rdx,0x78(%rcx,%rax,8) (gdb) info reg rcx rax rcx 0x1692a78 23669368 rax 0xdea2 56994 Valgrind shows more invalid reads: ==18618== Invalid write of size 8 ==18618== at 0x668E58: js::ion::DoUseCountFallback(JSContext*, js::ion::ICUseCount_Fallback*, js::ion::BaselineFrame*, js::ion::IonOsrTempData**) (BaselineIC.cpp:841) ==18618== by 0x41CCFE7: ??? ==18618== Address 0x638fbb0 is 8 bytes after a block of size 136 alloc'd ==18618== at 0x4C2B6CD: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==18618== by 0x4C2B857: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==18618== by 0x6CD08F: js::ion::IonRuntime::allocateOsrTempData(unsigned long) (Utility.h:163) ==18618== by 0x668DE5: js::ion::DoUseCountFallback(JSContext*, js::ion::ICUseCount_Fallback*, js::ion::BaselineFrame*, js::ion::IonOsrTempData**) (BaselineIC.cpp:806) ==18618== by 0x41CCFE7: ??? Marking s-s and sec-high based on that.
Crash Signature: [@ PrepareOsrTempData]
Whiteboard: [jsbugmon:update,bisect]
Reporter | ||
Updated•8 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 3•8 years ago
|
||
JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: http://hg.mozilla.org/mozilla-central/rev/c9455df0575c user: Hannes Verschore date: Thu Jun 20 18:11:25 2013 +0200 summary: Bug 884310 - IonMonkey: Inline function called from .call(), r=jandem This iteration took 320.287 seconds to run.
Comment 5•8 years ago
|
||
Hannes and I were debugging this. It's a pretty nasty bug when we bailout from inlined funcall or funapply. Attaching a funapply testcase that fails. It doesn't crash but with some more work it should be possible I think.
Comment 6•8 years ago
|
||
I'm not sure if 22 (bailout to interpreter) is affected. But 23+ (bailout to baseline) is.
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox25:
--- → affected
tracking-firefox23:
--- → ?
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
Assignee | ||
Comment 7•8 years ago
|
||
This fixes the funcall issue, like discussed on irc. Now looking into the funapply issue.
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 774567 [details] [diff] [review] Fix funcall oh wait, there might be a small issue with this patch. Checking first
Attachment #774567 -
Flags: review?(jdemooij)
Assignee | ||
Comment 9•8 years ago
|
||
This should be better. Only fixup when we have inlined the JSOP_FUNCALL.
Attachment #774567 -
Attachment is obsolete: true
Attachment #774574 -
Flags: review?(jdemooij)
Assignee | ||
Updated•8 years ago
|
Attachment #774574 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•8 years ago
|
||
Third time's a charm? So this fixes both the funcall and funapply issue. The patch is a bit bigger than wanted (esp. since we will have to uplift this :( ). Now on the caller side of funcall/funapply we see the right amount of arguments. (= the not inlined amount). On the callee side we still see the inlined amount of arguments.
Attachment #774574 -
Attachment is obsolete: true
Attachment #774652 -
Flags: review?(jdemooij)
Reporter | ||
Comment 11•8 years ago
|
||
Attachment #773205 -
Attachment is obsolete: true
Comment 12•8 years ago
|
||
Comment on attachment 774652 [details] [diff] [review] Fix funcall and funapply Review of attachment 774652 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! A few nits and a more serious issue, would be good to have a testcase for that (even if we don't land it immediately). ::: js/src/ion/BaselineBailouts.cpp @@ +627,5 @@ > + // Fixup inlined JSOP_FUNCALL and JSOP_FUNAPPLY on the caller side. > + // On the caller side this must represent like the function wasn't inlined. > + uint32_t pushedSlots = 0; > + jsbytecode *current_pc = script->code + iter.pcOffset(); > + JSOp current_op = JSOp(*current_pc); This function also has this a bit lower: jsbytecode *pc = script->code + iter.pcOffset(); JSOp op = JSOp(*pc); Can you move these up and use them instead? @@ +643,5 @@ > + pushedSlots = exprStackSlots - inlined_args; > + > + IonSpew(IonSpew_BaselineBailouts, > + " pushing %d expression stack slots before fixup", > + int(pushedSlots)); Nit: Use %u instead of %d and remove the int() cast. @@ +661,5 @@ > + if (!builder.writeValue(UndefinedValue(), "StackValue")) > + return false; > + } > + > + if (JSOp(*current_pc) == JSOP_FUNAPPLY && iter.moreFrames()) { The outer |if| checks iter.moreFrames() too, so you can remove it here. @@ +669,5 @@ > + // This means transforming the stack from |target, this, arg1, ...| > + // to |js_fun_apply, target, this, argObject|. > + // Since the information is never read, we can just push undefined > + // for all values. > + IonSpew(IonSpew_BaselineBailouts, " pushing undefined to fixup funcall"); Nit: s/funcall/funapply @@ +683,5 @@ > + // Save the actual arguments. They are needed on the callee side > + // as the arguments. Else we can't recover them. > + if (!funapplyargs.resize(inlined_args)) > + return false; > + for (uint32_t i = 0; i < inlined_args; i++) { Nit: no { } @@ +973,5 @@ > + > + JS_ASSERT(actualArgc + 2 <= exprStackSlots); > + JS_ASSERT(funapplyargs.length() == actualArgc + 2); > + for (unsigned i = 0; i < actualArgc + 1; i++) { > + if (!builder.writeValue(funapplyargs[i + 1], "ArgVal")) This should start at the end of the vector (callee arguments should be written last-to-first). Can you try to write a testcase that fails with the current patch to verify that works?
Attachment #774652 -
Flags: review?(jdemooij)
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #774652 -
Attachment is obsolete: true
Attachment #776763 -
Flags: review?(jdemooij)
Assignee | ||
Comment 14•8 years ago
|
||
Comment 15•8 years ago
|
||
Comment on attachment 776763 [details] [diff] [review] Patch for funapply/funcall Review of attachment 776763 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks for writing that testcase. We should try to get this in 23, I can see this break websites..
Attachment #776763 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Reporter | ||
Comment 16•8 years ago
|
||
Reporter | ||
Updated•8 years ago
|
Attachment #775546 -
Attachment is obsolete: true
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 776763 [details] [diff] [review] Patch for funapply/funcall [Security approval request comment] How easily could an exploit be constructed based on the patch? It's quite easy to derive that this is about bailing a script that inlined funapply or funcall. So I think it wouldn't be that hard :( Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? comments in patch: contains information that is derivable from patch itself. But is way more verbose that only the code adjustments ... check-in comment: "IonMonkey: Improve bailing information for JSOP_FUNAPPLY and JSOP_FUNCALL, r=jandem". Quite obvious from patch it is about that tests: not included Which older supported branches are affected by this flaw? FF23/FF24/FF25 If not all supported branches, which bug introduced the flaw? The landing of baseline Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I haven't created them yet, but they are not hard to make. Quick look shows no difference for those branches How likely is this patch to cause regressions; how much testing does it need? Shouldn't need dedicated testing. The testcases here should normally report fallout.
Attachment #776763 -
Flags: sec-approval?
Comment 18•8 years ago
|
||
Comment on attachment 776763 [details] [diff] [review] Patch for funapply/funcall sec-approval+ for trunk. Since this was +'d for branches by Release Management, please create an aurora and beta set of patches ASAP so we can get this in before beta is too late in the cycle.
Attachment #776763 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 19•8 years ago
|
||
Since I only have a approval requesting for beta bug. Aurora bug is the same as trunk [Security approval request comment] How easily could an exploit be constructed based on the patch? It's quite easy to derive that this is about bailing a script that inlined funapply or funcall. So I think it wouldn't be that hard :( Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? comments in patch: contains information that is derivable from patch itself. But is way more verbose that only the code adjustments ... check-in comment: "IonMonkey: Improve bailing information for JSOP_FUNAPPLY and JSOP_FUNCALL, r=jandem". Quite obvious from patch it is about that tests: not included Which older supported branches are affected by this flaw? FF23/FF24/FF25 If not all supported branches, which bug introduced the flaw? The landing of baseline Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Just created for beta. Aurora has the same patch as inbound. How likely is this patch to cause regressions; how much testing does it need? Shouldn't need dedicated testing. The testcases here should normally report fallout.
Attachment #778651 -
Flags: sec-approval?
Attachment #778651 -
Flags: review+
Assignee | ||
Comment 20•8 years ago
|
||
Comment on attachment 776763 [details] [diff] [review] Patch for funapply/funcall [Approval Request Comment] Bug caused by (feature/regressing bug #): Landing of baseline. Can't immediately find the bug where it is was merged into inbound. Baseline is only enabled since ff23 User impact if declined: Issues when using funapply/funcall. Reading values from wrong stack locations. Very dangerous. Testing completed (on m-c, etc.): jit-tests completed and testcases are fine. Gonna push inbound any minute now. But I want to land on all trees as close as possible together Risk to taking this patch (and alternatives if risky): The state without this patch is riskier. The fix is in a hairy part of the engine, but is well understood and the tests show that the code should be correct. String or IDL/UUID changes made by this patch: /
Attachment #776763 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/24eb84ceb5ed
Comment 22•8 years ago
|
||
Not clear why you need sec-approval on the beta patch, one sec-approval to land on central should rule them all :)
Comment 23•8 years ago
|
||
Comment on attachment 778651 [details] [diff] [review] Patch for beta nominate for beta approval please
Attachment #778651 -
Flags: sec-approval?
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 778651 [details] [diff] [review] Patch for beta [Approval Request Comment] Bug caused by (feature/regressing bug #): Landing of baseline. Can't immediately find the bug where it is was merged into inbound. Baseline is only enabled since ff23 User impact if declined: Issues when using funapply/funcall. Reading values from wrong stack locations. Very dangerous. Testing completed (on m-c, etc.): jit-tests completed and testcases are fine. Trunk is pushed and green. I want to land on all trees as close as possible together. Risk to taking this patch (and alternatives if risky): The state without this patch is riskier. The fix is in a hairy part of the engine, but is well understood and the tests show that the code should be correct. String or IDL/UUID changes made by this patch: /
Attachment #778651 -
Flags: approval-mozilla-beta?
Reporter | ||
Updated•8 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Reporter | ||
Comment 25•8 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f80683d8c3e7).
Comment 26•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/24eb84ceb5ed
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Reporter | ||
Updated•8 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 28•8 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Updated•8 years ago
|
Attachment #776763 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•8 years ago
|
Attachment #778651 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d2ec54e97b5 https://hg.mozilla.org/releases/mozilla-beta/rev/6bb15bfb8887
Comment 30•8 years ago
|
||
Backed out of beta due to dromaeo crashes (yes, I pushed "Patch for beta"). https://hg.mozilla.org/releases/mozilla-beta/rev/b154fb80409e https://tbpl.mozilla.org/php/getParsedLog.php?id=25581204&tree=Mozilla-Beta
Updated•8 years ago
|
Flags: needinfo?(hv1989)
Assignee | ||
Comment 31•8 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #30) > Backed out of beta due to dromaeo crashes (yes, I pushed "Patch for beta"). > https://hg.mozilla.org/releases/mozilla-beta/rev/b154fb80409e > > https://tbpl.mozilla.org/php/getParsedLog.php?id=25581204&tree=Mozilla-Beta Thanks for landing and seeing this issue. I somehow messed up the merging and there was a small part not merged (the last part). Haven't been able to create a jit-test out of it, but pushed the full patch to try and that is green. https://tbpl.mozilla.org/?tree=Try&rev=3ecacd1791db Repushed: https://hg.mozilla.org/releases/mozilla-beta/rev/ab34727c13c9
Flags: needinfo?(hv1989)
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
status-firefox22:
--- → unaffected
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main23-]
Updated•7 years ago
|
status-b2g18:
--- → unaffected
status-b2g-v1.1hd:
--- → unaffected
status-b2g-v1.2:
--- → unaffected
Updated•7 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•