1479794 - Regularize wasm globals and functions so that we don't expose module-internal struct types

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P3)

Description

[Lars T Hansen [:lth]]

We need a small amount of cleanup to implement the following rules.  When I write "Ref" I mean (ref T), not anyref.

- an imported function cannot accept Ref parameters or return a Ref result
- an exported function cannot accept Ref parameters, but can return a Ref
  result
- an imported global cannot have Ref type
- an exported global can have Ref type iff it is immutable

Together these rules ensure that struct types defined in a module do not "escape" that module via globals or functions in the sense that we need perform no type compatibility checks:

- at a call boundary in and out of a module
- when linking to an imported global
- when JS code stores into a global

In turn, that is desirable because we do not yet want to commit to any meaning for what it means for the manifest type of a value outside a module to be compatible with a type that is internal to the module.

(A similar conundrum for TypedObject properties of Ref type is solved slightly differently, see bug 1478982.)

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1479718-neuter-to-constructor.patch

Obviously these rules are fairly restrictive and a little user-hostile, and I am not at all averse to allowing more cases once we have struct.narrow or something like it to build on.  However, this patch is part of the puzzle that will let me land the struct.whatever instructions in the first place without creating something that is unsound.

Comment 2

[Lars T Hansen [:lth]]

Attachment: bug1479794-restrict-ref-types.patch

Upload the right patch.  See comment 0 and comment 1 for some thoughts around this.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

I haven't looked at the patch, but there was something i wanted to mention: from this rule:

- an exported function cannot accept Ref parameters, but can return a Ref
  result

It means that if a module exports a Table (which can implicitly export functions), then no functions can have ref parameters.

Comment 4

[Lars T Hansen [:lth]]

Oh, nice observation!  I'll have to think about that.  The alternative to the scheme I have here is to try to do what we do for int64, which is in a similar situation obviously; there we throw at the invocation point.  That is hostile to static analysis / early checking but might solve the above problem.

Comment 5

[Luke Wagner [:luke]]

bbouvier++  I like the throw-on-invocation semantics, and we can have the exception text just say "Not Yet Implemented".

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8996342 [details] [diff] [review]
bug1479794-restrict-ref-types.patch

(I think that means this patch needs an update.)

Comment 7

[Lars T Hansen [:lth]]

Note/memo to self: There's the JS<->wasm boundary, but also the wasm<->wasm boundary when two modules are linked through exports/imports.  In the latter case we also do not want to allow the link to succeed, this is stronger than the int64 restriction.

Comment 8

[Luke Wagner [:luke]]

No problem if it's necessary (and that may be a good argument for the static signature restriction instead of the dynamic one (which doesn't have a good cut point for wasm<->wasm) but I was wondering: why is it necessary to prevent wasm<->wasm?

Comment 9

[Lars T Hansen [:lth]]

(In reply to Luke Wagner [:luke] from comment #8)
> No problem if it's necessary (and that may be a good argument for the static
> signature restriction instead of the dynamic one (which doesn't have a good
> cut point for wasm<->wasm) but I was wondering: why is it necessary to
> prevent wasm<->wasm?

So this is cross-module wasm<->wasm, and the reason we should prevent those calls with Ref parameters or returns for the time being is that like JS<->wasm calls, they effectively export/import types.  Until we have a notion of type compatibility - and we can't use nominal equivalence here - we can't check that type constraints are obeyed.

(anyref's fine, of course.)

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1479794-restrict-ref-types-v2.patch

Here's another attempt.  See the commit message for details; in brief, we still have only static checks but that is sufficient because the restrictions are a little stronger than last time (notably including parameters and return types in all cases) and we also check the element initializers and call_indirect to handle the table cases.  Extensive negative and positive test cases in ref-restrict.js, included here.

Again, the purpose of this patch is to clear the ground so that I can land my preliminary patches for struct.new, struct.get, struct.set and struct.narrow without risking any unsound behavior in the engine as a result of unsound or missing type compatibility checks where values flow into a module.  Eventually these restrictions will be removed again.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 9004495 [details] [diff] [review]
bug1479794-restrict-ref-types-v2.patch

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

Makes sense; I like having the #ifdef so it's quite simple to chop this out later.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9004495 [details] [diff] [review]
bug1479794-restrict-ref-types-v2.patch

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

The code looks good to me as is, but it's still possible to implicitly expose functions that are exposed for ref, right?

- have a wasm module declare a local table of size non-zero that's exported, with no declared segments
- the wasm module has a wasm function exposing ref, not exported either
- a JS caller instantiates the module and get the table
- Table.set(0, 0);
- Table.get(0)();

If so, I think you need to change the implementation of Table.set (WasmTableObject::setImpl) to check if the function designated by the function index exposes ref or not.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(oops, nevermind my comment, I remembered for some reason that Table.set() would take as the second argument indexes into the module's function index space, which is false)

Comment 14

[Lars T Hansen [:lth]]

But it does raise the issue of whether there are other back doors in the spirit of this.  For example, we did have the situation where it's possible to export the table and thus revealing the functions, yet the functions in the table are not marked as exported; the patch prevents revealing functions exposed for Ref this way by checking the elemSegments, of course, but it is sneaky functionality.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fcca8293f56
Do not expose Ref types outside the defining module.  r=luke

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/9fcca8293f56