[wasm] Fix and tighten memory validation

src/wasm/wasm-module.cc

Index: src/wasm/wasm-module.cc
diff --git a/src/wasm/wasm-module.cc b/src/wasm/wasm-module.cc
index 74fa1bf93031e2ab31088ca0b8696f36078a04b8..f3528cafef2de18c1e1046cff883c6b57313fa4f 100644
--- a/src/wasm/wasm-module.cc
+++ b/src/wasm/wasm-module.cc
@@ -1305,7 +1305,7 @@ class WasmInstanceBuilder {
       Address mem_start = static_cast<Address>(memory_->backing_store());
       uint32_t mem_size =
           static_cast<uint32_t>(memory_->byte_length()->Number());
-      LoadDataSegments(mem_start, mem_size);
+      if (!LoadDataSegments(mem_start, mem_size)) return nothing;
 
       uint32_t old_mem_size = compiled_module_->mem_size();
       Address old_mem_start =
@@ -1317,7 +1317,7 @@ class WasmInstanceBuilder {
                                      old_mem_size, mem_size);
       compiled_module_->set_memory(memory_);
     } else {
-      LoadDataSegments(nullptr, 0);
+      if (!LoadDataSegments(nullptr, 0)) return nothing;
     }
 
     //--------------------------------------------------------------------------
@@ -1550,7 +1550,7 @@ class WasmInstanceBuilder {
   }
 
   // Load data segments into the memory.
-  void LoadDataSegments(Address mem_addr, size_t mem_size) {
+  bool LoadDataSegments(Address mem_addr, size_t mem_size) {
     Handle<SeqOneByteString> module_bytes(compiled_module_->module_bytes(),
                                           isolate_);
     for (const WasmDataSegment& segment : module_->data_segments) {
@@ -1558,18 +1558,19 @@ class WasmInstanceBuilder {
       // Segments of size == 0 are just nops.
       if (source_size == 0) continue;
       uint32_t dest_offset = EvalUint32InitExpr(segment.dest_addr);
-      if (dest_offset >= mem_size || source_size >= mem_size ||
-          dest_offset > (mem_size - source_size)) {
+      if (dest_offset + source_size > mem_size ||

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

I think we have to be careful about integer overflow here. Maybe switch to
int64?

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

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

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

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]

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]

Ah, right. Damn brain.

+          dest_offset + source_size < dest_offset) {
         thrower_->LinkError("data segment (start = %" PRIu32 ", size = %" PRIu32
                             ") does not fit into memory (size = %" PRIuS ")",
                             dest_offset, source_size, mem_size);
-        return;
+        return false;
       }
       byte* dest = mem_addr + dest_offset;
       const byte* src = reinterpret_cast<const byte*>(
           module_bytes->GetCharsAddress() + segment.source_offset);
       memcpy(dest, src, source_size);
     }
+    return true;
   }
 
   void WriteGlobalValue(WasmGlobal& global, Handle<Object> value) {