Closed Bug 1535848 Opened 6 years ago Closed 6 years ago

Crash [@ js::jit::MacroAssembler::patchCall] or Assertion failure: vixl::is_int26(relTarget00), at js/src/jit/arm64/MacroAssembler-arm64.cpp:672

Categories

(Core :: JavaScript: WebAssembly, defect, P2)

ARM64
All
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: gkw, Assigned: lth)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:][post-critsmash-triage][adv-main68+])

Crash Data

Attachments

(3 files, 1 obsolete file)

Attached file stack (deleted) —

The following testcase crashes on mozilla-central revision 124ee436c421 (build with --enable-debug --enable-more-deterministic --enable-simulator=arm64, run with --fuzzing-safe --no-threads --no-baseline --no-ion w280-out.wrapper w280-out.wasm):

See attachment.

Backtrace:

#0 js::jit::MacroAssembler::patchCall (this=<optimized out>, callerOffset=<optimized out>, calleeOffset=<optimized out>)
at /home/ubuntu/trees/mozilla-central/js/src/jit/arm64/MacroAssembler-arm64.cpp:672
#1 0x00005555573e1ec1 in js::wasm::ModuleGenerator::linkCallSites (this=0x7fffffffa830)
at /home/ubuntu/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:519
#2 0x00005555573e3556 in js::wasm::ModuleGenerator::finishCodegen (this=0x7fffffffa830)
at /home/ubuntu/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:895
#3 0x00005555573e4310 in js::wasm::ModuleGenerator::finishCodeTier (this=0x7fffffffa830)
at /home/ubuntu/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:1013
#4 0x00005555573e4e78 in js::wasm::ModuleGenerator::finishModule (this=0x7fffffffa830, bytecode=..., maybeTier2Listener=0x0)
at /home/ubuntu/trees/mozilla-central/js/src/wasm/WasmGenerator.cpp:1100
#5 0x00005555573699c0 in js::wasm::CompileBuffer (args=..., bytecode=..., error=0x7fffffffbd40, warnings=<optimized out>, listener=0x0)
at /home/ubuntu/trees/mozilla-central/js/src/wasm/WasmCompile.cpp:578
/snip

For detailed crash information, see attachment.

This also crashes js opt shell ARM64 [@ js::jit::MacroAssembler::patchCall]. Not sure if this is wasm or ARM64-related but setting s-s for now and needinfo? from Lars as a start.

Flags: needinfo?(lhansen)

Due to skipped revisions, the first bad revision could be any of:
changeset: https://hg.mozilla.org/mozilla-central/rev/5014a6b01aea
user: Lars T Hansen
date: Tue Nov 28 10:05:36 2017 +0100
summary: Bug 1439404 - Wasm on ARM64: gating. r=bbouvier

changeset: https://hg.mozilla.org/mozilla-central/rev/56d7b668df45
user: Lars T Hansen
date: Tue Mar 06 16:29:42 2018 +0100
summary: Bug 1439404 - ARM64 simulator bugfix, add missing guard. r=bbouvier

changeset: https://hg.mozilla.org/mozilla-central/rev/27bc0a49fe15
user: Lars T Hansen
date: Fri Nov 17 13:17:29 2017 +0100
summary: Bug 1439404 - Wasm baseline, support the chunky ARM64 stack. r=bbouvier

changeset: https://hg.mozilla.org/mozilla-central/rev/32fdbeda9305
user: Lars T Hansen
date: Mon Nov 27 09:07:04 2017 +0100
summary: Bug 1439404 - Wasm baseline, fill in the porting APIs for ARM64. r=bbouvier

Lars, is bug 1439404 a likely regressor?

Blocks: 1439404

Gary, test case?

It looks like this is another one of those "code is too large for architecture" problems, we're seeing quite a few of these now because we're looking and we haven't been looking before (and Wasm makes this problem much worse than JS, since we compile entire modules into single blobs of code and don't always handle the limitations correctly).

Flags: needinfo?(lhansen) → needinfo?(nth10sd)
Blocks: 1536036
Attached file tests_5ef988033e6af184967502f1226dd6736d9b0450.zip (obsolete) (deleted) —

Oops, sorry for missing this out.

Flags: needinfo?(nth10sd) → needinfo?(lhansen)

Easy to repro here.

Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Flags: needinfo?(lhansen)
Priority: -- → P2

Architecture-arm64.h:

// TODO:
// This constant needs to be updated to account for whatever near/far branching
// strategy is used by ARM64.
static const uint32_t JumpImmediateRange = UINT32_MAX;

/me greps for TODO in jit/arm64 and finds 46 instances, this is the first one, others are actionable.

/me greps for FIXME in jit/arm64 and finds 15 instances, a few are actionable.

There are comments in the code suggesting that we've made plans at some point to handle very far jumps via patching + indirect jumps, but all of those comments are TODO/FIXME. Absent such a strategy, the furthest jump is 2^27-1 bytes, and we need to define JumpImmediateRange to reflect that.

This is a MOZ_RELEASE_ASSERT and hence this is not really a security problem, but the cluster of problems the bug belongs to is mildly security-sensitive, so keeping the s-s setting and setting sec-low.

Keywords: sec-low
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Is this something we should consider for Beta uplift or can it just ride the trains?

Flags: needinfo?(lhansen)
Flags: in-testsuite?

I don't think we should uplift, for a couple of reasons. First, we're being stopped from doing anything bad here by a MOZ_RELEASE_ASSERT and that's fine; the test case is synthetic and there's no exploit path, nor any real-world hardship at this point. Second, this is part of a cluster of patches and I'm not sure yet what else we'd need to uplift. I think if any of the other patches demand it we could uplift this, but it's more likely that other patches will make this patch even less critial.

Flags: needinfo?(lhansen)
Flags: qe-verify-
Whiteboard: [jsbugmon:] → [jsbugmon:][post-critsmash-triage]
Whiteboard: [jsbugmon:][post-critsmash-triage] → [jsbugmon:][post-critsmash-triage][adv-main68+]
Attached file 280.tar.xz (deleted) —

Compile with --enable-debug --enable-more-deterministic --enable-simulator=arm64 and run with --fuzzing-safe --no-threads --no-baseline --no-ion w280-out.wrapper w280-out.wasm on m-c rev 124ee436c421:

~/js-dbg-64-dm-armsim64-linux-x86_64-124ee436c421 --fuzzing-safe --no-threads --no-baseline --no-ion w280-out.wrapper w280-out.wasm
Assertion failure: vixl::is_int26(relTarget00), at /home/fuzz3/trees/mozilla-central/js/src/jit/arm64/MacroAssembler-arm64.cpp:672
Segmentation fault

Reduced using wasm-reduce.

Attachment #9051733 - Attachment is obsolete: true

Unable to repro the test case in comment 12.

(In reply to Lars T Hansen [:lth] from comment #13)

Unable to repro the test case in comment 12.

No action needed, m-c rev 124ee436c421 did not contain this fix, as I just wanted to practise getting a smaller wasm testcase off the large one in comment 3.

If you wanted to try and reproduce the problem, you'd have to try m-c rev 124ee436c421 and not tip (which contains the fix via rev c13b55d67ea2).

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: