Closed Bug 1259903 Opened 8 years ago Closed 8 years ago

[wasm] Assertion failure: expected != AnyType, at js/src/asmjs/Wasm.cpp:150

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 1 obsolete file)

The attached binary WebAssembly testcase crashes on mozilla-inbound revision 0fad49a543ea+ (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:

var data = os.file.readFile(file, 'binary');
Wasm.instantiateModule(new Uint8Array(data.buffer));



Backtrace:

==24396==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x082d19e8 bp 0xffdd1f98 sp 0xffdd1be0 T0)
==24396==The signal is caused by a WRITE memory access.
==24396==Hint: address points to the zero page.
    #0 0x82d19e7 in CheckType(FunctionDecoder&, js::wasm::ExprType, js::wasm::ExprType) js/src/asmjs/Wasm.cpp:150:5
    #1 0x82d19e7 in DecodeSetLocal(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:349
    #2 0x82d19e7 in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:673
    #3 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #4 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #5 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #6 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #7 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #8 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #9 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #10 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #11 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #12 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #13 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #14 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #15 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #16 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #17 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #18 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #19 0x82cc15c in DecodeReturn(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:633:14
    #20 0x82cc15c in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:894
    #21 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #22 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #23 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #24 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #25 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #26 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #27 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #28 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #29 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #30 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #31 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #32 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #33 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #34 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710
    #35 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
[...]
    #501 0x82c92ff in DecodeUnaryOperator(FunctionDecoder&, js::wasm::ValType, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:386:10
    #502 0x82c92ff in DecodeExpr(FunctionDecoder&, js::wasm::ExprType*) js/src/asmjs/Wasm.cpp:710

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/asmjs/Wasm.cpp:150:5 in CheckType(FunctionDecoder&, js::wasm::ExprType, js::wasm::ExprType)
==24396==ABORTING
Attached file Testcase
Reduced test case:

(module (func (select (return) (i32.const 0) (i32.const 1))))

Luke, Dan, how should "select" behave if one operand is of type AnyType?
- We *could* unify the left type with the right type if any of them is AnyType (or void, if any of the two types is void).  But in this case using "select" isn't useful at all: (select A B cond) would be equivalent to (A) (B) (cond) in this case! Which means the select itself would be a no-op.
- We could forbid this when validating: the compiler or code writer should have seen that A or B are Any or Void and they should have **not** used select at this place.
Flags: needinfo?(sunfish)
Flags: needinfo?(luke)
Another instance was found by fuzzers, with a test case that reduces to the following:

(module (func (select (loop) (i32.const 0) (i32.const 0))))

The ifTrue part is an empty loop which return type is Void. The CheckType check in DecodeSelectOperator works only because trueType is passed as the second argument... So it actually seems we have two issues here:

- AnyType can show up at any place
- The position of the void subexpr shouldn't have an impact on validation (swap the (loop) and (i32.const) operands in the above expression and it should fail as expected, instead of asserting).
AnyType means that control flow can't possibly return to that operator (and thus this will be inDeadCode() in WasmIonCompile).  I think the right thing to do here is to remove the CheckType entirely and have:
  *type = Unify(trueType, falseType);
which is symmetric to what we do for branches.  This change will allow these weird cases to validate:
  (module (func (select (i32.const 1) (f32.const 1) (i32.const 1))))
  (module (func (i32.add (i32.const 0) (select (return) (i32.const 1) (i32.const 0)))))
which both validate in ml-proto currently.

The latter case is NBD for WasmIonCompile since it's dead code.  The former case will likely cause problems for WasmIonCompile since it's not dead code; just the result is unused.  I think the right thing to do would be to check the MIRType of the result and if they mismatch, don't create the select, just set *def = null.
Flags: needinfo?(luke)
Is there any advantage to allowing obviously invalid code just because it's unreachable? In my current postorder patch, this will take extra work to accept.
Flags: needinfo?(sunfish)
Attached patch unify.select.patch (obsolete) — Splinter Review
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8735868 - Flags: review?(luke)
Updated patch to take into account the fact that both true/false exprs can have MIRType_None too (that was bug 1260236).
Attachment #8735868 - Attachment is obsolete: true
Attachment #8735868 - Flags: review?(luke)
Attachment #8735877 - Flags: review?(luke)
Comment on attachment 8735877 [details] [diff] [review]
unify.select.patch

Review of attachment 8735877 [details] [diff] [review]:
-----------------------------------------------------------------

Exactly!

::: js/src/asmjs/WasmIonCompile.cpp
@@ +2188,5 @@
>          return false;
>  
> +    if (trueExpr && falseExpr &&
> +        trueExpr->type() == falseExpr->type() &&
> +        trueExpr->type() != MIRType_None) {

{ on new line
Attachment #8735877 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/2d7bdc3491d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.