[wasm] Fix check failure on invalid name section

[wasm] Fix check failure on invalid name section

After decoding an invalid function name (e.g. OOB), we stored the parsed
offset and length into the WasmFunction anyway, resulting in a runtime
CHECK failure later on.
This CL fixes this, and adds a regression test.

R=titzer@chromium.org
CC=mtrofin@chromium.org, bradnelson@chromium.org
BUG=chromium:684858
Review-Url: https://codereview.chromium.org/2656713003
Cr-Commit-Position: refs/heads/master@{#42654}
Committed: https://chromium.googlesource.com/v8/v8/+/0ec3a264bcba5c899f7e72fa5b98dd191477e272

[Clemens Hammacher, 2017-01-25 10:46:49]

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

[commit-bot: I haz the power, 2017-01-25 10:46:56]

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

[titzer, 2017-01-25 10:48:17]

lgtm

[bradn, 2017-01-25 11:18:04]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2017-01-25 11:18:05]

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

https://codereview.chromium.org/2656713003/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:623: if (inner.ok() && func_index <
module->functions.size()) {
Ah I see.
Kind of goofy how this section is speced (i.e. can fail to validate but is still
ok).
Don't we need to back this out in the case that some later part of this section
fails?
I.e. we could end up with some names added but then fall over?

[commit-bot: I haz the power, 2017-01-25 11:21:25]

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

[commit-bot: I haz the power, 2017-01-25 11:21:26]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-25 11:35:42]

On 2017/01/25 at 11:18:05, bradnelson wrote:
> https://codereview.chromium.org/2656713003/diff/1/src/wasm/module-decoder.cc
> File src/wasm/module-decoder.cc (right):
> 
>
https://codereview.chromium.org/2656713003/diff/1/src/wasm/module-decoder.cc#...
> src/wasm/module-decoder.cc:623: if (inner.ok() && func_index <
module->functions.size()) {
> Ah I see.
> Kind of goofy how this section is speced (i.e. can fail to validate but is
still ok).
> Don't we need to back this out in the case that some later part of this
section fails?
> I.e. we could end up with some names added but then fall over?

Good point. We could just check after the loop whether there was an error in the
inner decoder, and in this case reset the function names on all functions.
It's not really specified, but I think that would match the expectation better.
Ben, WDYT?

I will land this CL now, and fix the other concern in a separate CL if we agree
on this.

[Clemens Hammacher, 2017-01-25 11:35:50]

The CQ bit was checked by clemensh@chromium.org

[commit-bot: I haz the power, 2017-01-25 11:35:52]

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, 2017-01-25 11:37:49]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485344150478300, "parent_rev":
"fdc5cd7987d50d45e6436079c8267993485bef1f", "commit_rev":
"0ec3a264bcba5c899f7e72fa5b98dd191477e272"}

[commit-bot: I haz the power, 2017-01-25 11:37:53]

Description was changed from

==========
[wasm] Fix check failure on invalid name section

After decoding an invalid function name (e.g. OOB), we stored the parsed
offset and length into the WasmFunction anyway, resulting in a runtime
CHECK failure later on.
This CL fixes this, and adds a regression test.

R=titzer@chromium.org
CC=mtrofin@chromium.org, bradnelson@chromium.org
BUG=chromium:684858
==========

to

==========
[wasm] Fix check failure on invalid name section

After decoding an invalid function name (e.g. OOB), we stored the parsed
offset and length into the WasmFunction anyway, resulting in a runtime
CHECK failure later on.
This CL fixes this, and adds a regression test.

R=titzer@chromium.org
CC=mtrofin@chromium.org, bradnelson@chromium.org
BUG=chromium:684858

Review-Url: https://codereview.chromium.org/2656713003
Cr-Commit-Position: refs/heads/master@{#42654}
Committed:
https://chromium.googlesource.com/v8/v8/+/0ec3a264bcba5c899f7e72fa5b98dd19147...
==========

[commit-bot: I haz the power, 2017-01-25 11:37:54]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/v8/v8/+/0ec3a264bcba5c899f7e72fa5b98dd19147...

[titzer, 2017-01-25 11:53:29]

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

https://codereview.chromium.org/2656713003/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:623: if (inner.ok() && func_index <
module->functions.size()) {
On 2017/01/25 11:18:05, bradn wrote:
> Ah I see.
> Kind of goofy how this section is speced (i.e. can fail to validate but is
still
> ok).
> Don't we need to back this out in the case that some later part of this
section
> fails?
> I.e. we could end up with some names added but then fall over?

I'm OK with "best effort" if we just leave the names that did parse
successfully. I'm also OK clearing them, as it is probably just a simple loop
after this. But land away.

[Mircea Trofin, 2017-01-25 15:58:51]

mtrofin@chromium.org changed reviewers:
+ mtrofin@chromium.org

[Mircea Trofin, 2017-01-25 15:58:51]

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

https://codereview.chromium.org/2656713003/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:623: if (inner.ok() && func_index <
module->functions.size()) {
On 2017/01/25 11:53:29, titzer wrote:
> On 2017/01/25 11:18:05, bradn wrote:
> > Ah I see.
> > Kind of goofy how this section is speced (i.e. can fail to validate but is
> still
> > ok).
> > Don't we need to back this out in the case that some later part of this
> section
> > fails?
> > I.e. we could end up with some names added but then fall over?
> 
> I'm OK with "best effort" if we just leave the names that did parse
> successfully. I'm also OK clearing them, as it is probably just a simple loop
> after this. But land away.
> 

We should take a stance though and propose a PR on the Spec.

[Mircea Trofin, 2017-01-25 16:42:01]

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

https://codereview.chromium.org/2656713003/diff/1/src/wasm/module-decoder.cc#...
src/wasm/module-decoder.cc:623: if (inner.ok() && func_index <
module->functions.size()) {
On 2017/01/25 15:58:51, Mircea Trofin wrote:
> On 2017/01/25 11:53:29, titzer wrote:
> > On 2017/01/25 11:18:05, bradn wrote:
> > > Ah I see.
> > > Kind of goofy how this section is speced (i.e. can fail to validate but is
> > still
> > > ok).
> > > Don't we need to back this out in the case that some later part of this
> > section
> > > fails?
> > > I.e. we could end up with some names added but then fall over?
> > 
> > I'm OK with "best effort" if we just leave the names that did parse
> > successfully. I'm also OK clearing them, as it is probably just a simple
loop
> > after this. But land away.
> > 
> 
> We should take a stance though and propose a PR on the Spec. 

Followed up with this PR, please take a look (includes a proposal for expected
behavior surrounding this section):

https://github.com/WebAssembly/design/pull/967/commits/c7ac091c4ab5488fe989d2...