Closed Bug 1325450 Opened 3 years ago Closed 3 years ago

Assertion failure: !minimalBundle(bundle), at js/src/jit/BacktrackingAllocator.cpp:1320

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + fixed
firefox53 + fixed

People

(Reporter: decoder, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [post-critsmash-triage])

Attachments

(3 files)

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.
Attached file Testcase
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?
Flags: needinfo?(jdemooij)
(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
  )
)
`))
Attached file regalloc.log
Here's the IONFLAGS=regalloc log, in case it helps. I don't understand what's going here, by looking at the logs.
Flags: needinfo?(jdemooij)
Flags: needinfo?(bhackett1024)
Flags: needinfo?(bbouvier)
Attached patch patchSplinter Review
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
Flags: needinfo?(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**)
Flags: needinfo?(jdemooij)
[Tracking Requested - why for this release]:
Since we have WebAssembly since FF52, which introduced SubI64
Priority: -- → P1
Tracking this wasm issue for 52/53
Attachment #8824521 - Flags: review?(sunfish) → review+
Pinging Brian to get this landed and uplifted to aurora.
Flags: needinfo?(bhackett1024)
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
Attachment #8824521 - Flags: sec-approval?
Attachment #8824521 - Flags: approval-mozilla-aurora?
Comment on attachment 8824521 [details] [diff] [review]
patch

Sec-approval+ for trunk and Aurora approval given.
Attachment #8824521 - Flags: sec-approval?
Attachment #8824521 - Flags: sec-approval+
Attachment #8824521 - Flags: approval-mozilla-aurora?
Attachment #8824521 - Flags: approval-mozilla-aurora+
I'll just mark it sec-high because it sounds like maybe something bad could happen...
Keywords: sec-high
https://hg.mozilla.org/mozilla-central/rev/43400d03b8df
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Group: javascript-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Group: core-security-release
Duplicate of this bug: 1315596
You need to log in before you can comment on or make changes to this bug.