2010763 - Assertion failure in MDefinition::setWasmRefType during ValueNumbering::ReplaceAllUsesWith

Status: RESOLVED FIXED

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

Description

[amolnaik4]

Attachment: poc.js

Steps to reproduce:

I have found a crash in SpiderMonkey's IonMonkey JIT compiler affecting WebAssembly Reference Types. The crash occurs in a Debug build during the Value Numbering optimization pass.

version: JavaScript-C149.0a1

Steps to reproduce:

1. Download attached poc.js
2. run /path/to/your/js poc.js

Actual results:

JS engine crashes. Please find the crashlog:
[4171376] Assertion failure: !(wasmRefType_.isSome() && refType.isNothing()), at /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/MIR.h:760
#01: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a685dd]
#02: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a00f8a]
#03: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a0038d]
#04: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a024f5]
#05: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a0283a]
#06: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a02e5f]
#07: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2a04524]
#08: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x2f93558]
#09: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x39c44ba]
#10: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x398540f]
#11: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x3987667]
#12: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x393e817]
#13: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x394cecf]
#14: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x3637fa6]
#15: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x366a3fd]
#16: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x366a018]
#17: ???[/home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/bin/js +0x36787a4]
#18: ???[/lib/x86_64-linux-gnu/libc.so.6 +0x9caa4]
#19: ???[/lib/x86_64-linux-gnu/libc.so.6 +0x129c6c]
#20: ??? (???:???)
UndefinedBehaviorSanitizer:DEADLYSIGNAL
==4171376==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x59d7f9b9d5f5 bp 0x7e6f5c9f8720 sp 0x7e6f5c9f8700 T4171378)
==4171376==The signal is caused by a WRITE memory access.
==4171376==Hint: address points to the zero page.
executionHash is 0x1 with 0 inputs
#0 0x59d7f9b9d5f5 in MOZ_CrashSequence(void*, long) /home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/include/mozilla/Assertions.h:237:3
#1 0x59d7f9b9d5f5 in js::jit::MDefinition::setWasmRefType(js::wasm::MaybeRefType) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/MIR.h:760:5
#2 0x59d7f9b35f89 in ReplaceAllUsesWith(js::jit::MDefinition*, js::jit::MDefinition*) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/ValueNumbering.cpp:164:7
#3 0x59d7f9b3538c in js::jit::ValueNumberer::visitDefinition(js::jit::MDefinition*) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/ValueNumbering.cpp:838:5
#4 0x59d7f9b374f4 in js::jit::ValueNumberer::visitBlock(js::jit::MBasicBlock*) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/ValueNumbering.cpp:1075:10
#5 0x59d7f9b37839 in js::jit::ValueNumberer::visitDominatorTree(js::jit::MBasicBlock*) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/ValueNumbering.cpp:1129:12
#6 0x59d7f9b37e5e in js::jit::ValueNumberer::visitGraph() /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/ValueNumbering.cpp:1168:12
#7 0x59d7f9b39523 in js::jit::ValueNumberer::run(js::jit::ValueNumberer::UpdateAliasAnalysisFlag) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/ValueNumbering.cpp:1336:10
#8 0x59d7fa0c8557 in js::jit::OptimizeMIR(js::jit::MIRGenerator*) /home/developer/fuzz_sm/sm_149/firefox/js/src/jit/Ion.cpp:1219:14
#9 0x59d7faaf94b9 in js::wasm::IonCompileFunctions(js::wasm::CodeMetadata const&, js::wasm::CodeTailMetadata const*, js::wasm::CompilerEnvironment const&, js::LifoAlloc&, mozilla::Vector<js::wasm::FuncCompileInput, 8ul, js::SystemAllocPolicy> const&, js::wasm::CompiledCode*, std::unique_ptr<char [], JS::FreePolicy>) /home/developer/fuzz_sm/sm_149/firefox/js/src/wasm/WasmIonCompile.cpp:10798:12
#10 0x59d7faaba40e in ExecuteCompileTask(js::wasm::CompileTask, std::unique_ptr<char [], JS::FreePolicy>) /home/developer/fuzz_sm/sm_149/firefox/js/src/wasm/WasmGenerator.cpp:613:12
#11 0x59d7faabc666 in js::wasm::ModuleGenerator::locallyCompileCurrentTask() /home/developer/fuzz_sm/sm_149/firefox/js/src/wasm/WasmGenerator.cpp:712:8
#12 0x59d7faabc666 in js::wasm::ModuleGenerator::finishFuncDefs() /home/developer/fuzz_sm/sm_149/firefox/js/src/wasm/WasmGenerator.cpp:849:24
#13 0x59d7faa73816 in js::wasm::CompilePartialTier2(js::wasm::Code const&, unsigned int, std::unique_ptr<char [], JS::FreePolicy>, mozilla::Vector<std::unique_ptr<char [], JS::FreePolicy>, 0ul, js::SystemAllocPolicy>, mozilla::Atomic<bool, (mozilla::MemoryOrdering)2, void>) /home/developer/fuzz_sm/sm_149/firefox/js/src/wasm/WasmCompile.cpp:1088:13
#14 0x59d7faa81ece in js::wasm::Module::PartialTier2CompileTaskImpl::runHelperThreadTask(js::AutoLockHelperThreadState&) /home/developer/fuzz_sm/sm_149/firefox/js/src/wasm/WasmCode.cpp:657:22
#15 0x59d7fa76cfa5 in js::GlobalHelperThreadState::runTaskLocked(JS::HelperThreadTask*, js::AutoLockHelperThreadState&) /home/developer/fuzz_sm/sm_149/firefox/js/src/vm/HelperThreads.cpp:667:11
#16 0x59d7fa79f3fc in js::GlobalHelperThreadState::runOneTask(JS::HelperThreadTask*, js::AutoLockHelperThreadState&) /home/developer/fuzz_sm/sm_149/firefox/js/src/vm/HelperThreads.cpp:623:3
#17 0x59d7fa79f3fc in js::HelperThread::threadLoop(js::InternalThreadPool*) /home/developer/fuzz_sm/sm_149/firefox/js/src/vm/InternalThreadPool.cpp:324:25
#18 0x59d7fa79f017 in js::HelperThread::ThreadMain(js::InternalThreadPool*, js::HelperThread*) /home/developer/fuzz_sm/sm_149/firefox/js/src/vm/InternalThreadPool.cpp:251:11
#19 0x59d7fa7ad7a3 in void js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::callMain<0ul, 1ul>(std::integer_sequence<unsigned long, 0ul, 1ul>) /home/developer/fuzz_sm/sm_149/firefox/js/src/threading/Thread.h:226:5
#20 0x59d7fa7ad7a3 in js::detail::ThreadTrampoline<void (&)(js::InternalThreadPool*, js::HelperThread*), js::InternalThreadPool*&, js::HelperThread*>::Start(void*) /home/developer/fuzz_sm/sm_149/firefox/js/src/threading/Thread.h:215:11
#21 0x7e6f5dc9caa3 (/lib/x86_64-linux-gnu/libc.so.6+0x9caa3) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
#22 0x7e6f5dd29c6b (/lib/x86_64-linux-gnu/libc.so.6+0x129c6b) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)

