Reland "[wasm] Do memory.copy bounds check in C++ code"

The return value of {memory_copy_wrapper} was defined as {bool} in
the original CL. When compiled with clang, the full return register is
written when {true} or {false} is returned. With msvc, however, the
return value is written as a single byte, without zero-extension. In
generated code, the full return register is used and therefore stale
bytes in the return register caused problems.

With this CL the return value is changed to {uint32_t}. This enforces
zero-extension of the return value and thereby fixes the issue.

R=clemensb@chromium.org

Bug: v8:10281
Change-Id: I628d01cfd7193fa960a7ccdf0d9fd896f510cd3e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2096626
Commit-Queue: Andreas Haas <ahaas@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66666}

src/codegen/external-reference.cc

@@ -289,6 +289,7 @@ FUNCTION_REFERENCE(wasm_word32_rol, wasm::word32_rol_wrapper)
 FUNCTION_REFERENCE(wasm_word32_ror, wasm::word32_ror_wrapper)
 FUNCTION_REFERENCE(wasm_word64_rol, wasm::word64_rol_wrapper)
 FUNCTION_REFERENCE(wasm_word64_ror, wasm::word64_ror_wrapper)
+FUNCTION_REFERENCE(wasm_memory_init, wasm::memory_init_wrapper)
 FUNCTION_REFERENCE(wasm_memory_copy, wasm::memory_copy_wrapper)
 FUNCTION_REFERENCE(wasm_memory_fill, wasm::memory_fill_wrapper)
 

src/codegen/external-reference.h

@@ -200,6 +200,7 @@ class StatsCounter;
   V(wasm_word64_ror, "wasm::word64_ror")                                      \
   V(wasm_word64_ctz, "wasm::word64_ctz")                                      \
   V(wasm_word64_popcnt, "wasm::word64_popcnt")                                \
+  V(wasm_memory_init, "wasm::memory_init")                                    \
   V(wasm_memory_copy, "wasm::memory_copy")                                    \
   V(wasm_memory_fill, "wasm::memory_fill")                                    \
   V(call_enqueue_microtask_function, "MicrotaskQueue::CallEnqueueMicrotask")  \

src/compiler/wasm-compiler.cc

