[wasm] Name section: skip duplicate subsections

Duplicate subsections in the name section are disallowed by the spec.
Since the whole name section is optional, we shouldn't fail validation
because of it, but we'll ignore duplicate subsections.

Drive-by cleanup: reduce code duplication by reusing DecodeNameMap from
DecodeIndirectNameMap.

Fixed: chromium:1342338
Change-Id: Icae14c27a0255c6107517354f07ec8eb78d2a7b1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3751211
Auto-Submit: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Manos Koukoutos <manoskouk@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81608}

src/wasm/module-decoder.cc

@@ -372,19 +372,9 @@ void DecodeIndirectNameMap(IndirectNameMap& target, Decoder& decoder) {
     uint32_t outer_index = decoder.consume_u32v("outer index");
     if (outer_index > IndirectNameMap::kMaxKey) continue;
     NameMap names;
-    uint32_t inner_count = decoder.consume_u32v("inner count");
-    for (uint32_t k = 0; k < inner_count; ++k) {
-      uint32_t inner_index = decoder.consume_u32v("inner index");
-      WireBytesRef name =
-          consume_string(&decoder, unibrow::Utf8Variant::kLossyUtf8, "name");
-      if (!decoder.ok()) break;
-      if (inner_index > NameMap::kMaxKey) continue;
-      if (name.is_empty()) continue;  // Empty names are useless.
-      if (!validate_utf8(&decoder, name)) continue;
-      names.Put(inner_index, name);
-    }
-    names.FinishInitialization();
+    DecodeNameMap(names, decoder);
     target.Put(outer_index, std::move(names));
+    if (!decoder.ok()) break;
   }
   target.FinishInitialization();
 }
@@ -408,6 +398,13 @@ void DecodeFunctionNames(const byte* module_start, const byte* module_end,
       }
       // We need to allow empty function names for spec-conformant stack traces.
       DecodeNameMap(names, decoder, kAllowEmptyNames);
+      // The spec allows only one occurrence of each subsection. We could be
+      // more permissive and allow repeated subsections; in that case we'd
+      // have to delay calling {target.FinishInitialization()} on the function
+      // names map until we've seen them all.
+      // For now, we stop decoding after finding the first function names
+      // subsection.
+      return;
     }
   }
 }
@@ -432,45 +429,59 @@ DecodedNameSection::DecodedNameSection(base::Vector<const uint8_t> wire_bytes,
         decoder.consume_bytes(name_payload_len);
         break;
       case kLocalCode:
+        if (local_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmFunctions <= IndirectNameMap::kMaxKey);
         static_assert(kV8MaxWasmFunctionLocals <= NameMap::kMaxKey);
         DecodeIndirectNameMap(local_names_, decoder);
         break;
       case kLabelCode:
+        if (label_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmFunctions <= IndirectNameMap::kMaxKey);
         static_assert(kV8MaxWasmFunctionSize <= NameMap::kMaxKey);
         DecodeIndirectNameMap(label_names_, decoder);
         break;
       case kTypeCode:
+        if (type_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmTypes <= NameMap::kMaxKey);
         DecodeNameMap(type_names_, decoder);
         break;
       case kTableCode:
+        if (table_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmTables <= NameMap::kMaxKey);
         DecodeNameMap(table_names_, decoder);
         break;
       case kMemoryCode:
+        if (memory_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmMemories <= NameMap::kMaxKey);
         DecodeNameMap(memory_names_, decoder);
         break;
       case kGlobalCode:
+        if (global_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmGlobals <= NameMap::kMaxKey);
         DecodeNameMap(global_names_, decoder);
         break;
       case kElementSegmentCode:
+        if (element_segment_names_.is_set()) {
+          decoder.consume_bytes(name_payload_len);
+        }
         static_assert(kV8MaxWasmTableInitEntries <= NameMap::kMaxKey);
         DecodeNameMap(element_segment_names_, decoder);
         break;
       case kDataSegmentCode:
+        if (data_segment_names_.is_set()) {
+          decoder.consume_bytes(name_payload_len);
+        }
         static_assert(kV8MaxWasmDataSegments <= NameMap::kMaxKey);
         DecodeNameMap(data_segment_names_, decoder);
         break;
       case kFieldCode:
+        if (field_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmTypes <= IndirectNameMap::kMaxKey);
         static_assert(kV8MaxWasmStructFields <= NameMap::kMaxKey);
         DecodeIndirectNameMap(field_names_, decoder);
         break;
       case kTagCode:
+        if (tag_names_.is_set()) decoder.consume_bytes(name_payload_len);
         static_assert(kV8MaxWasmTags <= NameMap::kMaxKey);
         DecodeNameMap(tag_names_, decoder);
         break;

test/mjsunit/regress/wasm/regress-crbug-1341180.js

@@ -34,3 +34,42 @@ let bytes = Uint8Array.from([
 assertThrows(
     () => new WebAssembly.Instance(new WebAssembly.Module(bytes)),
     WebAssembly.CompileError);
+
+// Variant found in crbug.com/1342338: duplicate "function names" subsection.
+// The spec doesn't allow this, but we shouldn't crash when it happens.
+let duplicate_funcname_subsection = Uint8Array.from([
+  0x00, 0x61, 0x73, 0x6d,  // wasm magic
+  0x01, 0x00, 0x00, 0x00,  // wasm version
+
+  0x01, 0x04,              // Type section, 4 bytes
+  0x01,                    // types count 1
+  0x60, 0x00, 0x00,        // kind: func, no params, no returns
+
+  0x03, 0x02,              // Function section, 2 bytes
+  0x01,                    // functions count 1
+  0x00,                    // func0: type0 (result i32)
+
+  0x0a, 0x02,              // Code section, 2 bytes
+  0x01,                    // functions count 1
+                           // function #0
+  0x00,                    // body size 0 (which is invalid, which causes
+                           // lazy decoding of the name section in order to
+                           // produce an error message)
+
+  0x00, 0x0f,              // Unknown section, 15 bytes
+  0x04,                    // section name length: 4
+  0x6e, 0x61, 0x6d, 0x65,  // section name: name
+  0x01,                    // name type: function
+  0x03,                    // payload length: 3
+  0x01,                    // names count 1
+  0x00, 0x00,              // index 0 name length: 0
+  0x01,                    // name type: function
+  0x03,                    // payload length: 3
+  0x01,                    // names count 1
+  0x01, 0x00,              // index 1 name length: 0
+]);
+
+assertThrows(
+    () => new WebAssembly.Instance(
+        new WebAssembly.Module(duplicate_funcname_subsection)),
+    WebAssembly.CompileError);