[wasm-gc] read_heap_type should check if index is in module bounds

read_heap_type did not have knowledge of the module for which the heap
type was being decoded. As a result, callers of read_heap_type (or
read_value_type, which in turn calls read_heap_type) had to check after
the fact that a decoded indexed type (ref, ref null, or rtt) references
a type index within the module's bounds. This was not done consistently,
and was missing (at least) in DecodeLocals.
To avoid such problems in the future, this CL refactors read_heap_type
to accept a module and check the decoded index against it.

Changes:
- Add WasmModule argument to read_heap_type. Do so accordingly to all
  its transitive callers (read_value_type, immediate arguments,
  DecodeLocalDecls, DecodeValue/HeapType in unittests).
- Add index check to read_heap_type and emit an error for an
  out-of-bounds index.
- Remove all other now-redundant index validations. Replace them with
  decoder->ok() if needed (since read_heap_type will now emit an error).
- Fix error message in Validate for BlockTypeImmediate.
- In DecodeLocalDecls in unittests, pass an empty module to
  DecodeLocalDecls in the main code.
- Add a unit test with an invalid index in local type declarations.

Bug: v8:9495
Change-Id: I4ed1204847db80f78b6ae85fa40d300cd2456295
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2569757Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71572}

src/wasm/function-body-decoder.cc

