Closed Bug 1316634 Opened 3 years ago Closed 3 years ago

wasm: Implement more efficient WebAssembly.Validate

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(5 files, 5 obsolete files)

No description provided.
Depends on: 1316635
Priority: P2 → P1
Depends on: 1317319
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).
Attachment #8811857 - Flags: review?(luke)
Attached patch v2.validate-file.patch (obsolete) — Splinter Review
Just a code motion patch: moves the function validation from WasmCompile.cpp to a new WasmValidate.cpp.
Attachment #8811858 - Flags: review?(luke)
Attached patch v3.efficient-validate.patch (obsolete) — Splinter Review
And this finally implements the efficient WebAssembly.Validate, using all the previous refactorings, it's a breeze.
Attachment #8811859 - Flags: review?(luke)
Some perf numbers, for validating AngryBots.wasm:

- before patches: ~950ms (full compilation)
- after patches: ~50ms
- v8 latest master: ~100ms
Comment on attachment 8811857 [details] [diff] [review]
v1.use-env-in-function-decoder.patch

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

Good choice
Attachment #8811857 - Flags: review?(luke) → review+
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.
Attachment #8811858 - Flags: review?(luke) → review+
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>.
Attachment #8811859 - Flags: review?(luke) → review+
(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.
Attached patch 4.checked-int.patch (obsolete) — Splinter Review
This uses CheckedInt<uint32_t> for checking the max number of functions and globals, as suggested in the other bug.
Attachment #8812212 - Flags: review?(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 ;)
Attached patch 4.checked-int.patch (obsolete) — Splinter Review
Doh!
Attachment #8812212 - Attachment is obsolete: true
Attachment #8812212 - Flags: review?(luke)
Attachment #8812224 - Flags: review?(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?
Attachment #8812224 - Flags: review?(luke) → review+
Yes! needinfo'ing myself to do it on monday.
Flags: needinfo?(bbouvier)
Attached patch v1.patchSplinter Review
updated and rebased
Attachment #8811857 - Attachment is obsolete: true
Attachment #8812247 - Flags: review+
Attachment #8812247 - Flags: checkin?(luke)
Attached patch v2.patchSplinter Review
updated and rebased
Attachment #8811858 - Attachment is obsolete: true
Attachment #8812248 - Flags: review+
Attachment #8812248 - Flags: checkin?(luke)
Attached patch v3.patchSplinter Review
updated and rebased
Attachment #8811859 - Attachment is obsolete: true
Attachment #8812249 - Flags: review+
Attachment #8812249 - Flags: checkin?(luke)
Attached patch v4.patchSplinter Review
updated and rebased
Attachment #8812224 - Attachment is obsolete: true
Attachment #8812250 - Flags: review+
Attachment #8812250 - Flags: checkin?(luke)
Pushed by lwagner@mozilla.com:
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
Attachment #8812247 - Flags: checkin?(luke) → checkin+
Attachment #8812248 - Flags: checkin?(luke) → checkin+
Attachment #8812249 - Flags: checkin?(luke) → checkin+
Attachment #8812250 - Flags: checkin?(luke) → checkin+
(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...
Attached patch aurora.patchSplinter Review
Here's the aurora patch, r+'d by previous review.
Flags: needinfo?(bbouvier)
Attachment #8812749 - Flags: review+
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
Attachment #8812749 - Flags: approval-mozilla-aurora?
Comment on attachment 8812749 [details] [diff] [review]
aurora.patch

protect wasm against DoS, take in aurora52
Attachment #8812749 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.