1449213 - wasm: mutable imported globals crash if a primitive value is provided

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

new WebAssembly.Instance(new WebAssembly.Module(wasmTextToBinary(`(module
    (global (import "a" "b") (mut i32))
)`)), {
    a: { b: 42 }
});

This makes the shell crash, because the global is indirect (imported and mutable), but we don't provide a WebAssembly.Global as its initializer.

Assertion failure: aIndex < mLength, at /home/ben/mozilla/builds/d64/dist/include/mozilla/Vector.h:552
https://searchfox.org/mozilla-central/source/js/src/wasm/WasmInstance.cpp#459

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Trying to consider possible solutions here:

- create dummy WebAssembly.Global for imported mutable globals that aren't exported as well, in Module::instantiateGlobalExports. That seems suboptimal because it will create an object that isn't reused by the embedder.
- just check if there's a global in the required slot when instantiating, and if there's not, just use the global val vector provided instead.

I think (2) makes more sense here.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

So I've tried (2), but if a global is indirect, then we have an extra indirection when reading/writing it, so we get a segfault later in the code.

Luke, why are imported non-exported mutable globals indirect in the first place? It seems their memory isn't accessible nor visible from the outside, but I may be missing something here.

Comment 3

[Luke Wagner [:luke]]

Good find!

An imported mutable global variable can be mutated outside the importing wasm module (by JS or another wasm module) and thus indirection is needed to see the live contents.

The way this is supposed to work (in the spec and impl) is that Global objects are synthesized for primitives when imported by a mutable import.  This should probably happen in Module::instantiateGlobalExports() (renamed to instantiateGlobals()) or GetImports(), not sure which is right-er yet...

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #3)
> An imported mutable global variable can be mutated outside the importing
> wasm module (by JS or another wasm module) and thus indirection is needed to
> see the live contents.

I get it, that's for the "regular" case where the imported global is actually a WebAssembly.Global and not a primitive. Will make a patch.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: globals.patch

Implementation notes:

- chose to reuse Module::instantiateGlobalExports because it was exactly doing the same thing for exported globals.
- I thought at some point we could just iterate over all the GlobalDesc in globals and ensure the wasm global object is there iff global->isIndirect(). But that's actually untrue! Exported constant globals are direct, but they still need a wasm global object. So we need to iterate over all exports and ensure the wasm global object is there, and then do the same for indirect imported globals by iterating over Imports. Added a comment explaining the non-equivalence of isIndirect() <=> get a wasm.Global object above GlobalDesc::isIndirect.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

(also please ignore the churn in the test file, my editor replaced tabs by spaces; I've only added the test case from comment 0)

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8963127 [details] [diff] [review]
globals.patch

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

Thanks, nicely done!

::: js/src/wasm/WasmModule.cpp
@@ +1070,2 @@
>  
> +    size_t globalImportIndex = 0;

nit: how about numGlobalImports?

::: js/src/wasm/WasmModule.h
@@ +148,5 @@
>      bool instantiateMemory(JSContext* cx, MutableHandleWasmMemoryObject memory) const;
>      bool instantiateTable(JSContext* cx,
>                            MutableHandleWasmTableObject table,
>                            SharedTableVector* tables) const;
> +    bool instantiateGlobalObjects(JSContext* cx,

nit: to match the above, how about just instantiateGlobals()?  Before long they'll all be plural :)

::: js/src/wasm/WasmTypes.h
@@ +809,5 @@
> +    // of their module.
> +    //
> +    // Note that isIndirect() isn't equivalent to getting a WasmGlobalObject:
> +    // an immutable exported global will still get an object, but will not be
> +    // indirect.

Great point!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/538ce8ca4b13
Create WebAssembly.Global objects for imported globals that received a primitive; r=luke

Comment 9

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 538ce8ca4b13 (bug 1449213) for spidermonkey build bustage at js/src/jit-test/tests/asm.js/testCall.js on a CLOSED TREE 

Failure push:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=538ce8ca4b132cc27bb645f5f20caef4c5d6fb93

Failure log: 
https://treeherder.mozilla.org/logviewer.html#?job_id=170993593&repo=mozilla-inbound&lineNumber=8207

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9125b9cf1730aac801ab8a6811557e10eec7bea1

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Crap, asm.js imports don't use Module::imports_, so the final assertion about imported globals in instantiateGlobals doesn't hold there. Changed it to a MOZ_ASSERT_IF(!metadata().isAsmJS(), ...).

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b78ea9a4d2f3
Create WebAssembly.Global objects for imported globals that received a primitive; r=luke

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b78ea9a4d2f3

Comment 13

[Lars T Hansen [:lth]]

Particularly galling that the comment in instantiateGlobalExports mentioned the immutable import case while the code did nothing to address that.  :-/

Thanks!