1412653 - Assertion failure: blFrame->numValueSlots() <= script->nslots(), at js/src/jit/BaselineBailouts.cpp:1011 with Debugger and OOM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision c16bc8097c10 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

g = newGlobal();
g.parent = this;
g.eval("new Debugger(parent).onExceptionUnwind = function () {}");
function pushLimits(limit, offset) {
    function pusher() {
        Array.prototype.push.apply(arr, arguments)
    }
    var arr = [0, 1, 2, 3, 4, 5, 6, 7];
    oomAtAllocation(limit);
    for (var i = 0; i < 100; i++) pusher(0, 1, 2, 3, 4, 5, 6, 7);
}
var limit = 1024 * 1024 * 1024;
while (true) {
    var res = pushLimits(limit, 0);
    limit = (limit / 1.5) | 0;
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000000000efacf5 in InitFromBailout (caller=..., ionScript=<optimized out>, excInfo=0x7fffffffc0e0, callPC=<synthetic pointer>, nextCallee=..., startFrameFormals=..., builder=..., invalidate=true, iter=..., script=..., fun=..., callerPC=<optimized out>, cx=0x7ffff6948000) at js/src/jit/BaselineBailouts.cpp:1011
#1  js::jit::BailoutIonToBaseline (cx=cx@entry=0x7ffff6948000, activation=0x7fffffffc930, iter=..., invalidate=invalidate@entry=true, bailoutInfo=bailoutInfo@entry=0x7fffffffbbe8, excInfo=excInfo@entry=0x7fffffffc0e0) at js/src/jit/BaselineBailouts.cpp:1656
#2  0x0000000000efcb51 in js::jit::ExceptionHandlerBailout (cx=cx@entry=0x7ffff6948000, frame=..., rfe=rfe@entry=0x7fffffffc62c, excInfo=..., overrecursed=overrecursed@entry=0x7fffffffbf6f) at js/src/jit/Bailouts.cpp:218
#3  0x000000000077e4e1 in js::jit::HandleExceptionIon (overrecursed=0x7fffffffbf6f, rfe=0x7fffffffc62c, frame=..., cx=0x7ffff6948000) at js/src/jit/JitFrames.cpp:204
#4  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:673
#5  0x000010cc1f39f676 in ?? ()
#6  0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x90	144
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbb90	140737488337808
rsp	0x7fffffffb690	140737488336528
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0xa	10
r13	0x7fffffffb890	140737488337040
r14	0x7fffffffc0e0	140737488339168
r15	0x7ffff6946180	140737330307456
rip	0xefacf5 <js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*)+11861>
=> 0xefacf5 <js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*)+11861>:	movl   $0x0,0x0
   0xefad00 <js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, bool, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*)+11872>:	ud2


I'm marking this s-s until investigated because the assertion sounds quite dangerous. The test uses Debugger, so it is at most sec-moderate if the use of the Debugger is required. But I could imagine that there might be other ways to cause this bailout, so I'll leave this to the JS devs to decide.

Comment 1

[Fuzzing Team]

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/8b1881ead0b6
user:        Nicolas B. Pierron
date:        Thu Sep 07 13:01:13 2017 +0000
summary:     Bug 966743 - Inline Array.prototype.push with more than one argument. r=jandem

This iteration took 267.413 seconds to run.

Comment 2

[Daniel Veditz [:dveditz]]

Calling it sec-high out of caution because probable regressor had nothing to do with the Debugger code, and jorendorff was worried enough to make this a P1.

Comment 3

[Nicolas B. Pierron [:nbp — off until 29-09]]

I am able to reproduce this issue, I will investigate it more on Monday.

Comment 4

[Nicolas B. Pierron [:nbp — off until 29-09]]

