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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, critical)

Description

[Christian Holler (:decoder)]

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(){}));
}

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Christian Holler (:decoder)]

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 4

[Jan de Mooij [:jandem]]

Hannes, is comment 3 correct?

Comment 5

[Jan de Mooij [:jandem]]

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

Comment 6

[Jan de Mooij [:jandem]]

I'm not sure if 22 (bailout to interpreter) is affected. But 23+ (bailout to baseline) is.

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: Fix funcall

This fixes the funcall issue, like discussed on irc. Now looking into the funapply issue.

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 774567 [details] [diff] [review]
Fix  funcall

oh wait, there might be a small issue with this patch. Checking first

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: Fix funcall

This should be better. Only fixup when we have inlined the JSOP_FUNCALL.

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: Fix funcall and funapply

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.

Comment 11

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 12

[Jan de Mooij [:jandem]]

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?

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: Patch for funapply/funcall

Comment 14

[Hannes Verschore [:h4writer]]

Attachment: Testcase: rightly order the arguments

Comment 15

[Jan de Mooij [:jandem]]

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

Comment 16

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 17

[Hannes Verschore [:h4writer]]

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.

Comment 18

[Al Billings [:abillings]]

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.

Comment 19

[Hannes Verschore [:h4writer]]

Attachment: Patch for beta

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.

Comment 20

[Hannes Verschore [:h4writer]]

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

Comment 21

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24eb84ceb5ed

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Not clear why you need sec-approval on the beta patch, one sec-approval to land on central should rule them all :)

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 778651 [details] [diff] [review]
Patch for beta

nominate for beta approval please

Comment 24

[Hannes Verschore [:h4writer]]

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

Comment 25

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision f80683d8c3e7).

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/24eb84ceb5ed

Comment 28

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d2ec54e97b5
https://hg.mozilla.org/releases/mozilla-beta/rev/6bb15bfb8887

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Hannes Verschore [:h4writer]]

(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