1686653 - Assertion failure: !activation_->bailoutData(), at jit/JSJitFrameIter.cpp:54

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210112-b97f7d02c912 (build with --enable-debug, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --fast-warmup):

try {
evaluate(`
  function maybeInvalidate(iplusk) {}
  function importedFunc(k12) {
    for (let i62 = 100; i62 --> 0 ;) 
        maybeInvalidate(i62 + k12);
  }
  let { exports } = new WebAssembly.Instance(
    new WebAssembly.Module(
        wasmTextToBinary(\`
            (func \\\$imp (import "env" "importedFunc") (param i32))
            (func (export "exp") (param i32)
                local.get 0
                call \\\$imp
        )
        \`)
    ), {
        env: { importedFunc }
    }
  );
  function eval(k12) {
    exports.exp(k12);
  }
`);
} catch(exc) {}
eval(undefined);
let lfFunc = new Function(`
  function main() {
  function v3(v5) {
    try {
      const v6 = v3(v5, null);
    } catch(v7) {
      const v20 = eval("function w(){}");
    }
  }
  const v28 = v3(exports, null);
  }
  main();
`); new lfFunc();

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555557962b3b in js::jit::JSJitFrameIter::JSJitFrameIter(js::jit::JitActivation const*, js::jit::FrameType, unsigned char*) ()
#0  0x0000555557962b3b in js::jit::JSJitFrameIter::JSJitFrameIter(js::jit::JitActivation const*, js::jit::FrameType, unsigned char*) ()
#1  0x0000555556d9b8ff in js::JitFrameIter::settle() ()
#2  0x0000555556d9d22e in js::FrameIter::popJitFrame() ()
#3  0x0000555556d9cd3d in js::FrameIter::operator++() ()
#4  0x0000555556f546a2 in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#5  0x0000555556f53e85 in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#6  0x00005555571bf205 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#7  0x0000555556c929cb in js::ErrorToException(JSContext*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) ()
#8  0x0000555556d86d4f in js::ReportErrorNumberVA(JSContext*, js::IsWarning, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, js::ErrorArgumentsType, __va_list_tag*) ()
#9  0x000055555718c8af in JS_ReportErrorNumberASCII(JSContext*, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, ...) ()
#10 0x0000555556e73770 in js::ReportOverRecursed(JSContext*, unsigned int) ()
#11 0x00005555575f8ae8 in js::jit::BailoutIonToBaseline(JSContext*, js::jit::JitActivation*, js::jit::JSJitFrameIter const&, js::jit::BaselineBailoutInfo**, js::jit::ExceptionBailoutInfo const*) ()
#12 0x00005555575f7ee4 in js::jit::Bailout(js::jit::BailoutStack*, js::jit::BaselineBailoutInfo**) ()
#13 0x0000121052ab740a in ?? ()
#14 0x00007fffffdfdb10 in ?? ()
#15 0x00007fffffdfdad8 in ?? ()
#16 0x00007ffff6024000 in ?? ()
#17 0x0000000000000000 in ?? ()
rax	0x5555557b0ece	93824994709198
rbx	0x7fffffdfc9d0	140737486244304
rcx	0x555557ff0018	93825036910616
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffdfc790	140737486243728
rsp	0x7fffffdfc780	140737486243712
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f998c0	140737353717952
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x7ffff6024000	140737320730624
r13	0xaaaaaaaaaaaaaaaa	-6148914691236517206
r14	0x7fffffdfc9d8	140737486244312
r15	0x7fffffdfdcf0	140737486249200
rip	0x555557962b3b <js::jit::JSJitFrameIter::JSJitFrameIter(js::jit::JitActivation const*, js::jit::FrameType, unsigned char*)+219>
=> 0x555557962b3b <_ZN2js3jit14JSJitFrameIterC2EPKNS0_13JitActivationENS0_9FrameTypeEPh+219>:	movl   $0x36,0x0
   0x555557962b46 <_ZN2js3jit14JSJitFrameIterC2EPKNS0_13JitActivationENS0_9FrameTypeEPh+230>:	callq  0x555556a97c60 <abort>

This test is quite fragile and I was not able to reduce it further. Marking s-s because this looks like a JIT issue.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Nicolas B. Pierron [:nbp]]

Iain & Julian, can any of you look why the stack walking failed in this context of having JS/WASM on the stack, while being near the end of the stack?

Comment 3

[Iain Ireland [:iain]]

I can't get this to replicate locally. (Given that it seems to rely on triggering a stack overflow while we're handling a bailout, I'm not all that surprised.)

Looking at the code involved, I'm not sure we need this assertion. The other JSJITFrameIter constructor handles this case. This might be a "I'm pretty sure there's no way to get here" assertion. If so, we could maybe fix this by duplicating the code from the other constructor.

I've pushed a hypothetical fix to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af51e521b4a4752f6b51a4c989ddc3a0a737c915

decoder, can you test that patch? If it works, we can find somebody who actually knows this code at all to review it.

Comment 4

[Iain Ireland [:iain]]

Ugh, forgot that this was marked s-s. I'm 90%+ sure that it's not exploitable, but still, dumb mistake to push to try.

Comment 5

[Christian Holler (:decoder)]

Confirmed, this fixes the issue. Thanks!

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1686653: Refactor JSJitFrameIter constructors r=jandem

This fix was written by looking at the stack trace from a fuzz bug. I could not reproduce the failure locally, but decoder has confirmed that this change fixes the problem.

If we bail out of Ion, then call ReportOverRecursed in BailoutIonToBaseline with a wasm frame on the stack, then while trying to capture the stack for the error, we assert while trying to construct a JSJitFrameIter using the constructor specialized for jit->wasm frames. The other JSJitFrameIter constructor has special handling for the activation->bailoutData() case, but the jit->wasm one just asserts that the activation doesn't have any bailout data.

Benjamin wrote this code and Luke reviewed it, and unfortunately neither of them are still around to answer questions. Reading through the patch history, I can't find any reason that the jit->wasm constructor doesn't support this case. It looks like we just assumed it couldn't happen, and our assumption would've been right if it weren't for stack overflow. Rewriting the jit->wasm constructor to use the same logic as the other constructor appears to fix the problem.

Comment 7

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

https://hg.mozilla.org/integration/autoland/rev/3d8df2ccc0d3032c7227c40b2179efcfbd880229
https://hg.mozilla.org/mozilla-central/rev/3d8df2ccc0d3

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:iain, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Iain Ireland [:iain]]

After talking this over with Jan, we concluded that we don't want to uplift a fix that we can't effectively test.

Comment 10

[Jason Kratzer [:jkratzer]]

Bugmon Analysis
Verified bug as fixed on rev mozilla-central 20210714215010-7ec7dd0cc7ac.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.