[wasm] Enforce strict ordering of WASM module sections.

[wasm] Enforce strict ordering of WASM module sections.

R=jfb@chromium.org,rossberg@chromium.org
BUG=

Committed: https://crrev.com/b3cf031eb833a9be11061f757a70f7d24a8b9c0c
Cr-Commit-Position: refs/heads/master@{#35698}

[JF, 2016-04-19 15:48:47]

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

https://codereview.chromium.org/1900153002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:124: section_length) {
This seems pretty impossible to cause to overflow, but it would still be good to
check all untrusted inputs.

https://codereview.chromium.org/1900153002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:149: }
Duplicate.

https://codereview.chromium.org/1900153002/diff/1/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/1900153002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:31: // Entries have the form F(enumerator, string).
Update doc on F.

https://codereview.chromium.org/1900153002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:45: F(End, 0, "end")
I'm not sure I understand orders: the non-zero ones have to follow each other,
but globals / functions / end ca appear anywhere, and repeat as much as they
want?

https://codereview.chromium.org/1900153002/diff/1/test/mjsunit/wasm/wasm-modu...
File test/mjsunit/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/1900153002/diff/1/test/mjsunit/wasm/wasm-modu...
test/mjsunit/wasm/wasm-module-builder.js:180: // Add functions declaractions
typo

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

https://codereview.chromium.org/1900153002/diff/1/test/unittests/wasm/module-...
test/unittests/wasm/module-decoder-unittest.cc:1077: EXPECT_VERIFIES(data);
Hmm, this is now OK?

[titzer, 2016-04-20 08:51:25]

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

https://codereview.chromium.org/1900153002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:124: section_length) {
On 2016/04/19 15:48:47, JF wrote:
> This seems pretty impossible to cause to overflow, but it would still be good
to
> check all untrusted inputs.

Done. (I've refactored this a bit so the check is after the consume_bytes, which
has an overflow check inside.)

https://codereview.chromium.org/1900153002/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:149: }
On 2016/04/19 15:48:47, JF wrote:
> Duplicate.

Done.

https://codereview.chromium.org/1900153002/diff/1/src/wasm/wasm-module.h
File src/wasm/wasm-module.h (right):

https://codereview.chromium.org/1900153002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:31: // Entries have the form F(enumerator, string).
On 2016/04/19 15:48:47, JF wrote:
> Update doc on F.

Done.

https://codereview.chromium.org/1900153002/diff/1/src/wasm/wasm-module.h#newc...
src/wasm/wasm-module.h:45: F(End, 0, "end")
On 2016/04/19 15:48:47, JF wrote:
> I'm not sure I understand orders: the non-zero ones have to follow each other,
> but globals / functions / end ca appear anywhere, and repeat as much as they
> want?

That's right. The 0-value sections are V8-internal/legacy sections. (We should
also probably rename the V8-only sections: later).

https://codereview.chromium.org/1900153002/diff/1/test/mjsunit/wasm/wasm-modu...
File test/mjsunit/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/1900153002/diff/1/test/mjsunit/wasm/wasm-modu...
test/mjsunit/wasm/wasm-module-builder.js:180: // Add functions declaractions
On 2016/04/19 15:48:47, JF wrote:
> typo

Done.

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

https://codereview.chromium.org/1900153002/diff/1/test/unittests/wasm/module-...
test/unittests/wasm/module-decoder-unittest.cc:1077: EXPECT_VERIFIES(data);
On 2016/04/19 15:48:47, JF wrote:
> Hmm, this is now OK?

Yeah. It's OK to have an empty import table without a signatures section, since
we don't explicitly disallow that in the design. (Of course a non-empty
signatures section would fail to validate any signature indexes in there; I've
added another test.)

[JF, 2016-04-20 17:59:54]

lgtm, I think all the other comments can be in a separate patch (I only just
noticed stuff was wrong).

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

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:399: expected_length);
If section is Code::Max then it'll print a slightly confusing error message.
Probably OK, just confusing :-)

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:25: #undef F
Need an entry for Code::Max. Should probably make it clear that it's an unknown
section.

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:31: #undef F
Same, but this one's weird.

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:37: #undef F
Need a 0 at the end for Code::Max?

[titzer, 2016-04-21 10:38:20]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-04-21 10:38:21]

The patchset sent to the CQ was uploaded after l-g-t-m from jfb@chromium.org
Link to the patchset: https://codereview.chromium.org/1900153002/#ps120001
(title: "not OOB")

[commit-bot: I haz the power, 2016-04-21 10:38:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900153002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900153002/120001

[commit-bot: I haz the power, 2016-04-21 10:41:20]

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

[commit-bot: I haz the power, 2016-04-21 10:41:21]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/13969)

[titzer, 2016-04-21 10:59:23]

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

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/module-decoder...
src/wasm/module-decoder.cc:399: expected_length);
On 2016/04/20 at 17:59:54, JF wrote:
> If section is Code::Max then it'll print a slightly confusing error message.
Probably OK, just confusing :-)

Acknowledged.

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:25: #undef F
On 2016/04/20 at 17:59:54, JF wrote:
> Need an entry for Code::Max. Should probably make it clear that it's an
unknown section.

done.

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:31: #undef F
On 2016/04/20 at 17:59:54, JF wrote:
> Same, but this one's weird.

done

https://codereview.chromium.org/1900153002/diff/60001/src/wasm/wasm-module.cc...
src/wasm/wasm-module.cc:37: #undef F
On 2016/04/20 at 17:59:54, JF wrote:
> Need a 0 at the end for Code::Max?

done

[titzer, 2016-04-21 11:06:30]

The CQ bit was checked by titzer@chromium.org

[titzer, 2016-04-21 11:06:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jfb@chromium.org
Link to the patchset: https://codereview.chromium.org/1900153002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-04-21 11:06:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1900153002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1900153002/140001

[commit-bot: I haz the power, 2016-04-21 11:18:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-04-22 19:14:33]

Description was changed from

==========
[wasm] Enforce strict ordering of WASM module sections.

R=jfb@chromium.org,rossberg@chromium.org
BUG=
==========

to

==========
[wasm] Enforce strict ordering of WASM module sections.

R=jfb@chromium.org,rossberg@chromium.org
BUG=

Committed: https://crrev.com/b3cf031eb833a9be11061f757a70f7d24a8b9c0c
Cr-Commit-Position: refs/heads/master@{#35698}
==========

[commit-bot: I haz the power, 2016-04-22 19:14:34]

Patchset 8 (id:??) landed as
https://crrev.com/b3cf031eb833a9be11061f757a70f7d24a8b9c0c
Cr-Commit-Position: refs/heads/master@{#35698}