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

This reverts commit 03d5a7ba.

Reason for revert: Needs rebaseline:
https://ci.chromium.org/p/v8/builders/ci/V8%20Blink%20Linux/3243

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}

TBR=ahaas@chromium.org,clemensb@chromium.org

Change-Id: I5a7ea265ce47b9e685a5056bb83db6dc58f774a9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:10126
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2065168Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Commit-Queue: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#66356}

src/wasm/decoder.h

@@ -266,7 +266,6 @@ 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,19 +2191,16 @@ bool AsyncStreamingProcessor::ProcessSection(SectionCode section_code,
                                       NativeModuleCache::WireBytesHash(bytes));
   }
   if (section_code == SectionCode::kUnknownSectionCode) {
-    size_t bytes_consumed = ModuleDecoder::IdentifyUnknownSection(
-        &decoder_, bytes, offset, &section_code);
-    if (!decoder_.ok()) {
-      FinishAsyncCompileJobWithError(decoder_.FinishDecoding(false).error());
-      return false;
-    }
+    Decoder decoder(bytes, offset);
+    section_code = ModuleDecoder::IdentifyUnknownSection(
+        &decoder, bytes.begin() + bytes.length());
     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 += bytes_consumed;
-    bytes = bytes.SubVector(bytes_consumed, bytes.size());
+    offset += decoder.position();
+    bytes = bytes.SubVector(decoder.position(), bytes.size());
   }
   constexpr bool verify_functions = false;
   decoder_.DecodeSection(section_code, bytes, offset, verify_functions);

src/wasm/module-decoder.cc

@@ -149,42 +149,6 @@ 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 {
@@ -268,13 +232,8 @@ class WasmSectionIterator {
     if (section_code == kUnknownSectionCode) {
       // Check for the known "name", "sourceMappingURL", or "compilationHints"
       // section.
-      // 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);
+      section_code =
+          ModuleDecoder::IdentifyUnknownSection(decoder_, section_end_);
       // As a side effect, the above function will forward the decoder to after
       // the identifier string.
       payload_start_ = decoder_->pc();
@@ -2071,14 +2030,39 @@ void ModuleDecoder::set_code_section(uint32_t offset, uint32_t size) {
   return impl_->set_code_section(offset, size);
 }
 
-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();
+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;
 }
 
 bool ModuleDecoder::ok() { return impl_->ok(); }

src/wasm/module-decoder.h

@@ -205,10 +205,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.
-  // 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);
+  // 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);
 
  private:
   const WasmFeatures enabled_features_;

src/wasm/wasm-engine.cc

@@ -254,6 +254,10 @@ 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,6 +160,9 @@
   # 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,22 +1753,12 @@ 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.
-      1,  // section length, only one byte for the name length.
-      0,  // string length of 0.
-          // Empty section name, no content, nothing but sadness.
+      0,  // Empty section name.
+          // No section name, no content, nothing but sadness.
+      0,  // No section content.
   };
   EXPECT_VERIFIES(data);
 }