1324008 - Baldr: tweak internal limits to match other browsers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

E.g.: https://codereview.chromium.org/2574133002/patch/40001/50003

Comment 1

[Luke Wagner [:luke]]

Attachment: impl-limits

See https://github.com/v8/v8/blob/master/src/wasm/wasm-limits.h for the more up-to-date version.

Comment 2

[Luke Wagner [:luke]]

Attachment: impl-limits

The static_assert in ARM's MacroAssembler::farJumpWithPatch() reminded me to lower the JumpImmediateRange.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8821349 [details] [diff] [review]
impl-limits

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

Sweet! Spotted a few other instances that could benefit from having checks...

::: js/src/wasm/WasmBinaryConstants.h
@@ +461,5 @@
> +static const unsigned MaxParams                   =     1000;
> +static const unsigned MaxBrTableElems             =  1000000;
> +static const unsigned MaxModuleBytes              = 1024 * 1024 * 1024;
> +static const unsigned MaxFunctionBytes            =         128 * 1024;
> +

Should we have one constant for MaxMemoryPages too?

::: js/src/wasm/WasmValidate.cpp
@@ +1398,5 @@
>          if (!d.readVarU32(&numElems))
>              return d.fail("expected segment size");
>  
> +        if (numElems > MaxTableLength)
> +            return d.fail("too many table elements");

Nice! I've realized that a good rule of thumb is that each resize() or append() call be preceded by a size check: under this rule, the following would also need supplementary checks (that I should have added back when doing the refactoring):

- in the Name section, for the number of names (it's different from failing because of OOM, which is acceptable in the not-mandatory Name section).
- in DecodeImport, for the number of functions and the number of globals.

Could you add these, please? (extra credit for adding binary test cases)

Comment 4

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Should we have one constant for MaxMemoryPages too?

Well, I'm not as sure about that one, because INT32_MAX (the current limit imposed by ArrayBuffer) seems perfectly reasonable on 64-bit (and on 32-bit we do indeed clamp to 1gb as a practical matter).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01c0cd38284
Baldr: tweak internal limits to match other browsers (r=bbouvier)

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d01c0cd38284