1655848 - AddressSanitizer: SEGV [@ js::gc::CellWithTenuredGCPointer] or [@ js::gc::detail::GetCellLocation] with ARM64 simulator

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: testcase.tar.xz

AddressSanitizer:DEADLYSIGNAL
=================================================================
==1308158==ERROR: AddressSanitizer: SEGV on unknown address 0x40007fff4000 (pc 0x5602ea595668 bp 0x7ffcd1399aa0 sp 0x7ffcd1399980 T0)
==1308158==The signal is caused by a READ memory access.
    #0 0x5602ea595667 in js::gc::CellWithTenuredGCPointer<js::gc::Cell, js::ObjectGroup>::headerPtr() const /home/mini3lin/trees/mozilla-central/js/src/gc/Cell.h:732:42
    #1 0x5602ea595667 in JSObject::groupRaw() const /home/mini3lin/trees/mozilla-central/js/src/vm/JSObject.h:89:46
    #2 0x5602ea595667 in JSObject::isSingleton() const /home/mini3lin/trees/mozilla-central/js/src/vm/JSObject.h:155:37
    #3 0x5602ea595667 in js::TypeSet::ObjectType(JSObject const*) /home/mini3lin/trees/mozilla-central/js/src/vm/TypeInference-inl.h:134:12
    #4 0x5602ea595667 in js::TypeSet::GetValueType(JS::Value const&) /home/mini3lin/trees/mozilla-central/js/src/vm/TypeInference-inl.h:236:12
    #5 0x5602ea595667 in js::jit::JitScript::MonitorArgType(JSContext*, JSScript*, unsigned int, JS::Value const&) /home/mini3lin/trees/mozilla-central/js/src/vm/TypeInference-inl.h:856:35
    #6 0x5602ea595667 in js::jit::DoTypeMonitorFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICTypeMonitor_Fallback*, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /home/mini3lin/trees/mozilla-central/js/src/jit/BaselineIC.cpp:1276:5
/snip

Run with --fuzzing-safe --no-threads --no-baseline --no-ion --wasm-compiler=cranelift --blinterp --blinterp-warmup-threshold=0, compile with AR=ar sh ./configure --enable-simulator=arm64 --enable-address-sanitizer --disable-jemalloc --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests, tested on m-c rev 2f8d4f1f6608.

Full log coming up. Unsure if this is s-s as this involves cranelift and ARM64 simulator. Please open up if otherwise.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This may be related to decoder's bug 1655695.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: log

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Debug stack

Stack with the following build: AR=ar sh ./configure --enable-simulator=arm64 --enable-debug --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 4

[Chris Fallin [:cfallin]]

Reproduced locally -- I will look into this ASAP. Thanks!

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7c91f246f03f
user:        Lars T Hansen
date:        Mon Jul 27 14:23:18 2020 +0000
summary:     Bug 1649718 - Introduce fuzzing intercession points in feature switches.  r=bbouvier

Probably related to bug 1649718?

Comment 6

[Chris Fallin [:cfallin]]

The true regression point is likely bug 1633721, which introduced reftypes support in Cranelift/aarch64; the above bug just exposes Cranelift to more fuzzing.

Comment 7

[Chris Fallin [:cfallin]]

This turns out to actually be an issue with value width and ABI assumptions about high bits. I managed to reduce the example down to the helper (function 21) that passes an i32 to a JS import after narrowing it from a 64-bit value with i32.wrap_i64. We follow the usual convention of allowing undefined bits above the value's width until they need to be cleared or sign-extended (this works fine for e.g. adds/subtracts and logical ops). So i32.wrap_i64 is a no-op (and of course the i32 result type will force a zero-extension later to clear the high bits if needed). However the SpiderMonkey ABI really does want an i32 to have zeroed high bits in a 64-bit register.

The following patch to Cranelift (then re-vendoring into m-c) fixes this:

diff --git a/cranelift/codegen/src/isa/aarch64/lower_inst.rs b/cranelift/codegen/src/isa/aarch64/lower_inst.rs
index 5fa0ebdc6..d8f11d8d8 100644
--- a/cranelift/codegen/src/isa/aarch64/lower_inst.rs
+++ b/cranelift/codegen/src/isa/aarch64/lower_inst.rs
@@ -1616,7 +1616,12 @@ pub(crate) fn lower_insn_to_regs<C: LowerCtx<I = Inst>>(
             abi.emit_stack_pre_adjust(ctx);
             assert!(inputs.len() == abi.num_args());
             for (i, input) in inputs.iter().enumerate() {
-                let arg_reg = put_input_in_reg(ctx, *input, NarrowValueMode::None);
+                let input_ty = ctx.input_ty(input.insn, input.input);
+                let narrow_mode = match input_ty {
+                    B1 | I8 | B8 | I16 | B16 | I32 | B32 => NarrowValueMode::ZeroExtend64,
+                    _ => NarrowValueMode::None,
+                };
+                let arg_reg = put_input_in_reg(ctx, *input, narrow_mode);
                 abi.emit_copy_reg_to_arg(ctx, i, arg_reg);
             }
             abi.emit_call(ctx);

I'll clean it up and create a PR tomorrow (and probably put the widening logic in the ABI module instead, conditional on the Baldrdash flag).

Comment 8

[Christian Holler (:decoder)]

Cranelift is currently not enabled by default and we don't file Cranelift bugs as s-s yet.

Comment 9

[Chris Fallin [:cfallin]]

Cranelift PR at https://github.com/bytecodealliance/wasmtime/pull/2081. Will post a patch here to update vendored Cranelift once the PR merges.

Comment 10

[Chris Fallin [:cfallin]]

Attachment: Bug 1655848: vendor in new Cranelift to fix fuzzbug. r=jseward

This PR vendors in the latest version of Cranelift, rev
026fb8d388964c7c1bace7019c4fe0d63c584560. This includes a fix for bug
1655848 (from GitHub PR #2081), as well as several other miscellaneous
changes.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0ac26d4e4a5
vendor in new Cranelift to fix fuzzbug. r=jseward

Comment 12

[Natalia Csoregi [:nataliaCs]]

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