Reland "[wasm] The name of a custom section can cause a validation error"

This is a reland of 03d5a7ba

Nothing changed here compared to the original test. The tests on the
blink side were invalid, I fixed them in https://crrev.com/c/2066907.

Original change's description:
> [wasm] The name of a custom section can cause a validation error
>
> The WebAssembly spec defines that the name of a custom section can cause
> a validation error. The streaming decoder, however, used a separate
> Decoder object to decode the name, and thereby avoided a validation
> error. With this CL the streaming decoder uses the main decoder to
> decode the name of the custom section.
>
> In addition this CL removes the test mjsunit/regress/wasm/regress-789952.
> This test defined an invalid WebAssembly module and expected it to
> compile. As it is a regression test, it makes no sense to fix the test.
> The module is invalid because it defines the length of the custom section
> to be '0', so there are no bytes in the custom section for its name.
>
> R=clemensb@chromium.org
> CC=thibaudm@chromium.org
>
> Bug: v8:10126
> Change-Id: I8cfc77c9a5916570d5362d5922e0179a29774da8
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2041446
> Commit-Queue: Andreas Haas <ahaas@chromium.org>
> Reviewed-by: Clemens Backes <clemensb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#66348}

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

src/wasm/decoder.h

@@ -266,6 +266,7 @@ class Decoder {
     return offset - buffer_offset_;
   }
   const byte* end() const { return end_; }
+  void set_end(const byte* end) { end_ = end; }
 
   // Check if the byte at {offset} from the current pc equals {expected}.
   bool lookahead(int offset, byte expected) {

src/wasm/module-compiler.cc

@@ -2191,16 +2191,19 @@ bool AsyncStreamingProcessor::ProcessSection(SectionCode section_code,
                                       NativeModuleCache::WireBytesHash(bytes));
   }
   if (section_code == SectionCode::kUnknownSectionCode) {
-    Decoder decoder(bytes, offset);
-    section_code = ModuleDecoder::IdentifyUnknownSection(
-        &decoder, bytes.begin() + bytes.length());
+    size_t bytes_consumed = ModuleDecoder::IdentifyUnknownSection(
+        &decoder_, bytes, offset, &section_code);
+    if (!decoder_.ok()) {
+      FinishAsyncCompileJobWithError(decoder_.FinishDecoding(false).error());
+      return false;
+    }
     if (section_code == SectionCode::kUnknownSectionCode) {
       // Skip unknown sections that we do not know how to handle.
       return true;
     }
     // Remove the unknown section tag from the payload bytes.
-    offset += decoder.position();
-    bytes = bytes.SubVector(decoder.position(), bytes.size());
+    offset += bytes_consumed;
+    bytes = bytes.SubVector(bytes_consumed, bytes.size());
   }
   constexpr bool verify_functions = false;
   decoder_.DecodeSection(section_code, bytes, offset, verify_functions);

src/wasm/module-decoder.cc

@@ -149,6 +149,42 @@ WireBytesRef consume_string(Decoder* decoder, bool validate_utf8,
   return {offset, decoder->failed() ? 0 : length};
 }
 