@@ -20,11 +20,12 @@ namespace internal {
 namespace wasm {
 
 bool DecodeLocalDecls(const WasmFeatures& enabled, BodyLocalDecls* decls,
-                      const byte* start, const byte* end) {
+                      const WasmModule* module, const byte* start,
+                      const byte* end) {
   WasmFeatures no_features = WasmFeatures::None();
   Zone* zone = decls->type_list.get_allocator().zone();
   WasmDecoder<Decoder::kFullValidation> decoder(
-      zone, nullptr, enabled, &no_features, nullptr, start, end, 0);
+      zone, module, enabled, &no_features, nullptr, start, end, 0);
   uint32_t length;
   if (decoder.DecodeLocals(decoder.pc(), &length, 0) < 0) {
     decls->encoded_size = 0;
@@ -42,7 +43,7 @@ BytecodeIterator::BytecodeIterator(const byte* start, const byte* end,
                                    BodyLocalDecls* decls)
     : Decoder(start, end) {
   if (decls != nullptr) {
-    if (DecodeLocalDecls(WasmFeatures::All(), decls, start, end)) {
+    if (DecodeLocalDecls(WasmFeatures::All(), decls, nullptr, start, end)) {
       pc_ += decls->encoded_size;
       if (pc_ > end_) pc_ = end_;
     }
@@ -244,7 +245,7 @@ bool PrintRawWasmCode(AccountingAllocator* allocator, const FunctionBody& body,
       case kExprBlock:
       case kExprTry: {
         BlockTypeImmediate<Decoder::kNoValidation> imm(WasmFeatures::All(), &i,
-                                                       i.pc() + 1);
+                                                       i.pc() + 1, module);
         os << " @" << i.pc_offset();
         if (decoder.Complete(imm)) {
           for (uint32_t i = 0; i < imm.out_arity(); i++) {

src/wasm/function-body-decoder.h

@@ -67,6 +67,7 @@ struct BodyLocalDecls {
 
 V8_EXPORT_PRIVATE bool DecodeLocalDecls(const WasmFeatures& enabled,
                                         BodyLocalDecls* decls,
+                                        const WasmModule* module,
                                         const byte* start, const byte* end);
 
 V8_EXPORT_PRIVATE BitVector* AnalyzeLoopAssignmentForTesting(

src/wasm/module-decoder.cc

@@ -1648,22 +1648,6 @@ class ModuleDecoderImpl : public Decoder {
     return true;
   }
 
-  // TODO(manoskouk): This is copy-modified from function-body-decoder-impl.h.
-  // We should find a way to share this code.
-  V8_INLINE bool Validate(const byte* pc,
-                          HeapTypeImmediate<kFullValidation>& imm) {
-    if (V8_UNLIKELY(imm.type.is_bottom())) {
-      error(pc, "invalid heap type");
-      return false;
-    }
-    if (V8_UNLIKELY(!(imm.type.is_generic() ||
-                      module_->has_type(imm.type.ref_index())))) {
-      errorf(pc, "Type index %u is out of bounds", imm.type.ref_index());
-      return false;
-    }
-    return true;
-  }
-
   WasmInitExpr consume_init_expr(WasmModule* module, ValueType expected,
                                  size_t current_global_index) {
     constexpr Decoder::ValidateFlag validate = Decoder::kFullValidation;
@@ -1735,10 +1719,10 @@ class ModuleDecoderImpl : public Decoder {
                    kExprRefNull);
             return {};
           }
-          HeapTypeImmediate<Decoder::kFullValidation> imm(enabled_features_,
-                                                          this, pc() + 1);
+          HeapTypeImmediate<Decoder::kFullValidation> imm(
+              enabled_features_, this, pc() + 1, module_.get());
+          if (V8_UNLIKELY(failed())) return {};
           len = 1 + imm.length;
-          if (!Validate(pc() + 1, imm)) return {};
           stack.push_back(
               WasmInitExpr::RefNullConst(imm.type.representation()));
           break;
@@ -1786,19 +1770,19 @@ class ModuleDecoderImpl : public Decoder {
           opcode = read_prefixed_opcode<validate>(pc(), &len);
           switch (opcode) {
             case kExprRttCanon: {
-              HeapTypeImmediate<validate> imm(enabled_features_, this,
-                                              pc() + 2);
+              HeapTypeImmediate<validate> imm(enabled_features_, this, pc() + 2,
+                                              module_.get());
+              if (V8_UNLIKELY(failed())) return {};
               len += imm.length;
-              if (!Validate(pc() + len, imm)) return {};
               stack.push_back(
                   WasmInitExpr::RttCanon(imm.type.representation()));
               break;
             }
             case kExprRttSub: {
-              HeapTypeImmediate<validate> imm(enabled_features_, this,
-                                              pc() + 2);
+              HeapTypeImmediate<validate> imm(enabled_features_, this, pc() + 2,
+                                              module_.get());
+              if (V8_UNLIKELY(failed())) return {};
               len += imm.length;
-              if (!Validate(pc() + len, imm)) return {};
               if (stack.empty()) {
                 error(pc(), "calling rtt.sub without arguments");
                 return {};
@@ -1870,13 +1854,8 @@ class ModuleDecoderImpl : public Decoder {
   ValueType consume_value_type() {
     uint32_t type_length;
     ValueType result = value_type_reader::read_value_type<kFullValidation>(
-        this, this->pc(), &type_length,
+        this, this->pc(), &type_length, module_.get(),
         origin_ == kWasmOrigin ? enabled_features_ : WasmFeatures::None());
-    // We use capacity() over size() so this function works
-    // mid-DecodeTypeSection.
-    if (result.has_index() && result.ref_index() >= module_->types.capacity()) {
-      errorf(pc(), "Type index %u is out of bounds", result.ref_index());
-    }
     consume_bytes(type_length, "value type");
     return result;
   }
@@ -2166,7 +2145,7 @@ class ModuleDecoderImpl : public Decoder {
     switch (opcode) {
       case kExprRefNull: {
         HeapTypeImmediate<kFullValidation> imm(WasmFeatures::All(), this,
-                                               this->pc());
+                                               this->pc(), module_.get());
         consume_bytes(imm.length, "ref.null immediate");
         index = WasmElemSegment::kNullIndex;
         break;

test/common/wasm/wasm-interpreter.cc

@@ -790,8 +790,8 @@ class SideTable : public ZoneObject {
         case kExprBlock:
         case kExprLoop: {
           bool is_loop = opcode == kExprLoop;
-          BlockTypeImmediate<Decoder::kNoValidation> imm(WasmFeatures::All(),
-                                                         &i, i.pc() + 1);
+          BlockTypeImmediate<Decoder::kNoValidation> imm(
+              WasmFeatures::All(), &i, i.pc() + 1, module);
           if (imm.type == kWasmBottom) {
             imm.sig = module->signature(imm.sig_index);
           }
@@ -812,8 +812,8 @@ class SideTable : public ZoneObject {
           break;
         }
         case kExprIf: {
-          BlockTypeImmediate<Decoder::kNoValidation> imm(WasmFeatures::All(),
-                                                         &i, i.pc() + 1);
+          BlockTypeImmediate<Decoder::kNoValidation> imm(
+              WasmFeatures::All(), &i, i.pc() + 1, module);
           if (imm.type == kWasmBottom) {
             imm.sig = module->signature(imm.sig_index);
           }
@@ -852,8 +852,8 @@ class SideTable : public ZoneObject {
           break;
         }
         case kExprTry: {
-          BlockTypeImmediate<Decoder::kNoValidation> imm(WasmFeatures::All(),
-                                                         &i, i.pc() + 1);
+          BlockTypeImmediate<Decoder::kNoValidation> imm(
+              WasmFeatures::All(), &i, i.pc() + 1, module);
           if (imm.type == kWasmBottom) {
             imm.sig = module->signature(imm.sig_index);
           }
@@ -3262,13 +3262,13 @@ class WasmInterpreterInternals {
         case kExprLoop:
         case kExprTry: {
           BlockTypeImmediate<Decoder::kNoValidation> imm(
-              WasmFeatures::All(), &decoder, code->at(pc + 1));
+              WasmFeatures::All(), &decoder, code->at(pc + 1), module());
           len = 1 + imm.length;
           break;
         }
         case kExprIf: {
           BlockTypeImmediate<Decoder::kNoValidation> imm(
-              WasmFeatures::All(), &decoder, code->at(pc + 1));
+              WasmFeatures::All(), &decoder, code->at(pc + 1), module());
           WasmValue cond = Pop();
           bool is_true = cond.to<uint32_t>() != 0;
           if (is_true) {
@@ -3328,7 +3328,7 @@ class WasmInterpreterInternals {
         }
         case kExprSelectWithType: {
           SelectTypeImmediate<Decoder::kNoValidation> imm(
-              WasmFeatures::All(), &decoder, code->at(pc + 1));
+              WasmFeatures::All(), &decoder, code->at(pc + 1), module());
           len = 1 + imm.length;
           V8_FALLTHROUGH;
         }
@@ -3417,7 +3417,7 @@ class WasmInterpreterInternals {
         }
         case kExprRefNull: {
           HeapTypeImmediate<Decoder::kNoValidation> imm(
-              WasmFeatures::All(), &decoder, code->at(pc + 1));
+              WasmFeatures::All(), &decoder, code->at(pc + 1), module());
           len = 1 + imm.length;
           Push(WasmValue(isolate_->factory()->null_value()));
           break;

test/fuzzer/wasm-fuzzer-common.cc

@@ -269,7 +269,7 @@ void GenerateTestCase(Isolate* isolate, ModuleWireBytes wire_bytes,
 
     // Add locals.
     BodyLocalDecls decls(&tmp_zone);
-    DecodeLocalDecls(enabled_features, &decls, func_code.begin(),
+    DecodeLocalDecls(enabled_features, &decls, module, func_code.begin(),
                      func_code.end());
     if (!decls.type_list.empty()) {
       os << "  ";

test/unittests/wasm/function-body-decoder-unittest.cc

@@ -4663,18 +4663,20 @@ TEST_F(WasmOpcodeLengthTest, PrefixedOpcodesLEB) {
 
 class TypeReaderTest : public TestWithZone {
  public:
-  ValueType DecodeValueType(const byte* start, const byte* end) {
+  ValueType DecodeValueType(const byte* start, const byte* end,
+                            const WasmModule* module) {
     Decoder decoder(start, end);
     uint32_t length;
     return value_type_reader::read_value_type<Decoder::kFullValidation>(
-        &decoder, start, &length, enabled_features_);
+        &decoder, start, &length, module, enabled_features_);
   }
 
-  HeapType DecodeHeapType(const byte* start, const byte* end) {
+  HeapType DecodeHeapType(const byte* start, const byte* end,
+                          const WasmModule* module) {
     Decoder decoder(start, end);
     uint32_t length;
     return value_type_reader::read_heap_type<Decoder::kFullValidation>(
-        &decoder, start, &length, enabled_features_);
+        &decoder, start, &length, module, enabled_features_);
   }
 
   // This variable is modified by WASM_FEATURE_SCOPE.
@@ -4692,34 +4694,34 @@ TEST_F(TypeReaderTest, HeapTypeDecodingTest) {
   // 1- to 5-byte representation of kFuncRefCode.
   {
     const byte data[] = {kFuncRefCode};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_TRUE(result == heap_func);
   }
   {
     const byte data[] = {kFuncRefCode | 0x80, 0x7F};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_EQ(result, heap_func);
   }
   {
     const byte data[] = {kFuncRefCode | 0x80, 0xFF, 0x7F};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_EQ(result, heap_func);
   }
   {
     const byte data[] = {kFuncRefCode | 0x80, 0xFF, 0xFF, 0x7F};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_EQ(result, heap_func);
   }
   {
     const byte data[] = {kFuncRefCode | 0x80, 0xFF, 0xFF, 0xFF, 0x7F};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_EQ(result, heap_func);
   }
 
   {
     // Some negative number.
     const byte data[] = {0xB4, 0x7F};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_EQ(result, heap_bottom);
   }
 
@@ -4728,7 +4730,7 @@ TEST_F(TypeReaderTest, HeapTypeDecodingTest) {
     // range. This should therefore NOT be decoded as HeapType::kFunc and
     // instead fail.
     const byte data[] = {kFuncRefCode | 0x80, 0x6F};
-    HeapType result = DecodeHeapType(data, data + sizeof(data));
+    HeapType result = DecodeHeapType(data, data + sizeof(data), nullptr);
     EXPECT_EQ(result, heap_bottom);
   }
 }
@@ -4750,7 +4752,9 @@ class LocalDeclDecoderTest : public TestWithZone {
 
   bool DecodeLocalDecls(BodyLocalDecls* decls, const byte* start,
                         const byte* end) {
-    return i::wasm::DecodeLocalDecls(enabled_features_, decls, start, end);
+    WasmModule module;
+    return i::wasm::DecodeLocalDecls(enabled_features_, decls, &module, start,
+                                     end);
   }
 };
 
@@ -4877,6 +4881,20 @@ TEST_F(LocalDeclDecoderTest, ExnRef) {
   EXPECT_EQ(type, map[0]);
 }
 
+TEST_F(LocalDeclDecoderTest, InvalidTypeIndex) {
+  WASM_FEATURE_SCOPE(reftypes);
+  WASM_FEATURE_SCOPE(typed_funcref);
+
+  const byte* data = nullptr;
+  const byte* end = nullptr;
+  LocalDeclEncoder local_decls(zone());
+
+  local_decls.AddLocals(1, ValueType::Ref(0, kNullable));
+  BodyLocalDecls decls(zone());
+  bool result = DecodeLocalDecls(&decls, data, end);
+  EXPECT_FALSE(result);
+}
+
 class BytecodeIteratorTest : public TestWithZone {};
 
 TEST_F(BytecodeIteratorTest, SimpleForeach) {