[wasm] Check segment bounds beforehand

[wasm] Check segment bounds beforehand

Also fixes check for table segments to be performed against actual size not declared one.

Makes us pass memory.wast and linking.wast tests (modulo issue 5860).

R=titzer@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2649553002
Cr-Commit-Position: refs/heads/master@{#42607}
Committed: https://chromium.googlesource.com/v8/v8/+/fc44a1d9cab4b4bff1a817928fe7b6e98ef8be5e

[titzer, 2017-01-20 14:28:34]

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

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1303: uint32_t table_size = table.min_size;
Does the spec say we should check against the imported table size or the minimum
specified in the table import? My guess is the former.

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1344: if (!LoadDataSegments(nullptr, 0)) return nothing;
Shouldn't need to do anything in this case now.

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1584: bool LoadDataSegments(Address mem_addr, size_t
mem_size) {
Don't need to return anything anymore.

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1592: if (dest_offset + source_size > mem_size ||
These should be DCHECKS now I guess.

Can we factor out a range check method too?

[rossberg, 2017-01-20 14:57:35]

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

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1303: uint32_t table_size = table.min_size;
On 2017/01/20 14:28:34, titzer wrote:
> Does the spec say we should check against the imported table size or the
minimum
> specified in the table import? My guess is the former.

Spec interpreter currently says actual size -- which makes sense, I think,
because the actual address might be an imported global, and the imported memory
size statically unknown and thus conservatively annotated as 1 (or even 0).

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1344: if (!LoadDataSegments(nullptr, 0)) return nothing;
On 2017/01/20 14:28:34, titzer wrote:
> Shouldn't need to do anything in this case now.

Done.

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1584: bool LoadDataSegments(Address mem_addr, size_t
mem_size) {
On 2017/01/20 14:28:34, titzer wrote:
> Don't need to return anything anymore.

Done.

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1592: if (dest_offset + source_size > mem_size ||
On 2017/01/20 14:28:34, titzer wrote:
> These should be DCHECKS now I guess.
> 
> Can we factor out a range check method too?

Done.

[titzer, 2017-01-20 15:03:21]

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

https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1303: uint32_t table_size = table.min_size;
On 2017/01/20 14:57:35, rossberg wrote:
> On 2017/01/20 14:28:34, titzer wrote:
> > Does the spec say we should check against the imported table size or the
> minimum
> > specified in the table import? My guess is the former.
> 
> Spec interpreter currently says actual size -- which makes sense, I think,
> because the actual address might be an imported global, and the imported
memory
> size statically unknown and thus conservatively annotated as 1 (or even 0).

Ok, because this code checks against the size in the import declaration. You
need to look into the table_instances_ array, which stores the actual table
object that was imported.

[rossberg, 2017-01-23 12:26:26]

Description was changed from

==========
[wasm] Check segment bounds beforehand

Makes us pass memory.wast and linking.wast tests (modulo issue 5860).

R=titzer@chromium.org
BUG=
==========

to

==========
[wasm] Check segment bounds beforehand

Also fixes check for table segments to be performed against actual size not
declared one.

Makes us pass memory.wast and linking.wast tests (modulo issue 5860).

R=titzer@chromium.org
BUG=
==========

[rossberg, 2017-01-23 12:33:33]

On 2017/01/20 15:03:21, titzer wrote:
> https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc
> File src/wasm/wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2649553002/diff/1/src/wasm/wasm-module.cc#new...
> src/wasm/wasm-module.cc:1303: uint32_t table_size = table.min_size;
> On 2017/01/20 14:57:35, rossberg wrote:
> > On 2017/01/20 14:28:34, titzer wrote:
> > > Does the spec say we should check against the imported table size or the
> > minimum
> > > specified in the table import? My guess is the former.
> > 
> > Spec interpreter currently says actual size -- which makes sense, I think,
> > because the actual address might be an imported global, and the imported
> memory
> > size statically unknown and thus conservatively annotated as 1 (or even 0).
> 
> Ok, because this code checks against the size in the import declaration. You
> need to look into the table_instances_ array, which stores the actual table
> object that was imported.

Done. The check actually was wrong before. Had to split up InitializeTables to
make this work.

With this patch we pass the additional spec tests in
https://github.com/WebAssembly/spec/pull/408

[titzer, 2017-01-23 12:58:52]

lgtm

[rossberg, 2017-01-23 12:59:06]

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

[commit-bot: I haz the power, 2017-01-23 12:59:14]

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

[rossberg, 2017-01-23 12:59:18]

The CQ bit was unchecked by rossberg@chromium.org

[rossberg, 2017-01-23 12:59:23]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2017-01-23 12:59:31]

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-23 13:00:05]

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

[commit-bot: I haz the power, 2017-01-23 13:00:06]

Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21332)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)

[rossberg, 2017-01-23 13:11:14]

The CQ bit was checked by rossberg@chromium.org

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

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-23 13:11:57]

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

