Closed
Bug 1479394
Opened 7 years ago
Closed 7 years ago
Crash [@ js::AbstractFramePtr::global] with Debugger and stackTest
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla65
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox63 | --- | wontfix |
firefox64 | --- | wontfix |
firefox65 | --- | fixed |
People
(Reporter: decoder, Assigned: jandem)
References
Details
(4 keywords, Whiteboard: [jsbugmon:update,ignore])
Crash Data
Attachments
(1 file)
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 2•7 years ago
|
||
Jandem do you think you could help answer David's question about how important this is?
Flags: needinfo?(sdetar) → needinfo?(jdemooij)
Assignee | ||
Comment 3•7 years ago
|
||
decoder, can we get JSBugmon to bisect this?
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(choller)
Updated•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 4•7 years ago
|
||
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.
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(choller)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
Comment 5•7 years ago
|
||
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());
});
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Comment 6•7 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision cbeaa2d94304).
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?
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 9•7 years ago
|
||
Depends on D12103
Updated•7 years ago
|
Comment 10•7 years ago
|
||
Pushed by jdemooij@mozilla.com:
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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Updated•7 years ago
|
status-firefox-esr60:
--- → unaffected
Updated•7 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•