Closed Bug 1449213 Opened Last year Closed Last year

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file)

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
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.
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.
Flags: needinfo?(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...
Flags: needinfo?(luke)
(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.
Attached patch globals.patchSplinter Review
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.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8963127 - Flags: review?(luke)
(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 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!
Attachment #8963127 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/538ce8ca4b13
Create WebAssembly.Global objects for imported globals that received a primitive; r=luke
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(), ...).
Flags: needinfo?(bbouvier)
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b78ea9a4d2f3
Create WebAssembly.Global objects for imported globals that received a primitive; r=luke
https://hg.mozilla.org/mozilla-central/rev/b78ea9a4d2f3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1449932
Particularly galling that the comment in instantiateGlobalExports mentioned the immutable import case while the code did nothing to address that.  :-/

Thanks!
You need to log in before you can comment on or make changes to this bug.