[wasm] Enforce limits for maximums for many WebAssembly binary entities.

[wasm] Enforce limits for maximums for many WebAssembly binary entities.

This CL moves even more limits to wasm-limits.h and enforces limits for
types, functions, parameter counts, return counts, local counts, imports,
globals, and exports.

R=clemensh@chromium.org, ahaas@chromium.org
BUG=

Committed: https://crrev.com/058939ab93428ba6c959497e224da2c1eee7a408
Cr-Commit-Position: refs/heads/master@{#41699}

[titzer, 2016-12-14 12:22:06]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 12:22:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 12:25:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 12:25:41]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)

[titzer, 2016-12-14 12:29:05]

The CQ bit was checked by titzer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 12:29:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2016-12-14 12:44:14]

lgtm with nits.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:379: uint32_t table_count = consume_count("table
count", 1);
Can you also store the maximum table count as a constant?

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:397: uint32_t memory_count = consume_count("memory
count", 1);
Can you also store the maximum memory count as a constant?

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:1011: uint32_t param_count = consume_u32v("param
count");
Why can we not use consume_count here?

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:1025: uint32_t return_count = consume_u32v("return
count");
same here.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/wasm-limits.h
File src/wasm/wasm-limits.h (right):

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/wasm-limits.h#...
src/wasm/wasm-limits.h:12: const size_t kV8MaxWasmTypes = 1000000;
Can you add a comment that these number are just picked arbitrarily (are they?)?

https://codereview.chromium.org/2574133002/diff/20001/test/unittests/wasm/mod...
File test/unittests/wasm/module-decoder-unittest.cc (right):

https://codereview.chromium.org/2574133002/diff/20001/test/unittests/wasm/mod...
test/unittests/wasm/module-decoder-unittest.cc:871: EXPECT_FALSE(sig !=
nullptr);
Why do you do this double negation here? Would it not be easier to just write
"EXPECT_TRUE(sig == nullptr)", or "EXPECT_EQ(nullptr, sig)"?

[Clemens Hammacher, 2016-12-14 12:50:26]

lgtm with nits

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (left):

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:557: init->entries.reserve(SafeReserve(num_elem));
We could still reserve here if num_elem is "plausible" in the sense that there
are at least num_elem bytes left in the decoder.

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:1016: }
You can use consume_count() followed by "if (failed()) return nullptr;".
Also, a few lines below we "return new (module_zone) FunctionSig(0, 0,
nullptr);" in case of failure. This should be consistent.

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:1026: const size_t max_return_count =
FLAG_wasm_mv_prototype
Same here.

https://codereview.chromium.org/2574133002/diff/1/test/unittests/wasm/module-...
File test/unittests/wasm/module-decoder-unittest.cc (right):

https://codereview.chromium.org/2574133002/diff/1/test/unittests/wasm/module-...
test/unittests/wasm/module-decoder-unittest.cc:857: EXPECT_FALSE(sig !=
nullptr);
Double negation is hard to read IMO.
EXPECT_TRUE(sig == nullptr) reads easier for me.

https://codereview.chromium.org/2574133002/diff/1/test/unittests/wasm/module-...
test/unittests/wasm/module-decoder-unittest.cc:871: EXPECT_FALSE(sig !=
nullptr);
Same here.

[commit-bot: I haz the power, 2016-12-14 12:57:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 12:57:43]

Dry run: This issue passed the CQ dry run.

[titzer, 2016-12-14 17:19:54]

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (left):

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:557: init->entries.reserve(SafeReserve(num_elem));
On 2016/12/14 12:50:26, Clemens Hammacher wrote:
> We could still reserve here if num_elem is "plausible" in the sense that there
> are at least num_elem bytes left in the decoder.

I just really wanted to get rid of safe reserve :-)

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2574133002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:1016: }
On 2016/12/14 12:50:26, Clemens Hammacher wrote:
> You can use consume_count() followed by "if (failed()) return nullptr;".
> Also, a few lines below we "return new (module_zone) FunctionSig(0, 0,
> nullptr);" in case of failure. This should be consistent.

Done.

