1259936 - [wasm] Assertion failure: amount <= framePushed_, at js/src/jit/MacroAssembler.cpp:2358

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The attached binary WebAssembly testcase crashes on mozilla-inbound revision 078bf91ed20a+ (build with --enable-gczeal --enable-optimize --enable-debug --enable-address-sanitizer --without-intl-api --enable-posix-nspr-emulation --disable-jemalloc --disable-tests --target=i686-pc-linux-gnu --enable-debug). 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:

==30822==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000 (pc 0x090900eb bp 0xff9bfd68 sp 0xff9bfd50 T0)
==30822==The signal is caused by a WRITE memory access.
==30822==Hint: address points to the zero page.
    #0 0x90900ea in js::jit::MacroAssembler::freeStack(unsigned int) js/src/jit/MacroAssembler.cpp:2358:5
    #1 0x932871e in js::jit::CodeGeneratorShared::emitAsmJSCall(js::jit::LAsmJSCall*) js/src/jit/shared/CodeGenerator-shared.cpp:1501:9
    #2 0x95a6ccc in js::jit::CodeGeneratorX86::visitAsmJSCall(js::jit::LAsmJSCall*) js/src/jit/x86/CodeGenerator-x86.cpp:319:5
    #3 0x8f89638 in js::jit::LAsmJSCall::accept(js::jit::LElementVisitor*) js/src/jit/shared/LIR-shared.h:7574:5
    #4 0x8a4ee43 in js::jit::CodeGenerator::generateBody() js/src/jit/CodeGenerator.cpp:4691:13
    #5 0x8ab17a2 in js::jit::CodeGenerator::generateAsmJS(js::wasm::FuncOffsets*) js/src/jit/CodeGenerator.cpp:8307:10
    #6 0x835c317 in js::wasm::IonCompileFunction(js::wasm::IonCompileTask*) js/src/asmjs/WasmIonCompile.cpp:3152:14
    #7 0x8325b39 in js::wasm::ModuleGenerator::finishFuncDef(unsigned int, unsigned int, js::wasm::FunctionGenerator*) js/src/asmjs/WasmGenerator.cpp:815:14
    #8 0x82baef1 in DecodeFunctionBody(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&, unsigned int) js/src/asmjs/Wasm.cpp:1329:12
    #9 0x82baef1 in DecodeFunctionBodies(JSContext*, js::wasm::Decoder&, js::wasm::ModuleGenerator&) js/src/asmjs/Wasm.cpp:1357
    #10 0x82baef1 in DecodeModule(JSContext*, mozilla::UniquePtr<char [], JS::FreePolicy>, unsigned char const*, unsigned int, mozilla::Vector<ImportName, 0u, js::SystemAllocPolicy>*, mozilla::UniquePtr<js::wasm::ExportMap, JS::DeletePolicy<js::wasm::ExportMap> >*, JS::MutableHandle<js::ArrayBufferObject*>, JS::MutableHandle<js::WasmModuleObject*>) js/src/asmjs/Wasm.cpp:1456
    #11 0x82ae5b4 in js::wasm::Eval(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, JS::MutableHandle<JSObject*>) js/src/asmjs/Wasm.cpp:1613:10
    #12 0x821eb1f in WasmLoop(JSContext*, unsigned int, JS::Value*) js/src/shell/js.cpp:5105:14
[...]
    #25 0x80aa658 in _start (/home/ubuntu/build/build/js+0x80aa658)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV js/src/jit/MacroAssembler.cpp:2358:5 in js::jit::MacroAssembler::freeStack(unsigned int)
==30822==ABORTING

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1259936.patch

