[wasm] Fix and tighten memory validation

[wasm] Fix and tighten memory validation

Makes us pass the spec's memory.wast test.

R=titzer@chromium.org
BUG=

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

[titzer, 2017-01-18 10:17:30]

https://codereview.chromium.org/2640453003/diff/1/src/wasm/function-body-deco...
File src/wasm/function-body-decoder.cc (right):

https://codereview.chromium.org/2640453003/diff/1/src/wasm/function-body-deco...
src/wasm/function-body-decoder.cc:1115: if (!module_->has_memory) {
Can you factor out a little helper to turn this into a one-liner?

if (!CheckHasMemory(error_msg)) break;

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

https://codereview.chromium.org/2640453003/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1561: if (dest_offset + source_size > mem_size ||
I think we have to be careful about integer overflow here. Maybe switch to
int64?

https://codereview.chromium.org/2640453003/diff/1/test/cctest/wasm/wasm-run-u...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/2640453003/diff/1/test/cctest/wasm/wasm-run-u...
test/cctest/wasm/wasm-run-utils.h:92: module_.has_memory = true;
Shouldn't you set this in AddMemory() below?

[rossberg, 2017-01-18 11:28:09]

https://codereview.chromium.org/2640453003/diff/1/src/wasm/function-body-deco...
File src/wasm/function-body-decoder.cc (right):

https://codereview.chromium.org/2640453003/diff/1/src/wasm/function-body-deco...
src/wasm/function-body-decoder.cc:1115: if (!module_->has_memory) {
On 2017/01/18 10:17:30, titzer wrote:
> Can you factor out a little helper to turn this into a one-liner?
> 
> if (!CheckHasMemory(error_msg)) break;
> 

Done.

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

https://codereview.chromium.org/2640453003/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1561: if (dest_offset + source_size > mem_size ||
On 2017/01/18 10:17:30, titzer wrote:
> I think we have to be careful about integer overflow here. Maybe switch to
> int64?

That is what the second condition is capturing: if there is a wrap-around, the
sum is smaller than the original value.

https://codereview.chromium.org/2640453003/diff/1/test/cctest/wasm/wasm-run-u...
File test/cctest/wasm/wasm-run-utils.h (right):

https://codereview.chromium.org/2640453003/diff/1/test/cctest/wasm/wasm-run-u...
test/cctest/wasm/wasm-run-utils.h:92: module_.has_memory = true;
On 2017/01/18 10:17:30, titzer wrote:
> Shouldn't you set this in AddMemory() below?

Done.

[titzer, 2017-01-18 11:34:20]

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

https://codereview.chromium.org/2640453003/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1561: if (dest_offset + source_size > mem_size ||
On 2017/01/18 11:28:09, rossberg wrote:
> On 2017/01/18 10:17:30, titzer wrote:
> > I think we have to be careful about integer overflow here. Maybe switch to
> > int64?
> 
> That is what the second condition is capturing: if there is a wrap-around, the
> sum is smaller than the original value.

Yeah, I think it's even trickier, because source_size might be very large (e.g.
4GB - 1), which means it would wrap around and land in bounds again.

[rossberg, 2017-01-18 11:37:51]

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

https://codereview.chromium.org/2640453003/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1561: if (dest_offset + source_size > mem_size ||
On 2017/01/18 11:34:20, titzer wrote:
> On 2017/01/18 11:28:09, rossberg wrote:
> > On 2017/01/18 10:17:30, titzer wrote:
> > > I think we have to be careful about integer overflow here. Maybe switch to
> > > int64?
> > 
> > That is what the second condition is capturing: if there is a wrap-around,
the
> > sum is smaller than the original value.
> 
> Yeah, I think it's even trickier, because source_size might be very large
(e.g.
> 4GB - 1), which means it would wrap around and land in bounds again.

The overflow condition isn't checking against the bounds but whether the value
got smaller. That should always work.

[titzer, 2017-01-18 11:40:42]

lgtm

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

https://codereview.chromium.org/2640453003/diff/1/src/wasm/wasm-module.cc#new...
src/wasm/wasm-module.cc:1561: if (dest_offset + source_size > mem_size ||
On 2017/01/18 11:37:51, rossberg wrote:
> On 2017/01/18 11:34:20, titzer wrote:
> > On 2017/01/18 11:28:09, rossberg wrote:
> > > On 2017/01/18 10:17:30, titzer wrote:
> > > > I think we have to be careful about integer overflow here. Maybe switch
to
> > > > int64?
> > > 
> > > That is what the second condition is capturing: if there is a wrap-around,
> the
> > > sum is smaller than the original value.
> > 
> > Yeah, I think it's even trickier, because source_size might be very large
> (e.g.
> > 4GB - 1), which means it would wrap around and land in bounds again.
> 
> The overflow condition isn't checking against the bounds but whether the value
> got smaller. That should always work.

Ah, right. Damn brain.

[rossberg, 2017-01-18 11:41:21]

The CQ bit was checked by rossberg@chromium.org

[commit-bot: I haz the power, 2017-01-18 11:41:23]

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-18 12:07:59]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484739681716120,
"parent_rev": "3c89788373ee71affaba8c874d339aea9c8ef298", "commit_rev":
"b86ef5ce8a89b488e0d1df6468857769b4e0e25b"}

[commit-bot: I haz the power, 2017-01-18 12:08:02]

Description was changed from

==========
[wasm] Fix and tighten memory validation

Makes us pass the spec's memory.wast test.

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

to

==========
[wasm] Fix and tighten memory validation

Makes us pass the spec's memory.wast test.

R=titzer@chromium.org
BUG=

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

[commit-bot: I haz the power, 2017-01-18 12:08:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/b86ef5ce8a89b488e0d1df6468857769b4e...