1452592 - Missing mutability guard on WebAssembly.Global

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P1)

Description

[Lars T Hansen [:lth]]

Currently we allow an import that creates an immutable global to import a mutable global.  (Found this when trying to run the WebAssembly.Global reference tests.)  It is also possible we're missing the test in the opposite direction.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1452592-check-global-mutability.patch

Check mutability when we link.  As noted in some comments, the spec is still unclear on what happens when we import a number to a mutable global, so let's disallow it for now.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8966242 [details] [diff] [review]
bug1452592-check-global-mutability.patch

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

Looks good, thanks.

::: js/src/jit-test/tests/wasm/globals.js
@@ +542,5 @@
> +	// The spec is vague here; conservatively it is safest to reject
> +	// importing a primitive value to a mutable global.
> +	//
> +	// See commented-out test case below which we'll need to resurrect if
> +	// this behavior is supposed to be allowed.

Note: in this case, the behavior of creating wasm.Global objects for imported mutable primitives could be removed as well, would this behavior get forbidden.

::: js/src/wasm/WasmJS.cpp
@@ +265,5 @@
> +#if defined(ENABLE_WASM_GLOBAL) && defined(EARLY_BETA_OR_EARLIER)
> +                // If the import value is not a WasmGlobalObject but a
> +                // primitive, conservatively treat it as immutable.  The spec is
> +                // vague on this.
> +                if (global.isMutable()) {

Can this block be fused after the else above? That is, replace the `else` above by:

} else if (global.isMutable()) {
  ... /// report error
} else

Comment 3

[Lars T Hansen [:lth]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8966242 [details] [diff] [review]
> bug1452592-check-global-mutability.patch

So there was a brief discussion about this yesterday, and the conclusion is that we'll not allow a constant to be matched to a mutable global, ie a constant value in the import object is going to translate as an immutable WebAssembly.Global.

> ::: js/src/jit-test/tests/wasm/globals.js
> @@ +542,5 @@
> > +	// The spec is vague here; conservatively it is safest to reject
> > +	// importing a primitive value to a mutable global.
> > +	//
> > +	// See commented-out test case below which we'll need to resurrect if
> > +	// this behavior is supposed to be allowed.
> 
> Note: in this case, the behavior of creating wasm.Global objects for
> imported mutable primitives could be removed as well, would this behavior
> get forbidden.

I will in fact remove that behavior, and I'll upload a new patch for you to look at, since this code is a little tricky.

> ::: js/src/wasm/WasmJS.cpp
> @@ +265,5 @@
> > +#if defined(ENABLE_WASM_GLOBAL) && defined(EARLY_BETA_OR_EARLIER)
> > +                // If the import value is not a WasmGlobalObject but a
> > +                // primitive, conservatively treat it as immutable.  The spec is
> > +                // vague on this.
> > +                if (global.isMutable()) {
> 
> Can this block be fused after the else above? That is, replace the `else`
> above by:
> 
> } else if (global.isMutable()) {
>   ... /// report error
> } else

Apparently not, for a subtle reason: In the current draft spec (and I think this won't change), the i64 check happens before the mutability check, so I have to move this code further down into the block that it's in right now.

Comment 4

[Lars T Hansen [:lth]]

Attachment: bug1452592-check-global-mutability-v2.patch

Asking for a re-review here primarily to make sure we agree on the change in WasmModule.cpp.

I opted to leave the second loop in place to guard against future bugs, but to change it to a DEBUG-only loop that checks that we don't need to create any more Global objects for immutable imports that come in as values.

Another test case had to change, too: the regression test that previously used the now-illegal behavior to provoke a crash could have been removed, but I instead changed it slightly to more-or-less have the same heap dynamics.  It probably doesn't quite mean the same thing but it doesn't cost much either.  LMK what you think.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8966496 [details] [diff] [review]
bug1452592-check-global-mutability-v2.patch

I want to drive-by-fix another problem here first.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1452592-check-global-mutability-v3.patch

As before, but with an additional fix for spec compliance: we first dispatch on the type of the import value (WebAssembly.Global; Number; something else) and then when we know it's a Number we first test for int64 and then for immutability.  The order is observable through exception handling, naturally.

This version also cleans up test cases by using wasmEvalText more broadly.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8966505 [details] [diff] [review]
bug1452592-check-global-mutability-v3.patch

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

Looks good to me, thanks for the patch! Good idea to keep the loop within a DEBUG only block.

While you're around, I've spotted two other spec issues: the i64 error in the value getter/setter should be a type error, not a link error (it should be enough to change the _LINK suffix to a _TYPE suffix and update tests accordingly).

::: js/src/jit-test/tests/wasm/globals.js
@@ +300,4 @@
>  
>      let g = i.exports.g;
>  
> +    assertErrorMessage(() => i.exports.g.value, LinkError, /cannot pass i64 to or from JS/);

Here and below, should be TypeErrors.

::: js/src/wasm/WasmJS.cpp
@@ +278,5 @@
>  
>                  if (!ToWebAssemblyValue(cx, global.type(), v, &val))
>                      return false;
> +            } else {
> +                return ThrowBadImportType(cx, import.field.get(), "Number");

style nit: you can probably lower indent by one level if you revert the condition here (would also make it easier to follow).

::: js/src/wasm/WasmModule.cpp
@@ -1075,5 @@
>          const GlobalDesc& global = globals[globalIndex];
>          MOZ_ASSERT(global.importIndex() == globalIndex);
> -        if (!global.isIndirect())
> -            continue;
> -        if (!EnsureGlobalObject(cx, globalImportValues, globalIndex, global, globalObjs))

Now that EnsureGlobalObject is only used once, maybe it can be inlined back into this function?

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a735eb98349
Check mutability when importing globals. r=bbouvier

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/8a735eb98349