Closed Bug 1325450 Opened 3 years ago Closed 3 years ago
Assertion failure: !minimal
Bundle(bundle), at js/src/jit/Backtracking Allocator .cpp:1320
The attached binary WebAssembly testcase crashes on mozilla-inbound revision 5a74fd7f6ddd+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2 --without-intl-api --enable-debug --target=i686-pc-linux-gnu). To reproduce, you can run the following code in the JS shell (running with --wasm-always-baseline might be necessary): var data = os.file.readFile(file, 'binary'); new WebAssembly.Instance(new WebAssembly.Module(data.buffer)); Backtrace: ==3985==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x0cb99a70 bp 0xffdd5378 sp 0xffdd5200 T0) #0 0xcb99a6f in js::jit::BacktrackingAllocator::processBundle(js::jit::MIRGenerator*, js::jit::LiveBundle*) js/src/jit/BacktrackingAllocator.cpp:1320:9 #1 0xcb91653 in js::jit::BacktrackingAllocator::go() js/src/jit/BacktrackingAllocator.cpp:837:14 #2 0x973bb85 in js::jit::GenerateLIR(js::jit::MIRGenerator*) js/src/jit/Ion.cpp:1956:18 #3 0x8b189db in js::wasm::IonCompileFunction(js::wasm::CompileTask*) js/src/wasm/WasmIonCompile.cpp:3769:25 #4 0x8a4f59c in js::wasm::CompileFunction(js::wasm::CompileTask*) js/src/wasm/WasmGenerator.cpp:1148:16 #5 0x8a4ec52 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/wasm/WasmGenerator.cpp:928:14 #6 0x89f9fba in DecodeFunctionBody(js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/wasm/WasmCompile.cpp:60:12 #7 0x89f9fba in DecodeCodeSection(js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/wasm/WasmCompile.cpp:88 #8 0x89f9fba in js::wasm::Compile(js::wasm::ShareableBytes const&, js::wasm::CompileArgs const&, mozilla::UniquePtr<char , JS::FreePolicy>*) js/src/wasm/WasmCompile.cpp:124 #9 0x8bf8a87 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/wasm/WasmJS.cpp:390:27 #10 0x828b14a in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5638:14 #11 0xbc0ecd3 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:239:15 [...] #25 0x80b130a in _start (/home/ubuntu/build/build/js+0x80b130a) Marking s-s because this is a backtracking allocator assertion and these can possibly be s-s.
Is this auto-bisect-able? What'll be a little annoying is if the bug goes back before 0xd then the binary will be invalid... Also not sure if not having a minimalBundle() is s-s, it might just be some assertion of optimality. Do you know Jan?
(In reply to Luke Wagner [:luke] from comment #2) > Also not sure if not having a minimalBundle() is s-s, it might just be some > assertion of optimality. Do you know Jan? This assert is bad AFAIK, not just a potential performance issue. In other bugs we have also seen opt builds misbehave.
I'm unable to reproduce this on x86. Benjamin said he would try as well.
Flags: needinfo?(jdemooij) → needinfo?(bbouvier)
Can repro on x86 only (no x64) with the following module: new WebAssembly.Module(wasmTextToBinary(` (module (func $func0 (param $var0 i64) f32.const 8.322591839317954e-41 i64.trunc_s/f32 get_local $var0 get_local $var0 get_local $var0 get_local $var0 i64.add tee_local $var0 i64.add get_local $var0 i64.sub i64.sub get_local $var0 i64.add i64.xor call $func0 ) ) `))
Here's the IONFLAGS=regalloc log, in case it helps. I don't understand what's going here, by looking at the logs.
I don't think the fix in bug 1315596 was complete; looking at the logs, the bundles the regalloc is evicting won't let it immediately allocate because there are additional overlapping bundles using the same register. The SubI64 here has two at-start register uses, two normal register uses, and two register definitions, and when allocating a minimal bundle for one of the normal uses it evicts the bundles for both at-start uses but hasn't gotten around to evicting the bundles for the definitions when it runs out of attempts. Setting MAX_ATTEMPTS to 3 should fix this, but it seems more robust to just let minimal bundles evict as many other bundles as they need to in order to find a compatible register. Benjamin, does this fix the crash you're seeing? (I haven't been able to reproduce this.) I also don't know what bad behavior this assertion can cause other than ilooping in the register allocator, but I guess the bundle splitting code could behave incorrectly when operating on minimal bundles.
Assignee: nobody → bhackett1024
Attachment #8824521 - Flags: review?(sunfish)
Yes, this fixes the crash I am seeing, thank you. (fwiw, comment 5 shows a 100% reproducible test case which asserts **on 32-bits x86**)
[Tracking Requested - why for this release]: Since we have WebAssembly since FF52, which introduced SubI64
Pinging Brian to get this landed and uplifted to aurora.
Comment on attachment 8824521 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? wasm-only it seems (wasm exposed a bug in the register allocator which could not previously be triggered) Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial. How likely is this patch to cause regressions; how much testing does it need? None. Approval Request Comment [Feature/Bug causing the regression]: needed for wasm [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [Is the change risky?]: no [Why is the change risky/not risky?]: simple regalloc change
Comment on attachment 8824521 [details] [diff] [review] patch Sec-approval+ for trunk and Aurora approval given.
I'll just mark it sec-high because it sounds like maybe something bad could happen...
You need to log in before you can comment on or make changes to this bug.