1479394 - Crash [@ js::AbstractFramePtr::global] with Debugger and stackTest

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2, critical)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 0be4463d2915 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-eager --ion-offthread-compile=off):

stackTest(new Function(`
  var evalInFrame = (function evalInFrame(global) {
    var dbgGlobal = newGlobal();
    var dbg = new dbgGlobal.Debugger();
    return function evalInFrame(upCount, code) {
	dbg.addDebuggee(global);
	var frame = dbg.getNewestFrame().older;
	for (var get of (yield))
	    frame = frame.older;
    };
  })(this);
  function f() {
    evalInFrame(0, "a.push(1)");
  }
  f();
`));


Backtrace:

received signal SIGSEGV, Segmentation fault.
js::AbstractFramePtr::global (this=0x7fffffffb0c8) at js/src/vm/Stack-inl.h:694
#0  js::AbstractFramePtr::global (this=0x7fffffffb0c8) at js/src/vm/Stack-inl.h:694
#1  js::Debugger::forEachDebuggerFrame<js::Debugger::inFrameMaps(js::AbstractFramePtr)::<lambda(js::DebuggerFrame*)> >(js::AbstractFramePtr, js::Debugger::<lambda(js::DebuggerFrame*)>) (frame=..., fn=..., fn@entry=...) at js/src/vm/Debugger.cpp:2685
#2  0x0000000000b024b4 in js::Debugger::inFrameMaps (frame=..., frame@entry=...) at js/src/vm/Debugger.cpp:6608
#3  0x000000000081cbff in js::jit::HandleExceptionIon (overrecursed=0x7fffffffb1c7, rfe=0x7fffffffb830, frame=..., cx=0x7ffff5f17000) at js/src/jit/JitFrames.cpp:209
#4  js::jit::HandleException (rfe=<optimized out>) at js/src/jit/JitFrames.cpp:702
#5  0x00002712bf80d3d6 in ?? ()
[...]
#23 0x0000000000000000 in ?? ()
rax	0xe5e5e5e5e5e5e5e5	-1880844493789993499
rbx	0x7ffff4a5e660	140737297901152
rcx	0x0	0
rdx	0x3	3
rsi	0x7fffffffb137	140737488335159
rdi	0x7fffffffb0c8	140737488335048
rbp	0x7fffffffb120	140737488335136
rsp	0x7fffffffb0c0	140737488335040
r8	0x0	0
r9	0x10	16
r10	0x1d	29
r11	0x7ffff3e66040	140737285349440
r12	0x7fffffffb0c8	140737488335048
r13	0x7ffff5f17000	140737319628800
r14	0x7fffffffb2e0	140737488335584
r15	0x7fffffffbac0	140737488337600
rip	0xb022f4 <js::Debugger::forEachDebuggerFrame<js::Debugger::inFrameMaps(js::AbstractFramePtr)::<lambda(js::DebuggerFrame*)> >(js::AbstractFramePtr, js::Debugger::<lambda(js::DebuggerFrame*)>)+68>
=> 0xb022f4 <js::Debugger::forEachDebuggerFrame<js::Debugger::inFrameMaps(js::AbstractFramePtr)::<lambda(js::DebuggerFrame*)> >(js::AbstractFramePtr, js::Debugger::<lambda(js::DebuggerFrame*)>)+68>:	mov    0x20(%rax),%rdi
   0xb022f8 <js::Debugger::forEachDebuggerFrame<js::Debugger::inFrameMaps(js::AbstractFramePtr)::<lambda(js::DebuggerFrame*)> >(js::AbstractFramePtr, js::Debugger::<lambda(js::DebuggerFrame*)>)+72>:	callq  0x5b6130 <JS::Realm::maybeGlobal() const>

Comment 1

[David Durst [:ddurst]]

How important is this?

Comment 2

[Steven DeTar [:sdetar]]

Jandem do you think you could help answer David's question about how important this is?

Comment 3

[Jan de Mooij [:jandem]]

decoder, can we get JSBugmon to bisect this?

Comment 4

[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/66ee3a9fe9ca
user:        Nicolas B. Pierron
date:        Mon May 28 15:41:23 2018 +0000
summary:     Bug 1418971 - Remove rematerialized frames after bailouts and exceptions. r=jandem

This iteration took 286.958 seconds to run.

Comment 5

[Jason Orendorff [:jorendorff]]

Slightly simplified:

var dbgGlobal = newGlobal();
var dbg = new dbgGlobal.Debugger(this);
function f1() {
    dbg.getNewestFrame().older;
    throw new Error();
}
function f2() { f1(); }
function f3() { f2(); }
stackTest(f3);

----

The code in HandleExceptionIon is like this:

        if (shouldBail) {
            ...
            ExceptionBailoutInfo propagateInfo;
            uint32_t retval = ExceptionHandlerBailout(cx, frame, rfe, propagateInfo, overrecursed);
            if (retval == BAILOUT_RETURN_OK)
                return;
        }

        MOZ_ASSERT_IF(rematFrame, !Debugger::inFrameMaps(rematFrame));

The call to ExceptionHandlerBailout calls BailoutIonToBaseline, which destroys rematFrame from this ScopeExit:

    // Ion bailout can fail due to overrecursion and OOM. In such cases we
    // cannot honor any further Debugger hooks on the frame, and need to
    // ensure that its Debugger.Frame entry is cleaned up.
    auto guardRemoveRematerializedFramesFromDebugger = mozilla::MakeScopeExit([&] {
        activation->removeRematerializedFramesFromDebugger(cx, iter.fp());
    });

Comment 6

[Fuzzing Team]

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

Comment 7

[Gary Kwong [:gkw] [:nth10sd]]

autobisectjs shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9697472e6ab7
user:        Jan de Mooij
date:        Tue Oct 23 01:18:22 2018 +0000
summary:     Bug 1493627 part 2 - Unify OOM testing state variables for different kinds to make AutoEnterOOMUnsafeRegion work for stack checks. r=jonco

Not sure if this may be related, but is bug 1493627 a likely fix?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #7)
> Not sure if this may be related, but is bug 1493627 a likely fix?

Ugh, it's not a fix but it hides the bug. This code uses AutoEnterOOMUnsafeRegion and that now also works for stack-"OOM" testing:

https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/js/src/jit/Bailouts.cpp#217-221

I'll post a patch to clean up the bailout code.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1479394 - Clean up exception handling in bailout code and remove a MOZ_ASSERT_IF that triggered a UAF. r?nbp

Depends on D12103

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b81f4153e1a8
Clean up exception handling in bailout code and remove a MOZ_ASSERT_IF that triggered a UAF. r=nbp

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b81f4153e1a8