(In reply to Christian Holler (:decoder) from comment #0)
>     function pusher() {
>         Array.prototype.push.apply(arr, arguments)
>     }

The problem is here, and suppose that we OOM or freeze of the array "arr", which cause the array.push to fail and bailout, then we would be restoring twice the set of arguments, as part of the bailout frame.

I am not completely sure of the security implication of this modification, but I am not confident that the Debugger is mandatory for creating a similar issue.

Comment 5

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: Disable Array.push optimization when given multiple arguments. r=

This patch would be good if we need to backport a fix, and if I cannot find
a better solution in the mean time.

This restores the behaviour that we had before Bug 966743.

Comment 6

[Julien Cristau [:jcristau]]

It's too late for 57, wontfix.

Comment 7

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: Update test cases to cover all code paths.

This adds the fuzzer's test case in addition to a copy of the existing
fun.apply test case for fun.call, just to ensure that we can bailout
correctly as well with fun.call.

Comment 8

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction.

This patch rename pushFormals to pushCallStack and pushPriorCallStack.

 - pushCallStack is used for filling the content of the Outer resume
   points. This content is expected to explicitly list of the arguments,
   such that the call-frame can be emulated in Baseline.

 - pushPriorCallStack is used for filling the content of the stack for At
   resume points. This content is expected to be used for resuming at the
   call instruction, with enough content to resume the execution properly.

The problem seen here happens when we reconstruct the baseline frame under
HandleException (when resizing the array?). From this point of view I would
not think that this issue is restricted to the Debugger.

The problem seen here, is that the resume point contained the full set of
arguments instead of the state of the stack prior JSOP_FUNAPPLY.  Thus, the
frame recovery which attempted to reconstruct the stack certainly gets it
wrong.

This patch adds machinery to capture the stack, if needed, such that it can
be restored properly as-is. It defaults to restoring the set of call-stack
definition if no state were captured.  Thus JSOP_FUNCALL get one argument less
when restored (could that cause Debugger issues?).

This should solve the issue, as we restore a proper stack where the
execution can be resumed, and satisfies the bailout assertion raised under
HandleException.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8928198 [details] [diff] [review]
Update test cases to cover all code paths.

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

We will land the tests once this is in release, right?

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8928206 [details] [diff] [review]
Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction.

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

Sorry for the delay; I thought this was more complicated but the patch makes sense.

::: js/src/jit/IonBuilder.h
@@ +1255,5 @@
>      }
>  
> +    // Before doing any pop to the stack, capture whatever flows into the
> +    // instruction, such that we can restore it later.
> +    AbortReasonOr<Ok> savePriorCallStack(MIRGenerator* mir, MBasicBlock* current, size_t peekDepth)

Nit: we should define this in IonBuilder.cpp since it's pretty big and inlining it doesn't matter (and the caller is in IonBuilder.cpp too).

@@ +1261,5 @@
> +        MOZ_ASSERT(priorArgs_.empty());
> +        if (!priorArgs_.reserve(peekDepth))
> +            return mir->abort(AbortReason::Alloc);
> +        while (peekDepth) {
> +            priorArgs_.infallibleAppend(current->peek(0 - peekDepth));

Nit: int32_t(peekDepth) to make the unsigned-to-signed conversion explicit and (maybe) to keep some compilers from complaining.

Comment 11

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8928206 [details] [diff] [review]
Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

I tried to do it without the Debugger, and failed to make something crash.  Still, I think this is possible, as this MArrayPush is made to bailout, and resume its execution in Baseline, thus resuming with a corrupted JS stack, and failing again.

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

Yes, based on the patch it is obvious that the way the stack is recovered on a bailout is different in case of MArrayPush with multiple arguments.

> Which older supported branches are affected by this flaw?

Firefox 57, and newer.

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

Bug 966743

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

The same patch should apply cleanly.

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

Not likely. The test added in the previous patch, and the existing test should be enough.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8928206 [details] [diff] [review]
Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction.

sec-approval+ for trunk. If you nominate it for beta, I can approve it as well so we fix it everywhere affected.

Comment 13

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8928206 [details] [diff] [review]
Distinguish call stack used for outer resume points than call stacks used for resuming at one instruction.

Landed, with fixup patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6596b8c168f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/43ccb7de1afe

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/43ccb7de1afe
https://hg.mozilla.org/mozilla-central/rev/6596b8c168f5

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for Beta uplift. Please post a rebased patch and nominate it for approval when you get a chance.

Comment 17

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: Distinguish between call stack used for outer resume points from call stacks used for resuming at one instruction.

This is the beta backport patch.

Comment 18

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8936854 [details] [diff] [review]
Distinguish between call stack used for outer resume points from call stacks used for resuming at one instruction.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 966743
[User impact if declined]: Potential crash, and misuse of pointers.
[Is this code covered by automated tests?]: (new & old test cases works)
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: no.
[List of other uplifts needed for the feature/fix]: none.
[Is the change risky?]: no.
[Why is the change risky/not risky?]: this patch mostly rename a function, and changes the behaviour of one to match the expected resume state.
[String changes made/needed]: none.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8936854 [details] [diff] [review]
Distinguish between call stack used for outer resume points from call stacks used for resuming at one instruction.

Sec-high, Beta58+

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/935393b713cb

Comment 21

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx58