Closed Bug 891808 Opened 6 years ago Closed 6 years ago

Assertion failure: scope == cx->global(), at ../vm/Interpreter-inl.h:285 or Crash [@ PrepareOsrTempData]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

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

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main23-])

Crash Data

Attachments

(5 files, 5 obsolete files)

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(){}));
}
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]
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:   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.
Hannes, is comment 3 correct?
Flags: needinfo?(hv1989)
Attached file Apply testcase
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.
I'm not sure if 22 (bailout to interpreter) is affected. But 23+ (bailout to baseline) is.
Attached patch Fix funcall (obsolete) — Splinter Review
This fixes the funcall issue, like discussed on irc. Now looking into the funapply issue.
Assignee: general → hv1989
Attachment #774567 - Flags: review?(jdemooij)
Flags: needinfo?(hv1989)
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)
Attached patch Fix funcall (obsolete) — Splinter Review
This should be better. Only fixup when we have inlined the JSOP_FUNCALL.
Attachment #774567 - Attachment is obsolete: true
Attachment #774574 - Flags: review?(jdemooij)
Attachment #774574 - Flags: review?(jdemooij)
Attached patch Fix funcall and funapply (obsolete) — Splinter Review
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)
Attachment #773205 - Attachment is obsolete: true
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)
Attachment #774652 - Attachment is obsolete: true
Attachment #776763 - Flags: review?(jdemooij)
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+
Blocks: 893734
Attachment #775546 - Attachment is obsolete: true
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 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+
Attached patch Patch for betaSplinter Review
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+
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?
Not clear why you need sec-approval on the beta patch, one sec-approval to land on central should rule them all :)
Comment on attachment 778651 [details] [diff] [review]
Patch for beta

nominate for beta approval please
Attachment #778651 - Flags: sec-approval?
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?
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision f80683d8c3e7).
https://hg.mozilla.org/mozilla-central/rev/24eb84ceb5ed
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Duplicate of this bug: 893734
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Attachment #776763 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #778651 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(hv1989)
(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)
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.