Closed Bug 1324008 Opened 4 years ago Closed 4 years ago

Baldr: tweak internal limits to match other browsers

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file, 1 obsolete file)

Component: JavaScript Engine → JavaScript Engine: JIT
Attached patch impl-limits (obsolete) — Splinter Review
See https://github.com/v8/v8/blob/master/src/wasm/wasm-limits.h for the more up-to-date version.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Attachment #8821323 - Flags: review?(bbouvier)
Attached patch impl-limitsSplinter Review
The static_assert in ARM's MacroAssembler::farJumpWithPatch() reminded me to lower the JumpImmediateRange.
Attachment #8821323 - Attachment is obsolete: true
Attachment #8821323 - Flags: review?(bbouvier)
Attachment #8821349 - Flags: review?(bbouvier)
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)
Attachment #8821349 - Flags: review?(bbouvier) → review+
(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).
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d01c0cd38284
Baldr: tweak internal limits to match other browsers (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/d01c0cd38284
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.