@@ -4931,7 +4931,7 @@ Node* WasmGraphBuilder::MemoryInit(uint32_t data_segment_index, Node* dst,
   }
 
   Node* function = graph()->NewNode(mcgraph()->common()->ExternalConstant(
-      ExternalReference::wasm_memory_copy()));
+      ExternalReference::wasm_memory_init()));
 
   Node* stack_slot =
       StoreArgsInStackSlot({{MachineType::PointerRepresentation(), dst},
@@ -4981,22 +4981,19 @@ Node* WasmGraphBuilder::StoreArgsInStackSlot(
 
 Node* WasmGraphBuilder::MemoryCopy(Node* dst, Node* src, Node* size,
                                    wasm::WasmCodePosition position) {
-  Node* dst_fail = BoundsCheckMemRange(&dst, &size, position);
-  TrapIfTrue(wasm::kTrapMemOutOfBounds, dst_fail, position);
-  Node* src_fail = BoundsCheckMemRange(&src, &size, position);
-  TrapIfTrue(wasm::kTrapMemOutOfBounds, src_fail, position);
-
   Node* function = graph()->NewNode(mcgraph()->common()->ExternalConstant(
       ExternalReference::wasm_memory_copy()));
 
-  Node* stack_slot =
-      StoreArgsInStackSlot({{MachineType::PointerRepresentation(), dst},
-                            {MachineType::PointerRepresentation(), src},
+  Node* stack_slot = StoreArgsInStackSlot(
+      {{MachineType::PointerRepresentation(), instance_node_.get()},
+       {MachineRepresentation::kWord32, dst},
+       {MachineRepresentation::kWord32, src},
        {MachineRepresentation::kWord32, size}});
 
-  MachineType sig_types[] = {MachineType::Pointer()};
-  MachineSignature sig(0, 1, sig_types);
-  return SetEffect(BuildCCall(&sig, function, stack_slot));
+  MachineType sig_types[] = {MachineType::Bool(), MachineType::Pointer()};
+  MachineSignature sig(1, 1, sig_types);
+  Node* call = SetEffect(BuildCCall(&sig, function, stack_slot));
+  return TrapIfFalse(wasm::kTrapMemOutOfBounds, call, position);
 }
 
 Node* WasmGraphBuilder::MemoryFill(Node* dst, Node* value, Node* size,

src/wasm/module-instantiate.cc

@@ -669,11 +669,8 @@ void InstanceBuilder::LoadDataSegments(Handle<WasmInstanceObject> instance) {
       }
       // No need to copy empty segments.
       if (size == 0) continue;
-      Address dest_addr =
-          reinterpret_cast<Address>(instance->memory_start()) + dest_offset;
-      Address src_addr = reinterpret_cast<Address>(wire_bytes.begin()) +
-                         segment.source.offset();
-      memory_copy(dest_addr, src_addr, size);
+      std::memcpy(instance->memory_start() + dest_offset,
+                  wire_bytes.begin() + segment.source.offset(), size);
     } else {
       DCHECK(segment.active);
       // Segments of size == 0 are just nops.

src/wasm/wasm-external-refs.cc

@@ -11,7 +11,9 @@
 
 #include "src/base/bits.h"
 #include "src/base/ieee754.h"
+#include "src/common/assert-scope.h"
 #include "src/utils/memcopy.h"
+#include "src/wasm/wasm-objects-inl.h"
 
 #if defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER) || \
     defined(THREAD_SANITIZER) || defined(LEAK_SANITIZER) ||    \
@@ -323,41 +325,37 @@ void float64_pow_wrapper(Address data) {
   WriteUnalignedValue<double>(data, base::ieee754::pow(x, y));
 }
 
-// Asan on Windows triggers exceptions in this function to allocate
-// shadow memory lazily. When this function is called from WebAssembly,
-// these exceptions would be handled by the trap handler before they get
-// handled by Asan, and thereby confuse the thread-in-wasm flag.
-// Therefore we disable ASAN for this function. Alternatively we could
-// reset the thread-in-wasm flag before calling this function. However,
-// as this is only a problem with Asan on Windows, we did not consider
-// it worth the overhead.
-DISABLE_ASAN void memory_copy(Address dst, Address src, uint32_t size) {
-  // Use explicit forward and backward copy to match the required semantics for
-  // the memory.copy instruction. It is assumed that the caller of this
-  // function has already performed bounds checks, so {src + size} and
-  // {dst + size} should not overflow.
-  DCHECK(src + size >= src && dst + size >= dst);
-  uint8_t* dst8 = reinterpret_cast<uint8_t*>(dst);
-  uint8_t* src8 = reinterpret_cast<uint8_t*>(src);
-  if (src < dst && src + size > dst && dst + size > src) {
-    dst8 += size - 1;
-    src8 += size - 1;
-    for (; size > 0; size--) {
-      *dst8-- = *src8--;
-    }
-  } else {
-    for (; size > 0; size--) {
-      *dst8++ = *src8++;
-    }
-  }
-}
-
-void memory_copy_wrapper(Address data) {
+void memory_init_wrapper(Address data) {
   Address dst = ReadUnalignedValue<Address>(data);
   Address src = ReadUnalignedValue<Address>(data + sizeof(dst));
   uint32_t size =
       ReadUnalignedValue<uint32_t>(data + sizeof(dst) + sizeof(src));
-  memory_copy(dst, src, size);
+  std::memmove(reinterpret_cast<byte*>(dst), reinterpret_cast<byte*>(src),
+               size);
+}
+
+int32_t memory_copy_wrapper(Address data) {
+  constexpr int32_t kSuccess = 1;
+  constexpr int32_t kOutOfBounds = 0;
+  DisallowHeapAllocation disallow_heap_allocation;
+  size_t offset = 0;
+  Object raw_instance = ReadUnalignedValue<Object>(data);
+  WasmInstanceObject instance = WasmInstanceObject::cast(raw_instance);
+  offset += sizeof(Object);
+  size_t dst = ReadUnalignedValue<uint32_t>(data + offset);
+  offset += sizeof(uint32_t);
+  size_t src = ReadUnalignedValue<uint32_t>(data + offset);
+  offset += sizeof(uint32_t);
+  size_t size = ReadUnalignedValue<uint32_t>(data + offset);
+
+  size_t mem_size = instance.memory_size();
+  if (!base::IsInBounds(dst, size, mem_size)) return kOutOfBounds;
+  if (!base::IsInBounds(src, size, mem_size)) return kOutOfBounds;
+
+  byte* memory_start = instance.memory_start();
+  // Use std::memmove, because the ranges can overlap.
+  std::memmove(memory_start + dst, memory_start + src, size);
+  return kSuccess;
 }
 
 // Asan on Windows triggers exceptions in this function that confuse the

src/wasm/wasm-external-refs.h

@@ -71,9 +71,11 @@ V8_EXPORT_PRIVATE void word64_ror_wrapper(Address data);
 
 V8_EXPORT_PRIVATE void float64_pow_wrapper(Address data);
 
-void memory_copy_wrapper(Address data);
+void memory_init_wrapper(Address data);
 
-void memory_copy(Address dst, Address src, uint32_t size);
+// The return type is {int32_t} instead of {bool} to enforce the compiler to
+// zero-extend the result in the return register.
+int32_t memory_copy_wrapper(Address data);
 
 void memory_fill_wrapper(Address dst, uint32_t value, uint32_t size);
 

src/wasm/wasm-interpreter.cc

@@ -1824,7 +1824,8 @@ class ThreadImpl {
         Address src_addr =
             instance_object_->data_segment_starts()[imm.data_segment_index] +
             src;
-        memory_copy(dst_addr, src_addr, size);
+        std::memmove(reinterpret_cast<void*>(dst_addr),
+                     reinterpret_cast<void*>(src_addr), size);
         return true;
       }
       case kExprDataDrop: {
@@ -1850,7 +1851,8 @@ class ThreadImpl {
           return false;
         }
 
-        memory_copy(dst_addr, src_addr, size);
+        std::memmove(reinterpret_cast<void*>(dst_addr),
+                     reinterpret_cast<void*>(src_addr), size);
         return true;
       }
       case kExprMemoryFill: {