[commit-bot: I haz the power, 2017-01-23 13:11:58]

Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21333)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21417)

[rossberg, 2017-01-23 13:15:34]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2017-01-23 13:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2649553002/#ps60001
(title: "Explicit cast to make compiler happy")

[commit-bot: I haz the power, 2017-01-23 13:15:44]

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-23 13:16:34]

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

[commit-bot: I haz the power, 2017-01-23 13:16:35]

Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21334)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21418)

[rossberg, 2017-01-23 13:19:54]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2017-01-23 13:19:54]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2649553002/#ps80001
(title: "YA explicit cast")

[commit-bot: I haz the power, 2017-01-23 13:20:04]

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-23 13:20:53]

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

[commit-bot: I haz the power, 2017-01-23 13:20:54]

Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21420)

[rossberg, 2017-01-23 13:25:42]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2017-01-23 13:25:43]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2649553002/#ps100001
(title: "Explicit cast fest pt 3")

[commit-bot: I haz the power, 2017-01-23 13:25:45]

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-23 13:26:14]

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

[commit-bot: I haz the power, 2017-01-23 13:26:14]

Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21337)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)

[rossberg, 2017-01-23 16:09:24]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2017-01-23 16:09:24]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2649553002/#ps120001
(title: "Fix error message")

[commit-bot: I haz the power, 2017-01-23 16:09:35]

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-23 16:41:02]

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

[commit-bot: I haz the power, 2017-01-23 16:41:03]

Failed to apply patch for src/wasm/wasm-js.cc:
While running git apply --index -p1;
  error: patch failed: src/wasm/wasm-js.cc:323
  error: src/wasm/wasm-js.cc: patch does not apply

Patch:       src/wasm/wasm-js.cc
Index: src/wasm/wasm-js.cc
diff --git a/src/wasm/wasm-js.cc b/src/wasm/wasm-js.cc
index
ee8e87de07788cb06463d9ab3c842d00d9e24b99..bba8be77f2381b4af72b958b90923a20a5e1ddf0
100644
--- a/src/wasm/wasm-js.cc
+++ b/src/wasm/wasm-js.cc
@@ -323,7 +323,7 @@ void WebAssemblyInstantiate(const
v8::FunctionCallbackInfo<v8::Value>& args) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
 
   HandleScope scope(isolate);
-  ErrorThrower thrower(i_isolate, "WebAssembly.compile()");
+  ErrorThrower thrower(i_isolate, "WebAssembly.instantiate()");
 
   Local<Context> context = isolate->GetCurrentContext();
   v8::Local<v8::Promise::Resolver> resolver;

[rossberg, 2017-01-23 16:50:42]

The CQ bit was checked by rossberg@chromium.org

[rossberg, 2017-01-23 16:50:43]

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

[commit-bot: I haz the power, 2017-01-23 16:50:45]

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-23 17:21:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485190242904480,
"parent_rev": "6d1894e4f4a63a165b5bfcae414bd7137e01f8d9", "commit_rev":
"fc44a1d9cab4b4bff1a817928fe7b6e98ef8be5e"}

[commit-bot: I haz the power, 2017-01-23 17:21:45]

Description was changed from

==========
[wasm] Check segment bounds beforehand

Also fixes check for table segments to be performed against actual size not
declared one.

Makes us pass memory.wast and linking.wast tests (modulo issue 5860).

R=titzer@chromium.org
BUG=
==========

to

==========
[wasm] Check segment bounds beforehand

Also fixes check for table segments to be performed against actual size not
declared one.

Makes us pass memory.wast and linking.wast tests (modulo issue 5860).

R=titzer@chromium.org
BUG=

Review-Url: https://codereview.chromium.org/2649553002
Cr-Commit-Position: refs/heads/master@{#42607}
Committed:
https://chromium.googlesource.com/v8/v8/+/fc44a1d9cab4b4bff1a817928fe7b6e98ef...
==========

[commit-bot: I haz the power, 2017-01-23 17:21:47]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/fc44a1d9cab4b4bff1a817928fe7b6e98ef...