So this one is fun (and another great catch by the fuzzers!):
- we have a few calls that set mirGen().maxStackArgBytes to something non 0
- then another call for which one argument stops control flow (it's a trap!)
- we've reset mirGen().maxStackArgBytes() but the call is actually never emitted: we don't call into finishCallArgs, don't set mirGen().maxStackArgBytes(), and its value is then inconsistent during codegen.

I think the fix is that when a call is aborted (can be tested in finishCallArgs() if we're inDeadCode() and the call->prevMaxArgBytes has been set to something), just restore the previous maxArgBytes, as if there were no calls at all.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8736673 [details] [diff] [review]
1259936.patch

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

Thanks for investigating this!

::: js/src/asmjs/WasmIonCompile.cpp
@@ +779,5 @@
>      void finishCallArgs(Call* call)
>      {
> +        if (inDeadCode()) {
> +            if (call->prevMaxStackBytes_)
> +                mirGen().setAsmJSMaxStackArgBytes(call->prevMaxStackBytes_);

I think this too could have a problem in a case like:
  (call $x (call $y) (unreachable))
in this case the parent call is unreachable but the inner call is alive and needs its stack space accounted for.

Stepping back, the root of this bug is the unnecessary complexity of abusing asmJSMaxStackArgBytes to track this mutable emitter state.  This field should be set once, at the end of emitting a MIRGraph, before codegening it.  Instead, FunctionCompiler would have its own mutable maxStackArgsBytes.  Rather than clearing and resetting it in the gross manner here, we'd store a Vector<Call*> stack in FC and when you finishCallArgs() on the outermost call, only then do you set FC::maxStackArgBytes = Max(FC::maxStackArgBytes, call->stackArgBytes).  When you finishCallArgs() on a non-outermost call, you simply propagate your stackArgBytes to the next-outer Call's maxChildStackBytes.  This strategy would naturally fix this whole bug (and that's how I should have written this from day 1).

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Simplify internal call stack management

You're right! And indeed it simplifies things a lot to do so. I really like how it ends up being lighter (and passArg does nothing but actually passing the arg, not messing up with the stackArgBytes anymore).

Note that startCallArgs always pushes the call, because:
- either we're in dead code at this point, and we'll end up in dead code in finish()
- or we're not in dead code at this point, but we might be in dead code in finish() (because of trap/br/return, etc.).
So if we always push in startCallArgs, we can always pop in finishCallArgs in any case.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8736751 [details] [diff] [review]
Simplify internal call stack management

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

Excellent!

::: js/src/asmjs/WasmIonCompile.cpp
@@ +167,2 @@
>      {
> +        mirGen().setAsmJSMaxStackArgBytes(maxStackArgBytes_);

Can you rename this to 'initWasmMaxStackArgBytes', assert that the previous value is the initial value, and then kill the reset MIRGenerator function?

@@ +803,5 @@
> +            return;
> +        }
> +
> +        // Non-outermost call
> +        Call* outer = callStack_[callStack_.length() - 1];

callStack_.back()

::: js/src/jit-test/tests/wasm/random-control-flow.js
@@ +37,5 @@
> +wasmEvalText(`(module
> +  (func (result i32) (param i32) (param i32) (i32.const 0))
> +  (func (result i32)
> +   (call 0 (i32.const 1) (call 0 (i32.const 2) (i32.const 3)))
> +   (call 0 (trap) (i32.const 4))

Could you also add a test case like the one I described in my previous comment that would've crashed with the previous patch?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you!

(In reply to Luke Wagner [:luke] from comment #5)
> Comment on attachment 8736751 [details] [diff] [review]
> Simplify internal call stack management
> 
> Review of attachment 8736751 [details] [diff] [review]:
> -----------------------------------------------------------------
> Could you also add a test case like the one I described in my previous
> comment that would've crashed with the previous patch?
I was afraid you'd ask this :-) It took some effort: the frame depth of a function is adjusted because of AsmJSFrameSize (4 bytes) + alignment for calls (rounded to 8 bytes) + AsmJSFrameBytesAfterReturnAddress (4 bytes more, for a total of 12 bytes). So we need the callee must have at least 4 int32 arguments, but then, we can smash the stack and force a return at the wrong location.

This test case actually also breaks the current patch, so there's a little tweak to do...

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: interdiff

I prefer to reask for review, just to be sure we're not missing anything else here:

- if we end up in dead code, we should propagate the maxStackArgBytes value, otherwise it is never set and we end up in the same bad stack smashing situation.
- i don't think we need to change anything on the next outer call, if there's one, though: if we're in dead code here, we'll be in dead code there too and won't have to worry about childClobbers, because the outer call is going to be aborted.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Full patch

Comment 9

[Luke Wagner [:luke]]

Oooh, good catch!  I think we *do* actually have to propagate from children to parents because I think, due to 'if', you can go from dead->live again.  Given that, I think we should do the stack depth propagation the same way in both cases (up front, before the dead code check).

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: folded.patch

So something like that? I don't see how that could happen in practice, do you have a test case in mind? Otherwise I'll try to make one on Monday.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 8737222 [details] [diff] [review]
folded.patch

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

Nice!

The example I was thinking about was something like:
  (call (if (cond) (call (block (call) (unreachable)))))
You have an inner reachable call, a parent unreachable call, but then the parent of *that* is back to reachable :)

::: js/src/asmjs/WasmIonCompile.cpp
@@ +683,5 @@
>          // always ABIStackAlignment-aligned, but don't forget to account for
>          // ShadowStackSpace and any other ABI warts.
>          ABIArgGenerator abi;
> +
> +        maxStackArgBytes_ = Max(maxStackArgBytes_, abi.stackBytesConsumedSoFar());

Hah, this hunk make me realize that, since loops are expressions, you should be using propagateMaxStackArgs() here too.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for the review!

(In reply to Luke Wagner [:luke] from comment #11)
> Comment on attachment 8737222 [details] [diff] [review]
> folded.patch
> 
> Review of attachment 8737222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> The example I was thinking about was something like:
>   (call (if (cond) (call (block (call) (unreachable)))))
> You have an inner reachable call, a parent unreachable call, but then the
> parent of *that* is back to reachable :)

I've kept it the way it was in the review, just to be extra-safe, but I think this example doesn't work:
- because of the associativity property of Max, we can't lose track of maxStackArgBytes_. So the only way this can go wrong is with call->childClobbers of the parent call, if there's one.
- either the condition is true, and we'll get the unreachable (or any control flow breaker) and the outer call won't actually happen.
- or the condition is false, and the inner call that would clobber the parent call doesn't happen.

So for this specific example, we should be fine. I tried long enough to find another that could go wrong, but they all seem to match this scheme at some point. Let fuzzers decide if they agree :) Landing the patch.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a184c73fcd3

Comment 14

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> - because of the associativity property of Max, we can't lose track of
> maxStackArgBytes_. So the only way this can go wrong is with
> call->childClobbers of the parent call, if there's one.

Not quite: child stack arg bytes must be added to a parent call's stack args bytes to produce the total stack arg bytes.  Assuming parents can be reachable even when some of their transitive children are unreachable, this means we could still hit the assertion.

> - either the condition is true, and we'll get the unreachable (or any
> control flow breaker) and the outer call won't actually happen.
> - or the condition is false, and the inner call that would clobber the
> parent call doesn't happen.

The point of the example was to show that 'if' allows a parent call to be statically reachable even if a nested child call is statically unreachable: you tweak the example to make the parent dynamically reachable by having an if-else switching around the order.

Comment 15

[Luke Wagner [:luke]]

Here's an example that I think would have asserted (but now validates and runs w/o error):
Wasm.instantiateModule(wasmTextToBinary(`
(module
    (func (param i32))
    (func (param i32) (param i32))
    (func
        (call 1
            (i32.const 43)
            (block $b
                (if (i32.const 1)
                    (call 0
                        (block
                            (call 0 (i32.const 42))
                            (br $b (i32.const 10))
                            (trap))))
                (i32.const 44))))
    (export "foo" 2))
`)).exports.foo();

In this example the innermost call sets childClobbers to true on the outermost call and both are executed at runtime.  The 'br' allows us to have the block containing the innermost call be unreachable but for both calls to dynamically execute.

Comment 16

[Luke Wagner [:luke]]

(Actually, the 'trap' is superfluous in that code.)

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: extra test case

You're right, thanks for offering a test case. I've misread the code written the other day...

I've tested with a 32 bits debug build, with the one line changed: it doesn't crash nor assert, but it does corrupt the stack, so we need a little extra: imports that check their arguments values.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 8737797 [details] [diff] [review]
extra test case

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

Great, thanks!

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd882bae05c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5a184c73fcd3
https://hg.mozilla.org/mozilla-central/rev/ffd882bae05c