1316634 - wasm: Implement more efficient WebAssembly.Validate

Status: RESOLVED FIXED

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

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v1.use-env-in-function-decoder.patch

This makes use of ModuleEnvironment in FunctionDecoder, allowing greater code reuse in the future. It's a bit unfortunate to open the black box here, but it works (and the returned ModuleEnv is const&, so no risk to mutate it somehow).

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v2.validate-file.patch

Just a code motion patch: moves the function validation from WasmCompile.cpp to a new WasmValidate.cpp.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v3.efficient-validate.patch

And this finally implements the efficient WebAssembly.Validate, using all the previous refactorings, it's a breeze.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Some perf numbers, for validating AngryBots.wasm:

- before patches: ~950ms (full compilation)
- after patches: ~50ms
- v8 latest master: ~100ms

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8811857 [details] [diff] [review]
v1.use-env-in-function-decoder.patch

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

Good choice

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8811858 [details] [diff] [review]
v2.validate-file.patch

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

Ooh, instead of splitting out a new file, could you rename WasmBinaryFormat.h/cpp to WasmValidate.h/cpp and put ValidateFunctionBody there?  That's (now) a much more apt name for its contents.  Might also be good to change all the prefixes from Decode* to Validate* to match.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8811859 [details] [diff] [review]
v3.efficient-validate.patch

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

\o/

::: js/src/wasm/WasmValidate.cpp
@@ +488,5 @@
> +wasm::Validate(const ShareableBytes& bytecode, bool* valid)
> +{
> +    *valid = false;
> +
> +    UniqueChars error;

How about taking in 'error' as an outparam and having the same interface contract as wasm::Compile() except returning a bool.  I think that would simplify the error handling here (removing SetResult()) and I like the regularity.  Going further, we could try out (for both wasm::Compile/Validate) this fancy new Result type with a Result<UniqueChars>.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Luke Wagner [:luke] from comment #6)
> Comment on attachment 8811858 [details] [diff] [review]
> v2.validate-file.patch
> 
> Review of attachment 8811858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ooh, instead of splitting out a new file, could you rename
> WasmBinaryFormat.h/cpp to WasmValidate.h/cpp and put ValidateFunctionBody
> there?  That's (now) a much more apt name for its contents.  Might also be
> good to change all the prefixes from Decode* to Validate* to match.

Thanks! I've moved the decoding functions to WasmValidate.h/cpp but the Decode to Validate renaming I will do in a follow-up patch.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.checked-int.patch

This uses CheckedInt<uint32_t> for checking the max number of functions and globals, as suggested in the other bug.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8812212 [details] [diff] [review]
4.checked-int.patch

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

::: js/src/wasm/WasmValidate.cpp
@@ +891,5 @@
>      uint32_t numDefs;
>      if (!d.readVarU32(&numDefs))
>          return d.fail("expected number of function definitions");
>  
> +    CheckedInt<uint32_t> numFuncs = env->funcSigs.length() + numDefs;

You need to assign one of the operands to the CheckedInt before the addition ;)

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 4.checked-int.patch

Doh!

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8812224 [details] [diff] [review]
4.checked-int.patch

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

Thanks!  Do you suppose you could create a m-a patch too?

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Yes! needinfo'ing myself to do it on monday.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v1.patch

updated and rebased

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v2.patch

updated and rebased

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v3.patch

updated and rebased

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: v4.patch

updated and rebased

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0077e64b3fd
Use ModuleEnvironment in the FunctionDecoder; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad96b0dcb41
Move function validation to WasmBinaryFormat.h/cpp && rename it WasmValidate; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/24995c8a348b
Implement efficient WebAssembly.validate; r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/c11a7836e61b
Use CheckedInt before checking against maximum limits; r=luke

Comment 19

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #7)
> Going further, we could try out this fancy new Result type with a
> Result<UniqueChars>.

FWIW the current Result patches may not correctly handle move-only types, so this might require a little extra fixing to Result.h.  Just in case something appears busted and it's unclear whether it's a Result bug or not...

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: aurora.patch

Here's the aurora patch, r+'d by previous review.

Comment 21

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8812749 [details] [diff] [review]
aurora.patch

Approval Request Comment
[Feature/regressing bug #]: since the beginning of wasm
[User impact if declined]: easy way to trigger ddos
[Describe test coverage new/current, TreeHerder]: all green on TBPL for a few days
[Risks and why]: no risks (locally compiles and passes all tests) 
[String/UUID change made/needed]: n/a

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0077e64b3fd
https://hg.mozilla.org/mozilla-central/rev/bad96b0dcb41
https://hg.mozilla.org/mozilla-central/rev/24995c8a348b
https://hg.mozilla.org/mozilla-central/rev/c11a7836e61b

Comment 23

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8812749 [details] [diff] [review]
aurora.patch

protect wasm against DoS, take in aurora52

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/19f94f079251