+namespace {
+SectionCode IdentifyUnknownSectionInternal(Decoder* decoder) {
+  WireBytesRef string = consume_string(decoder, true, "section name");
+  if (decoder->failed()) {
+    return kUnknownSectionCode;
+  }
+  const byte* section_name_start =
+      decoder->start() + decoder->GetBufferRelativeOffset(string.offset());
+
+  TRACE("  +%d  section name        : \"%.*s\"\n",
+        static_cast<int>(section_name_start - decoder->start()),
+        string.length() < 20 ? string.length() : 20, section_name_start);
+
+  if (string.length() == num_chars(kNameString) &&
+      strncmp(reinterpret_cast<const char*>(section_name_start), kNameString,
+              num_chars(kNameString)) == 0) {
+    return kNameSectionCode;
+  } else if (string.length() == num_chars(kSourceMappingURLString) &&
+             strncmp(reinterpret_cast<const char*>(section_name_start),
+                     kSourceMappingURLString,
+                     num_chars(kSourceMappingURLString)) == 0) {
+    return kSourceMappingURLSectionCode;
+  } else if (string.length() == num_chars(kCompilationHintsString) &&
+             strncmp(reinterpret_cast<const char*>(section_name_start),
+                     kCompilationHintsString,
+                     num_chars(kCompilationHintsString)) == 0) {
+    return kCompilationHintsSectionCode;
+  } else if (string.length() == num_chars(kDebugInfoString) &&
+             strncmp(reinterpret_cast<const char*>(section_name_start),
+                     kDebugInfoString, num_chars(kDebugInfoString)) == 0) {
+    return kDebugInfoSectionCode;
+  }
+  return kUnknownSectionCode;
+}
+}  // namespace
+
 // An iterator over the sections in a wasm binary module.
 // Automatically skips all unknown sections.
 class WasmSectionIterator {
@@ -232,8 +268,13 @@ class WasmSectionIterator {
     if (section_code == kUnknownSectionCode) {
       // Check for the known "name", "sourceMappingURL", or "compilationHints"
       // section.
-      section_code =
-          ModuleDecoder::IdentifyUnknownSection(decoder_, section_end_);
+      // To identify the unknown section we set the end of the decoder bytes to
+      // the end of the custom section, so that we do not read the section name
+      // beyond the end of the section.
+      const byte* module_end = decoder_->end();
+      decoder_->set_end(section_end_);
+      section_code = IdentifyUnknownSectionInternal(decoder_);
+      if (decoder_->ok()) decoder_->set_end(module_end);
       // As a side effect, the above function will forward the decoder to after
       // the identifier string.
       payload_start_ = decoder_->pc();
@@ -2020,39 +2061,14 @@ ModuleResult ModuleDecoder::FinishDecoding(bool verify_functions) {
   return impl_->FinishDecoding(verify_functions);
 }
 
-SectionCode ModuleDecoder::IdentifyUnknownSection(Decoder* decoder,
-                                                  const byte* end) {
-  WireBytesRef string = consume_string(decoder, true, "section name");
-  if (decoder->failed() || decoder->pc() > end) {
-    return kUnknownSectionCode;
-  }
-  const byte* section_name_start =
-      decoder->start() + decoder->GetBufferRelativeOffset(string.offset());
-
-  TRACE("  +%d  section name        : \"%.*s\"\n",
-        static_cast<int>(section_name_start - decoder->start()),
-        string.length() < 20 ? string.length() : 20, section_name_start);
-
-  if (string.length() == num_chars(kNameString) &&
-      strncmp(reinterpret_cast<const char*>(section_name_start), kNameString,
-              num_chars(kNameString)) == 0) {
-    return kNameSectionCode;
-  } else if (string.length() == num_chars(kSourceMappingURLString) &&
-             strncmp(reinterpret_cast<const char*>(section_name_start),
-                     kSourceMappingURLString,
-                     num_chars(kSourceMappingURLString)) == 0) {
-    return kSourceMappingURLSectionCode;
-  } else if (string.length() == num_chars(kCompilationHintsString) &&
-             strncmp(reinterpret_cast<const char*>(section_name_start),
-                     kCompilationHintsString,
-                     num_chars(kCompilationHintsString)) == 0) {
-    return kCompilationHintsSectionCode;
-  } else if (string.length() == num_chars(kDebugInfoString) &&
-             strncmp(reinterpret_cast<const char*>(section_name_start),
-                     kDebugInfoString, num_chars(kDebugInfoString)) == 0) {
-    return kDebugInfoSectionCode;
-  }
-  return kUnknownSectionCode;
+size_t ModuleDecoder::IdentifyUnknownSection(ModuleDecoder* decoder,
+                                             Vector<const uint8_t> bytes,
+                                             uint32_t offset,
+                                             SectionCode* result) {
+  if (!decoder->ok()) return 0;
+  decoder->impl_->Reset(bytes, offset);
+  *result = IdentifyUnknownSectionInternal(decoder->impl_.get());
+  return decoder->impl_->pc() - bytes.begin();
 }
 
 bool ModuleDecoder::ok() { return impl_->ok(); }

src/wasm/module-decoder.h

@@ -203,10 +203,10 @@ class ModuleDecoder {
   // SectionCode if the unknown section is known to decoder.
   // The decoder is expected to point after the section length and just before
   // the identifier string of the unknown section.
-  // If a SectionCode other than kUnknownSectionCode is returned, the decoder
-  // will point right after the identifier string. Otherwise, the position is
-  // undefined.
-  static SectionCode IdentifyUnknownSection(Decoder* decoder, const byte* end);
+  // The return value is the number of bytes that were consumed.
+  static size_t IdentifyUnknownSection(ModuleDecoder* decoder,
+                                       Vector<const uint8_t> bytes,
+                                       uint32_t offset, SectionCode* result);
 
  private:
   const WasmFeatures enabled_features_;

src/wasm/wasm-engine.cc

@@ -260,10 +260,6 @@ size_t NativeModuleCache::PrefixHash(Vector<const uint8_t> wire_bytes) {
       break;
     }
     const uint8_t* payload_start = decoder.pc();
-    // TODO(v8:10126): Remove this check, bytes have been validated already.
-    if (decoder.position() + section_size > wire_bytes.size()) {
-      return hash;
-    }
     decoder.consume_bytes(section_size, "section payload");
     size_t section_hash = NativeModuleCache::WireBytesHash(
         Vector<const uint8_t>(payload_start, section_size));

test/mjsunit/mjsunit.status

@@ -160,9 +160,6 @@
   # OOM with too many isolates/memory objects (https://crbug.com/1010272)
   # Predictable tests fail due to race between postMessage and GrowMemory
   'regress/wasm/regress-1010272': [PASS, NO_VARIANTS, ['system == android', SKIP], ['predictable', SKIP]],
-
-  # https://crbug.com/v8/10126
-  'regress/wasm/regress-789952': [SKIP]
 }],  # ALWAYS
 
 ##############################################################################

test/mjsunit/regress/wasm/regress-10126-streaming.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --wasm-test-streaming
+
+load('test/mjsunit/regress/wasm/regress-10126.js')

test/mjsunit/regress/wasm/regress-10126.js

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+load('test/mjsunit/wasm/wasm-module-builder.js')
+
+let binary = new Binary;
+binary.emit_bytes([
+  kWasmH0,              //  0 header
+  kWasmH1,              //  1 -
+  kWasmH2,              //  2 -
+  kWasmH3,              //  3 -
+  kWasmV0,              //  4 version
+  kWasmV1,              //  5 -
+  kWasmV2,              //  6 -
+  kWasmV3,              //  7 -
+  kUnknownSectionCode,  //  8 custom section
+  0x5,                  //  9 length
+  0x6,                  // 10 invalid name length
+  'a',                  // 11 payload
+  'b',                  // 12 -
+  'c',                  // 13 -
+  'd',                  // 14 -
+  kCodeSectionCode,     // 15 code section start
+  0x1,                  // 16 code section length
+  19,                   // 17 invalid number of functions
+]);
+
+const buffer = binary.trunc_buffer();
+assertThrowsAsync(
+    WebAssembly.compile(buffer), WebAssembly.CompileError,
+    'WebAssembly.compile(): expected 6 bytes, fell off end @+11');

test/mjsunit/regress/wasm/regress-789952.js

-// Copyright 2017 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-let module_size = 19;
-let string_len = 0x00fffff0 - module_size;
-
-print("Allocating backing store: " + (string_len + module_size));
-let backing = new ArrayBuffer(string_len + module_size);
-
-print("Allocating typed array buffer");
-let buffer = new Uint8Array(backing);
-
-print("Filling...");
-buffer.fill(0x41);
-
-print("Setting up array buffer");
-// Magic
-buffer.set([0x00, 0x61, 0x73, 0x6D], 0);
-// Version
-buffer.set([0x01, 0x00, 0x00, 0x00], 4);
-// kUnknownSection (0)
-buffer.set([0], 8);
-// Section length
-buffer.set([0x80, 0x80, 0x80, 0x80, 0x00],  9);
-// Name length
-let x = string_len + 1;
-let b1 = ((x >> 0) & 0x7F) | 0x80;
-let b2 = ((x >> 7) & 0x7F) | 0x80;
-let b3 = ((x >> 14) & 0x7F) | 0x80;
-let b4 = ((x >> 21) & 0x7F);
-//buffer.set([0xDE, 0xFF, 0xFF, 0x7F], 14);
- buffer.set([b1, b2, b3, b4], 14);
-
-print("Parsing module...");
-let m = new WebAssembly.Module(buffer);
-
-print("Triggering!");
-let c = WebAssembly.Module.customSections(m, "A".repeat(string_len + 1));
-assertEquals(0, c.length);

test/unittests/wasm/module-decoder-unittest.cc

@@ -1753,12 +1753,22 @@ TEST_F(WasmModuleVerifyTest, SectionWithoutNameLength) {
   EXPECT_FAILURE(data);
 }
 
+TEST_F(WasmModuleVerifyTest, EmptyCustomSectionIsInvalid) {
+  // An empty custom section is invalid, because at least one byte for the
+  // length of the custom section name is required.
+  const byte data[] = {
+      0,  // unknown section code.
+      0   // section length.
+  };
+  EXPECT_FAILURE(data);
+}
+
 TEST_F(WasmModuleVerifyTest, TheLoneliestOfValidModulesTheTrulyEmptyOne) {
   const byte data[] = {
       0,  // unknown section code.
-      0,  // Empty section name.
-          // No section name, no content, nothing but sadness.
-      0,  // No section content.
+      1,  // section length, only one byte for the name length.
+      0,  // string length of 0.
+          // Empty section name, no content, nothing but sadness.
   };
   EXPECT_VERIFIES(data);
 }