Crash [@ js::wasm::Code::tiers] due to over-recursion with evalInCooperativeThread

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: bbouvier)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla58
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57+ fixed, firefox58 fixed)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
The following testcase crashes on mozilla-central revision 8dba4037f395 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

var lfLogBuffer = `
  const USE_ASM = '"use asm";';
  function asmCompile() {
    var f = Function.apply(null, arguments);
    return f;
  }
  function asmLink(f) {
    return f.apply(null, Array.slice(arguments, 1));
  }
  gczeal(9);
  asmLink(asmCompile(USE_ASM + "function asmRec() { asmRec() } return asmRec"))();
`;
evalInCooperativeThread(lfLogBuffer);


Backtrace:

received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf5dffb40 (LWP 30663)]
0x0898c21f in js::wasm::Code::tiers (this=0xf5eb6240) at js/src/wasm/WasmCode.cpp:695
#0  0x0898c21f in js::wasm::Code::tiers (this=0xf5eb6240) at js/src/wasm/WasmCode.cpp:695
#1  0x0898c8c4 in js::wasm::Code::lookupCallSite (this=0xf5eb6240, returnAddress=0x53bd3029) at js/src/wasm/WasmCode.cpp:802
#2  0x089b67af in js::wasm::WasmFrameIter::popFrame (this=0xf5cfedac) at js/src/wasm/WasmFrameIter.cpp:135
#3  0x089b6b9c in js::wasm::WasmFrameIter::operator++ (this=0xf5cfedac) at js/src/wasm/WasmFrameIter.cpp:102
#4  0x08853a7c in js::JitFrameIter::operator++ (this=0xf5cfeda8) at js/src/vm/Stack.cpp:592
#5  0x0825f3c1 in js::OnlyJSJitFrameIter::operator++ (this=0xf5cfeda8) at js/src/vm/Stack.h:1833
#6  js::OnlyJSJitFrameIter::settle (this=0xf5cfeda8) at js/src/vm/Stack.h:1824
#7  0x0825f457 in js::OnlyJSJitFrameIter::operator++ (this=0xf5cfeda8) at js/src/vm/Stack.h:1834
#8  js::OnlyJSJitFrameIter::settle (this=0xf5cfeda8) at js/src/vm/Stack.h:1824
[...]
#125 js::OnlyJSJitFrameIter::operator++ (this=0xf5cfeda8) at js/src/vm/Stack.h:1834
#126 js::OnlyJSJitFrameIter::settle (this=0xf5cfeda8) at js/src/vm/Stack.h:1824
#127 0x0825f457 in js::OnlyJSJitFrameIter::operator++ (this=0xf5cfeda8) at js/src/vm/Stack.h:1834
eax	0x0	0
ebx	0xf5eb6244	-169123260
ecx	0x0	0
edx	0x0	0
esi	0xf5ce0080	-171048832
edi	0x8da9ff4	148545524
ebp	0xf5ce0038	4123918392
esp	0xf5ce0000	4123918336
eip	0x898c21f <js::wasm::Code::tiers() const+47>
=> 0x898c21f <js::wasm::Code::tiers() const+47>:	call   0x89a7f80 <mozilla::UniquePtr<js::wasm::CodeSegment const, JS::DeletePolicy<js::wasm::CodeSegment const> >::operator->() const>
   0x898c224 <js::wasm::Code::tiers() const+52>:	mov    0x4(%eax),%eax


This is a crash due to stack space exhaustion.
Luke can you have someone look at this? thanks
Flags: needinfo?(luke)
Priority: -- → P2
Looks like accidental recursion from bug 1360211?
Flags: needinfo?(luke) → needinfo?(bbouvier)
Assignee

Comment 3

2 years ago
[Tracking Requested - why for this release]: wasm regression from bug 1384683, which is in 57.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Depends on: 1384683
Flags: needinfo?(bbouvier)
Assignee

Updated

2 years ago
Blocks: 1407290
Assignee

Comment 4

2 years ago
Duh. Scary no tests were trying to use OnlyJSJitFrameIter with wasm on the stack...
Attachment #8917401 - Flags: review?(luke)
Assignee

Comment 5

2 years ago
Comment on attachment 8917401 [details] [diff] [review]
onlyjsiter.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1384683
[User impact if declined]: crashes during GCs with wasm on the stack
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet checked in
[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?]: trivial one-liner
[String changes made/needed]: n/a
Attachment #8917401 - Flags: approval-mozilla-beta?

Updated

2 years ago
Attachment #8917401 - Flags: review?(luke) → review+

Comment 6

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34557043bd33
Use JitFrameIter::operator++ in OnlyJSJitFrameIter::settle; r=luke

Comment 7

2 years ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebcf1bc3e264
use quit() instead of top-level return in test to fix bustage on a CLOSED TREE; r=me

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34557043bd33
https://hg.mozilla.org/mozilla-central/rev/ebcf1bc3e264
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment on attachment 8917401 [details] [diff] [review]
onlyjsiter.patch

Crash fix, Beta57+
Attachment #8917401 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: not yet checked in
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Benjamin's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.