[utils] Add IsInBounds(index, size, max) helper

This CL adds a helper function that simplifies a bounds check pattern
that appears repeatedly in the code.

R=clemensh@chromium.org

Change-Id: I8c617515b34eb2d262d58a239a29c1515de2d92d
Reviewed-on: https://chromium-review.googlesource.com/c/1417611
Commit-Queue: Ben Titzer <titzer@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58892}

src/compiler/wasm-compiler.cc

@@ -3313,9 +3313,7 @@ Node* WasmGraphBuilder::BoundsCheckMem(uint8_t access_size, Node* index,
     return index;
   }
 
-  const bool statically_oob = access_size > env_->max_memory_size ||
-                              offset > env_->max_memory_size - access_size;
-  if (statically_oob) {
+  if (!IsInBounds(offset, access_size, env_->max_memory_size)) {
     // The access will be out of bounds, even for the largest memory.
     TrapIfEq32(wasm::kTrapMemOutOfBounds, Int32Constant(0), 0, position);
     return mcgraph()->IntPtrConstant(0);

src/utils.h

@@ -73,6 +73,12 @@ inline constexpr bool IsInRange(T value, U lower_limit, U higher_limit) {
                                  static_cast<unsigned_T>(lower_limit));
 }
 
+// Checks if [index, index+length) is in range [0, max). Note that this check
+// works even if {index+length} would wrap around.
+inline constexpr bool IsInBounds(size_t index, size_t length, size_t max) {
+  return length <= max && index <= (max - length);
+}
+
 // X must be a power of 2.  Returns the number of trailing zeros.
 template <typename T,
           typename = typename std::enable_if<std::is_integral<T>::value>::type>

src/wasm/baseline/liftoff-compiler.cc

@@ -14,6 +14,7 @@
 #include "src/macro-assembler-inl.h"
 #include "src/objects/smi.h"
 #include "src/tracing/trace-event.h"
+#include "src/utils.h"
 #include "src/wasm/baseline/liftoff-assembler.h"
 #include "src/wasm/function-body-decoder-impl.h"
 #include "src/wasm/function-compiler.h"
@@ -1393,8 +1394,8 @@ class LiftoffCompiler {
   // (a jump to the trap was generated then); return false otherwise.
   bool BoundsCheckMem(FullDecoder* decoder, uint32_t access_size,
                       uint32_t offset, Register index, LiftoffRegList pinned) {
-    const bool statically_oob = access_size > env_->max_memory_size ||
-                                offset > env_->max_memory_size - access_size;
+    const bool statically_oob =
+        !IsInBounds(offset, access_size, env_->max_memory_size);
 
     if (!statically_oob &&
         (FLAG_wasm_no_bounds_checks || env_->use_trap_handler)) {

src/wasm/module-instantiate.cc

@@ -5,6 +5,7 @@
 #include "src/wasm/module-instantiate.h"
 #include "src/asmjs/asm-js.h"
 #include "src/property-descriptor.h"
+#include "src/utils.h"
 #include "src/wasm/js-to-wasm-wrapper-cache-inl.h"
 #include "src/wasm/module-compiler.h"
 #include "src/wasm/wasm-import-wrapper-cache-inl.h"
@@ -24,10 +25,6 @@ namespace {
 byte* raw_buffer_ptr(MaybeHandle<JSArrayBuffer> buffer, int offset) {
   return static_cast<byte*>(buffer.ToHandleChecked()->backing_store()) + offset;
 }
-bool in_bounds(uint32_t offset, size_t size, size_t upper) {
-  return offset + size <= upper && offset + size >= offset;
-}
-
 }  // namespace
 
 // A helper class to simplify instantiating a module from a module object.
@@ -432,7 +429,7 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
     DCHECK(elem_segment.table_index < table_instances_.size());
     uint32_t base = EvalUint32InitExpr(elem_segment.offset);
     size_t table_size = table_instances_[elem_segment.table_index].table_size;
-    if (!in_bounds(base, elem_segment.entries.size(), table_size)) {
+    if (!IsInBounds(base, elem_segment.entries.size(), table_size)) {
       thrower_->LinkError("table initializer is out of bounds");
       return {};
     }
@@ -444,7 +441,7 @@ MaybeHandle<WasmInstanceObject> InstanceBuilder::Build() {
   for (const WasmDataSegment& seg : module_->data_segments) {
     if (!seg.active) continue;
     uint32_t base = EvalUint32InitExpr(seg.dest_addr);
-    if (!in_bounds(base, seg.source.length(), instance->memory_size())) {
+    if (!IsInBounds(base, seg.source.length(), instance->memory_size())) {
       thrower_->LinkError("data segment is out of bounds");
       return {};
     }
@@ -623,7 +620,7 @@ void InstanceBuilder::LoadDataSegments(Handle<WasmInstanceObject> instance) {
     // Passive segments are not copied during instantiation.
     if (!segment.active) continue;
     uint32_t dest_offset = EvalUint32InitExpr(segment.dest_addr);
-    DCHECK(in_bounds(dest_offset, source_size, instance->memory_size()));
+    DCHECK(IsInBounds(dest_offset, source_size, instance->memory_size()));
     byte* dest = instance->memory_start() + dest_offset;
     const byte* src = wire_bytes.start() + segment.source.offset();
     memcpy(dest, src, source_size);
@@ -1464,7 +1461,7 @@ void InstanceBuilder::LoadTableSegments(Handle<WasmInstanceObject> instance) {
     uint32_t num_entries = static_cast<uint32_t>(elem_segment.entries.size());
     uint32_t index = elem_segment.table_index;
     TableInstance& table_instance = table_instances_[index];
-    DCHECK(in_bounds(base, num_entries, table_instance.table_size));
+    DCHECK(IsInBounds(base, num_entries, table_instance.table_size));
     for (uint32_t i = 0; i < num_entries; ++i) {
       uint32_t func_index = elem_segment.entries[i];
       const WasmFunction* function = &module_->functions[func_index];

src/wasm/wasm-interpreter.cc

@@ -1402,14 +1402,18 @@ class ThreadImpl {
 
   template <typename mtype>
   inline Address BoundsCheckMem(uint32_t offset, uint32_t index) {
-    size_t mem_size = instance_object_->memory_size();
-    if (sizeof(mtype) > mem_size) return kNullAddress;
-    if (offset > (mem_size - sizeof(mtype))) return kNullAddress;
-    if (index > (mem_size - sizeof(mtype) - offset)) return kNullAddress;
+    uint32_t effective_index = offset + index;
+    if (effective_index < index) {
+      return kNullAddress;  // wraparound => oob
+    }
+    if (!IsInBounds(effective_index, sizeof(mtype),
+                    instance_object_->memory_size())) {
+      return kNullAddress;  // oob
+    }
     // Compute the effective address of the access, making sure to condition
     // the index even in the in-bounds case.
     return reinterpret_cast<Address>(instance_object_->memory_start()) +
-           offset + (index & instance_object_->memory_mask());
+           (effective_index & instance_object_->memory_mask());
   }
 
   template <typename ctype, typename mtype>

src/wasm/wasm-objects.cc

@@ -1415,6 +1415,7 @@ Address WasmInstanceObject::GetCallTarget(uint32_t func_index) {
 namespace {
 void CopyTableEntriesImpl(Handle<WasmInstanceObject> instance, uint32_t dst,
                           uint32_t src, uint32_t count) {
+  DCHECK(IsInBounds(dst, count, instance->indirect_function_table_size()));
   if (src < dst) {
     for (uint32_t i = count; i > 0; i--) {
       auto to_entry = IndirectFunctionTableEntry(instance, dst + i - 1);
@@ -1439,8 +1440,8 @@ bool WasmInstanceObject::CopyTableEntries(Isolate* isolate,
   CHECK_EQ(0, table_index);  // TODO(titzer): multiple tables in TableCopy
   if (count == 0) return true;  // no-op
   auto max = instance->indirect_function_table_size();
-  if (dst > max || count > (max - dst)) return false;  // out-of-bounds
-  if (src > max || count > (max - src)) return false;  // out-of-bounds
+  if (!IsInBounds(dst, count, max)) return false;
+  if (!IsInBounds(src, count, max)) return false;
   if (dst == src) return true;                         // no-op
 
   if (!instance->has_table_object()) {

test/unittests/utils-unittest.cc

@@ -132,5 +132,73 @@ TYPED_TEST(UtilsTest, PassesFilterTest) {
   EXPECT_FALSE(PassesFilter(CStrVector(""), CStrVector("a")));
 }
 
+TEST(UtilsTest, IsInBounds) {
+// for column consistency and terseness
+#define INB(x, y, z) EXPECT_TRUE(IsInBounds(x, y, z))
+#define OOB(x, y, z) EXPECT_FALSE(IsInBounds(x, y, z))
+  INB(0, 0, 1);
+  INB(0, 1, 1);
+  INB(1, 0, 1);
+
+  OOB(0, 2, 1);
+  OOB(2, 0, 1);
+
+  INB(0, 0, 2);
+  INB(0, 1, 2);
+  INB(0, 2, 2);
+
+  INB(0, 0, 2);
+  INB(1, 0, 2);
+  INB(2, 0, 2);
+
+  OOB(0, 3, 2);
+  OOB(3, 0, 2);
+
+  INB(0, 1, 2);
+  INB(1, 1, 2);
+
+  OOB(1, 2, 2);
+  OOB(2, 1, 2);
+
+  const size_t max = std::numeric_limits<size_t>::max();
+  const size_t half = max / 2;
+
+  // limit cases.
+  INB(0, 0, max);
+  INB(0, 1, max);
+  INB(1, 0, max);
+  INB(max, 0, max);
+  INB(0, max, max);
+  INB(max - 1, 0, max);
+  INB(0, max - 1, max);
+  INB(max - 1, 1, max);
+  INB(1, max - 1, max);
+
+  INB(half, half, max);
+  INB(half + 1, half, max);
+  INB(half, half + 1, max);
+
+  OOB(max, 0, 0);
+  OOB(0, max, 0);
+  OOB(max, 0, 1);
+  OOB(0, max, 1);
+  OOB(max, 0, 2);
+  OOB(0, max, 2);
+
+  OOB(max, 0, max - 1);
+  OOB(0, max, max - 1);
+
+  // wraparound cases.
+  OOB(max, 1, max);
+  OOB(1, max, max);
+  OOB(max - 1, 2, max);
+  OOB(2, max - 1, max);
+  OOB(half + 1, half + 1, max);
+  OOB(half + 1, half + 1, max);
+
+#undef INB
+#undef OOB
+}
+
 }  // namespace internal
 }  // namespace v8