1280930 - [wasm] Assertion failure: aNewLength <= mLength, at dist/include/mozilla/Vector.h:1045

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision e9723c6c6136+ (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). 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');
Wasm.instantiateModule(new Uint8Array(data.buffer));



Backtrace:

==374==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000029fc076 bp 0x7ffe899bc410 sp 0x7ffe899bc3c0 T0)
    #0 0x29fc075 in mozilla::Vector<js::wasm::BaseCompiler::Stk, 8ul, js::SystemAllocPolicy>::shrinkTo(unsigned long) /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/64/type/debug/dist/include/mozilla/Vector.h:1045:3
    #1 0x29fc075 in js::wasm::BaseCompiler::popValueStackTo(unsigned int) js/src/asmjs/WasmBaselineCompile.cpp:1566
    #2 0x29e1b35 in js::wasm::BaseCompiler::popValueStackBy(unsigned int) js/src/asmjs/WasmBaselineCompile.cpp:1570:9
    #3 0x29e1b35 in js::wasm::BaseCompiler::emitSelect() js/src/asmjs/WasmBaselineCompile.cpp:5379
    #4 0x29e7c25 in js::wasm::BaseCompiler::emitBody() js/src/asmjs/WasmBaselineCompile.cpp:5729:13
    #5 0x29f0745 in js::wasm::BaseCompiler::emitFunction() js/src/asmjs/WasmBaselineCompile.cpp:6215:10
    #6 0x29f47a2 in js::wasm::BaselineCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmBaselineCompile.cpp:6482:10
    #7 0x72c845 in js::wasm::CompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3477:16
    #8 0x6afca0 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:824:14
    #9 0x645e3d in DecodeFunctionBody(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/Wasm.cpp:940:12
    #10 0x645e3d in DecodeCodeSection(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/Wasm.cpp:968
    #11 0x645e3d in DecodeModule(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>, js::wasm::ShareableBytes const&, JS::MutableHandle<js::ArrayBufferObject*>) js/src/asmjs/Wasm.cpp:1141
    #12 0x63bc72 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<js::WasmInstanceObject*>) js/src/asmjs/Wasm.cpp:1249:27
    #13 0x59a42e in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5226:14
    #14 0x1ea21d1 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:235:15
[...]
    #27 0x461088 in _start (/home/ubuntu/build/build/js+0x461088)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /srv/jenkins/jobs/mozilla-inbound-build-wasm/workspace/arch/64/type/debug/dist/include/mozilla/Vector.h:1045:3 in mozilla::Vector<js::wasm::BaseCompiler::Stk, 8ul, js::SystemAllocPolicy>::shrinkTo(unsigned long)
==374==ABORTING

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Lars T Hansen [:lth]]

This could be a propagation-of-void error (no void value slot is pushed when one should be pushed); it does not look like a failure to cope with unreachable code.  Select, operating on a void condition (iself sort of a weird idea) tries to pop two values off the stack but only one is there, because the one arm resulted in a void value that was not pushed by pushJoinReg at a control flow join (this sounds like a bug).

Comment 3

[Lars T Hansen [:lth]]

This test case worries me.  It triggers what is a bug in the baseline compiler, but it does so with code that IMO probably should not have passed verification:

  loop
   f64const
   nop
   nop
   nop
   nop
   nop
  end
  i32const
  select
  ...

The select passes verification because it gets to consume the void value that is pushed onto the value stack on function body entry, ie, in pushControl(), called from readFunctionStart().  That could be right, but it feels weird.  Dan?

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1280930-push-void.patch

No test case yet, see comments.  We'll land a test case later, when appropriate.  But it will be nice to get this fix in.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Lars T Hansen [:lth] from comment #3)
> This test case worries me.  It triggers what is a bug in the baseline
> compiler, but it does so with code that IMO probably should not have passed
> verification:
> 
>   loop
>    f64const
>    nop
>    nop
>    nop
>    nop
>    nop
>   end
>   i32const
>   select
>   ...
> 
> The select passes verification because it gets to consume the void value
> that is pushed onto the value stack on function body entry, ie, in
> pushControl(), called from readFunctionStart().  That could be right, but it
> feels weird.  Dan?

I don't think this is true: readLoop/readBlock both call pushControl, which pushes a TypeAndValue with Void and no value, the same way readFunctionStart does? So as long as the result of the select is used as a Void expression, we're fine here, aren't we?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8764206 [details] [diff] [review]
bug1280930-push-void.patch

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

Yes, I think this is correct, re-reading WasmBinaryIterator.h. Thanks.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
> (In reply to Lars T Hansen [:lth] from comment #3)
> 
> I don't think this is true: readLoop/readBlock both call pushControl, which
> pushes a TypeAndValue with Void and no value, the same way readFunctionStart
> does? So as long as the result of the select is used as a Void expression,
> we're fine here, aren't we?

The more I think about this the more I think we are very far from fine.

In the ML semantics, there is no such thing as "pushControl", and no spurious void values on the stack.  In the ML semantics, this test case should not even decode, if I read the code correctly.  (I have not tried running it, I should do so.)

:decoder, I don't know if you're currently doing this for testing, but it would be an interesting experiment to run the ML semantics side-by-side with spidermonkey on various random-ish inputs, and to check that the two systems reject programs in the same way.

Comment 8

[Lars T Hansen [:lth]]

Indeed, the ML semantics throw a "decoding error" for the test input.

Comment 9

[Lars T Hansen [:lth]]

More data:

The ML parser for the (module "...") form is really buggy (https://github.com/WebAssembly/spec/issues/299) but once fixed, I have constructed a test case that mirrors the test case here, it looks like this:

(module "\00asm\0b\00\00\00\04type\04\01\40\00\00\08function\02\01\00\04code\07\01\04\00\00\0a\01\05")

which -- in case it's not obvious -- is a function that takes no parameters, returns no value, and whose body is NOP; I32(1); SELECT.  The ML implementation throws a verification error: too few operands for operator.

Comment 10

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/91f47e63c8394234b148c4a14afa43ddd25bdfd7
Bug 1280930 - pushJoinReg must push void values, not ignore them. r=bbouvier

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/91f47e63c839

Comment 12

[Lars T Hansen [:lth]]

Issue with verification filed as bug 1282740.