Closed Bug 1324008 Opened 5 years ago Closed 5 years ago
Baldr: tweak internal limits to match other browsers
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)
The static_assert in ARM's MacroAssembler::farJumpWithPatch() reminded me to lower the JumpImmediateRange.
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/d01c0cd38284 Baldr: tweak internal limits to match other browsers (r=bbouvier)
You need to log in before you can comment on or make changes to this bug.