==4171376==Register values:
rax = 0x0000000000000000 rbx = 0x00000000000001fe rcx = 0x00000000000002f8 rdx = 0x00007e6f5de04563
rdi = 0x00007e6f5de05700 rsi = 0x0000000000000000 rbp = 0x00007e6f5c9f8720 rsp = 0x00007e6f5c9f8700
r8 = 0x0000000000000000 r9 = 0x0000000000000003 r10 = 0x0000000000000002 r11 = 0x0000000000000293
r12 = 0x0000000000000010 r13 = 0x0000000000000057 r14 = 0x00007e6f5b1385a8 r15 = 0x01fffffffffffe00
UndefinedBehaviorSanitizer can not provide additional info.
SUMMARY: UndefinedBehaviorSanitizer: SEGV /home/developer/fuzz_sm/sm_149/firefox/obj-fuzzbuild/dist/include/mozilla/Assertions.h:237:3 in MOZ_CrashSequence(void*, long)
==4171376==ABORTING

Comment 1

[Ryan Hunt [:rhunt]]

Ben this is probably related to the patches you landed yesterday.

Comment 2

[Ryan Hunt [:rhunt]]

Bug 2000677 is probably related, not setting the regression flag until confirmed.

Comment 3

[Andrew McCreight [:mccr8]]

It looks like MaybeRefType uses release asserts, so at least there's no danger of memory safety issues directly on that.

Comment 4

[Ben Visness [:bvisness]]

Attachment: Bug 2010763: Update how Nothing is handled in GLB. r=rhunt

Our GLB logic for MaybeRefType was not quite right. Conceptually, it
makes sense to treat Nothing as the topmost type, above
anyref/funcref/etc., as a MaybeRefType of Nothing means it could be
literally anything. This implies that the GLB of Nothing and any ref
type should be that ref type, not Nothing.

Comment 5

[Ben Visness [:bvisness]]

The bug here is that an MPhi with no wasm ref type is being folded to its only input, an MWasmNullConstant with ref type nullexternref. When we do this, we set the ref type of the MWasmNullConstant to the GLB of the two nodes' ref types. Because the Phi has Nothing, the GLB comes back Nothing, and we trip a sanity assert that ensures that, over the course of Ion optimization, the wasm ref types we track on MIR nodes only ever narrow, and never widen.

This is a general best-practice kind of thing, since it gives us assurance that iterative processes will converge and that we never make a type-based decision that turns out to be invalidated later. The thing is, this assert didn't even exist until the most recent change. Our GVN logic used to use the LUB to combine nodes, but in bug 2000677 we updated it to use the GLB instead, as this is more precise. But in the one case triggered by the bug, the GLB behaved like a LUB instead. I don't believe this is incorrect, it's just imprecise.

Furthermore, Nothing is essentially a poison pill for all type-based optimizations. It's conceptually the topmost type in the entire wasm type lattice, above anyref/funcref/etc. It means we know literally nothing about the type. In the case triggered by the bug, we could in theory make a type-based optimization based on the original nullexternref value, for example folding a ref.is_null to a constant 1. But erasing the wasm ref type from the MWasmNullConstant in that case does not invalidate the optimization.

So I believe this is a harmless case, but nonetheless useful to find for our own sanity. Because these type issues can be subtle, I think I'll get a second opinion on Monday, but then we can likely declassify this.

Comment 7

[Ryan Hunt [:rhunt]]

Yes, I agree with Ben that this should be safe and shouldn't lead to miscompiles. We will end up with some type information on MIR being more conservative than we'd like, but that won't lead to incorrect decisions. The assert is primarily to ensure type information converges. This particular folding of a phi pointing at a single null is correct.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/91c117795deb
https://hg.mozilla.org/integration/autoland/rev/7306bba46e29
Update how Nothing is handled in GLB. r=rhunt

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/7306bba46e29