1518331 - Assertion failure: false (offset.isValid()), at js/src/jit/shared/Assembler-shared.h:286

Status: RESOLVED FIXED

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

Description

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

The following testcase crashes on mozilla-central revision aac6849c6ff0 (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion --spectre-mitigations=off w49-out.wrapper w49-out.wasm):

See attachment.

Backtrace:

#0 js::jit::HighWord (address=...) at js/src/jit/shared/Assembler-shared.h:286
#1 0x5709c035 in js::jit::HighWord (address=...) at js/src/jit/x86/Assembler-x86.h:230
#2 js::jit::HighWord (op=...) at js/src/jit/x86/Assembler-x86.h:224
#3 0x570a8326 in js::jit::MacroAssembler::wasmLoadI64 (this=0xffe2cc00, access=..., srcAddr=..., out=...) at js/src/jit/x86/MacroAssembler-x86.cpp:696
#4 0x570be984 in js::jit::CodeGeneratorX86::emitWasmLoad<js::jit::LWasmLoadI64> (this=0xffe2d688, ins=0xf1416fa0) at js/src/jit/x86/CodeGenerator-x86.cpp:258
#5 0x570a86a9 in js::jit::CodeGenerator::visitWasmLoadI64 (this=0xffe2d688, ins=0xf1416fa0) at js/src/jit/x86/CodeGenerator-x86.cpp:266
/snip

For detailed crash information, see attachment.

Seems to be 32-bit only, setting s-s to be safe as we've got macroassembler stuff on the stack and also seems to involve spectre mitigations.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: w49-out.wasm

Comment 3

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

Attachment: 49-wrapper.tar.bz2

Comment 4

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

Attachment: All files for the testcase (alternative archive)

Comment 5

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

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/25900f3b9936
user: Dan Gohman
date: Thu Feb 15 09:56:00 2018 +0200
summary: Bug 1435369: Implement non-trapping float-to-int conversions for WebAssembly r=luke

Dan, is bug 1435369 a likely regressor?

Comment 6

[Dan Gohman [:sunfish]]

From the stack trace it doesn't immediately look like it, since this appears to be a crash while translating a load opcode, however I'll investigate further.

Comment 7

[Dan Gohman [:sunfish]]

The attached wasm file does have saturating float-to-int conversions in it, so before bug 1435369, this wasm file would have just been rejected as invalid wasm.

Comment 8

[Dan Gohman [:sunfish]]

Confirmed that this doesn't look related to bug 1435369. There's an i64.load with an address of i32.const 0x7fffffff, and codegen is attempting to compute an address for the high half of the 64-bit load, and finding that this pushes the offset out of representable range.

Comment 9

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

Not sure who should look at this then. Luke/Lars/Benjamin?

Comment 10

[Dan Gohman [:sunfish]]

Attachment: w49-out.wasm

Here's a version of w49-out.wasm which has all saturating float-to-int conversions replaced by trapping float-to-int conversions. It still reproduces the same assertion failure.

Gary, could you try autobisectjs with this version?

Comment 11

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

It goes back to prior to m-c rev a98f615965d7 then, which was when the --spectre-mitigations=off flag was added. The testcase does not reproduce without the flag.

Not sure if Jan knows what changed to trigger the bug either. (sorry for all the needinfos, I'm not sure who is best to take a look either)

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #11)

which was when the --spectre-mitigations=off flag was added. The testcase does not reproduce without the flag.

Hm I'd expect it to repro on that revision (a98f615965d7) and the parent revision without the flag, because at that point the flag wasn't used anywhere...

Comment 13

[Lars T Hansen [:lth]]

Spectre mitigation is likely a red herring here because it likely {inadvertently,deliberately,occasionally} disables the following optimization and hence masks the problem:

There's an optimization on x86 that allows for a constant base address to be pushed through to code generation without being placed in a register provided the address is zero or the load/store offset is zero, since in this case the add that asserts right now can't assert (one of the operands will be zero). For int64 this is not true since we have an additional offset, which we fail to add here.

See https://searchfox.org/mozilla-central/source/js/src/jit/x86/Lowering-x86.cpp#280.

We can disable the optimization for int64 or we can have an additional check for int64 on the constant value so that we know constant value + 4 does not overflow; ideally we'd abstract this in some way. Does not seem difficult.

I guess in the worst case this bug is actually a bounds check bypass, even if we normally run with spectre mitigations.

Like Jan I'd expect this to repro before spectre mitigations were added, but IIRC the HighWord/LowWord abstraction that catches the problem here was also added sometime during the previous year, and it's possible we didn't have this guard a year ago.

Comment 14

[Lars T Hansen [:lth]]

Attachment: oob-x86.js

Rather smaller test case. Compile with --enable-debug, run with --no-wasm-baseline --spectre-mitigations=off. Not a problem if spectre mitigations are enabled, presumably that forces the pointer into a register.

Comment 15

[Lars T Hansen [:lth]]

Attachment: bug1518331-optimize-constant-access.patch

I think this should be adequate. No comparable code on ARM to worry about that I can see.

No TC included, and anodyne commit msg, until we know security rating.

Comment 16

[Lars T Hansen [:lth]]

Re security classification: What's going on here is that there is an overflow in an address calculation in the assembler when a certain large constant address is used to perform an int64 access. The calculation uses CheckedInt so it asserts in debug builds; in release builds the result of the overflowing calculation is zero. In the case where the result is zero no OOB access actually occurs, though the incorrect cell in the wasm memory may be read or written provided a program can be crafted that makes the optimizer elide the normal explicit bounds check; frankly I doubt this can be done.

And all of this is only with --spectre-mitigations=off, not our default.

As a consequence I would say that this is barely a security issue, certainly not sec-high, so I will go ahead and land. I'll request an uplift to beta because the patch applies as-is but I don't see it as critical.

Comment 17

[Lars T Hansen [:lth]]

Actually, probably not even beta-worthy.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

From what you're saying, I agree that this can just ride the trains.

Also, this was pushed to inbound by lth:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0985217ed619

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/0985217ed619