[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}

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();
@@ -2030,39 +2071,14 @@ void ModuleDecoder::set_code_section(uint32_t offset, uint32_t size) {
   return impl_->set_code_section(offset, size);
 }
 
-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

@@ -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.
-  // 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

@@ -254,10 +254,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);
 }