https://codereview.chromium.org/2574133002/diff/1/test/unittests/wasm/module-...
File test/unittests/wasm/module-decoder-unittest.cc (right):

https://codereview.chromium.org/2574133002/diff/1/test/unittests/wasm/module-...
test/unittests/wasm/module-decoder-unittest.cc:857: EXPECT_FALSE(sig !=
nullptr);
On 2016/12/14 12:50:26, Clemens Hammacher wrote:
> Double negation is hard to read IMO.
> EXPECT_TRUE(sig == nullptr) reads easier for me.

Done.

https://codereview.chromium.org/2574133002/diff/1/test/unittests/wasm/module-...
test/unittests/wasm/module-decoder-unittest.cc:871: EXPECT_FALSE(sig !=
nullptr);
On 2016/12/14 12:50:26, Clemens Hammacher wrote:
> Same here.

Done.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder.cc
File src/wasm/module-decoder.cc (right):

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:379: uint32_t table_count = consume_count("table
count", 1);
On 2016/12/14 12:44:14, ahaas wrote:
> Can you also store the maximum table count as a constant?

Done.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:397: uint32_t memory_count = consume_count("memory
count", 1);
On 2016/12/14 12:44:14, ahaas wrote:
> Can you also store the maximum memory count as a constant?

Done.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:1011: uint32_t param_count = consume_u32v("param
count");
On 2016/12/14 12:44:14, ahaas wrote:
> Why can we not use consume_count here?

Done.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:1025: uint32_t return_count = consume_u32v("return
count");
On 2016/12/14 12:44:14, ahaas wrote:
> same here.

Done.

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/wasm-limits.h
File src/wasm/wasm-limits.h (right):

https://codereview.chromium.org/2574133002/diff/20001/src/wasm/wasm-limits.h#...
src/wasm/wasm-limits.h:12: const size_t kV8MaxWasmTypes = 1000000;
On 2016/12/14 12:44:14, ahaas wrote:
> Can you add a comment that these number are just picked arbitrarily (are
they?)?

Done.

[titzer, 2016-12-14 17:20:00]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-12-14 17:20:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
clemensh@chromium.org, ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2574133002/#ps40001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-12-14 17:20:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 17:45:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481736000468290,
"parent_rev": "5d61ec84837913ab754d4c1fa390fcf08579398b", "commit_rev":
"90cd2c1e82994d9ac9aeada3868d73847fc9a2c3"}

[commit-bot: I haz the power, 2016-12-14 17:46:08]

Description was changed from

==========
[wasm] Enforce limits for maximums for many WebAssembly binary entities.

This CL moves even more limits to wasm-limits.h and enforces limits for
types, functions, parameter counts, return counts, local counts, imports,
globals, and exports.

R=clemensh@chromium.org, ahaas@chromium.org
BUG=
==========

to

==========
[wasm] Enforce limits for maximums for many WebAssembly binary entities.

This CL moves even more limits to wasm-limits.h and enforces limits for
types, functions, parameter counts, return counts, local counts, imports,
globals, and exports.

R=clemensh@chromium.org, ahaas@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2574133002
==========

[commit-bot: I haz the power, 2016-12-14 17:46:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-14 17:46:57]

Description was changed from

==========
[wasm] Enforce limits for maximums for many WebAssembly binary entities.

This CL moves even more limits to wasm-limits.h and enforces limits for
types, functions, parameter counts, return counts, local counts, imports,
globals, and exports.

R=clemensh@chromium.org, ahaas@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2574133002
==========

to

==========
[wasm] Enforce limits for maximums for many WebAssembly binary entities.

This CL moves even more limits to wasm-limits.h and enforces limits for
types, functions, parameter counts, return counts, local counts, imports,
globals, and exports.

R=clemensh@chromium.org, ahaas@chromium.org
BUG=

Committed: https://crrev.com/058939ab93428ba6c959497e224da2c1eee7a408
Cr-Commit-Position: refs/heads/master@{#41699}
==========

[commit-bot: I haz the power, 2016-12-14 17:46:58]

Patchset 3 (id:??) landed as
https://crrev.com/058939ab93428ba6c959497e224da2c1eee7a408
Cr-Commit-Position: refs/heads/master@{#41699}