[wasm][streaming] Check section order of code section

The streaming decoder did not properly check the ordering of sections
relative to the code section.
This CL fixes that for both empty and non-empty code sections.
The special path for empty code sections is not actually needed, so
remove it to simplify code paths.

Drive-by:
1. Refactor the existing code for checking section ordering to make it
   more structured and readable.
2. Ensure that we either call {DecodeCodeSection} or {StartCodeSection},
   but not both.
3. Remove {set_code_section}, merge it into {StartCodeSection}.
4. Simplify calls to {CalculateGlobalOffsets} (make them unconditional
   and remove one redundant one).

R=ahaas@chromium.org

Bug: chromium:1336380
Change-Id: Ia2c5c115d43d2b5315e3b3c9e4a21175a36aa326
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3747860Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81554}

src/wasm/module-compiler.cc

@@ -2820,11 +2820,9 @@ bool AsyncStreamingProcessor::ProcessCodeSectionHeader(
     FinishAsyncCompileJobWithError(decoder_.FinishDecoding(false).error());
     return false;
   }
-  if (num_functions == 0) return true;
 
-  decoder_.StartCodeSection();
-  decoder_.set_code_section(code_section_start,
-                            static_cast<uint32_t>(code_section_length));
+  decoder_.StartCodeSection({static_cast<uint32_t>(code_section_start),
+                             static_cast<uint32_t>(code_section_length)});
 
   if (!GetWasmEngine()->GetStreamingCompilationOwnership(prefix_hash_)) {
     // Known prefix, wait until the end of the stream and check the cache.

src/wasm/module-decoder-impl.h

@@ -321,109 +321,78 @@ class ModuleDecoderImpl : public Decoder {
 #undef BYTES
   }
 
-  bool CheckSectionOrder(SectionCode section_code,
-                         SectionCode prev_section_code,
-                         SectionCode next_section_code) {
-    if (next_ordered_section_ > next_section_code) {
-      errorf(pc(), "The %s section must appear before the %s section",
-             SectionName(section_code), SectionName(next_section_code));
-      return false;
-    }
-    if (next_ordered_section_ <= prev_section_code) {
-      next_ordered_section_ = prev_section_code + 1;
+  void CheckSectionOrder(SectionCode section_code) {
+    // Check the order of ordered sections.
+    if (section_code >= kFirstSectionInModule &&
+        section_code < kFirstUnorderedSection) {
+      if (section_code < next_ordered_section_) {
+        errorf(pc(), "unexpected section <%s>", SectionName(section_code));
       }
-    return true;
+      next_ordered_section_ = section_code + 1;
+      return;
     }
 
-  bool CheckUnorderedSection(SectionCode section_code) {
+    // Ignore ordering problems in unknown / custom sections. Even allow them to
+    // appear multiple times. As optional sections we use them on a "best
+    // effort" basis.
+    if (section_code == kUnknownSectionCode) return;
+    if (section_code > kLastKnownModuleSection) return;
+
+    // The rest is standardized unordered sections; they are checked more
+    // thoroughly..
+    DCHECK_LE(kFirstUnorderedSection, section_code);
+    DCHECK_GE(kLastKnownModuleSection, section_code);
+
+    // Check that unordered sections don't appear multiple times.
     if (has_seen_unordered_section(section_code)) {
       errorf(pc(), "Multiple %s sections not allowed",
              SectionName(section_code));
-      return false;
     }
     set_seen_unordered_section(section_code);
-    return true;
-  }
-
-  void DecodeSection(SectionCode section_code,
-                     base::Vector<const uint8_t> bytes, uint32_t offset,
-                     bool verify_functions = true) {
-    if (failed()) return;
-    Reset(bytes, offset);
-    TRACE("Section: %s\n", SectionName(section_code));
-    TRACE("Decode Section %p - %p\n", bytes.begin(), bytes.end());
 
-    // Check if the section is out-of-order.
-    if (section_code < next_ordered_section_ &&
-        section_code < kFirstUnorderedSection) {
-      errorf(pc(), "unexpected section <%s>", SectionName(section_code));
-      return;
+    // Define a helper to ensure that sections <= {before} appear before the
+    // current unordered section, and everything >= {after} appears after it.
+    auto check_order = [this, section_code](SectionCode before,
+                                            SectionCode after) {
+      DCHECK_LT(before, after);
+      if (next_ordered_section_ > after) {
+        errorf(pc(), "The %s section must appear before the %s section",
+               SectionName(section_code), SectionName(after));
       }
+      if (next_ordered_section_ <= before) next_ordered_section_ = before + 1;
+    };
 
+    // Now check the ordering constraints of specific unordered sections.
     switch (section_code) {
-      case kUnknownSectionCode:
-        break;
       case kDataCountSectionCode:
-        if (!CheckUnorderedSection(section_code)) return;
-        // If wasm-gc is enabled, we allow the data cound section anywhere in
+        // If wasm-gc is enabled, we allow the data count section anywhere in
         // the module.
-        if (!enabled_features_.has_gc() &&
-            !CheckSectionOrder(section_code, kElementSectionCode,
-                               kCodeSectionCode)) {
-          return;
+        if (!enabled_features_.has_gc()) {
+          check_order(kElementSectionCode, kCodeSectionCode);
         }
         break;
-      case kTagSectionCode: {
-        if (!CheckUnorderedSection(section_code)) return;
-        if (!CheckSectionOrder(section_code, kMemorySectionCode,
-                               kGlobalSectionCode)) {
-          return;
-        }
+      case kTagSectionCode:
+        check_order(kMemorySectionCode, kGlobalSectionCode);
         break;
-      }
-      case kStringRefSectionCode: {
+      case kStringRefSectionCode:
         // TODO(12868): If there's a tag section, assert that we're after the
         // tag section.
-        if (!CheckUnorderedSection(section_code)) return;
-        if (!CheckSectionOrder(section_code, kMemorySectionCode,
-                               kGlobalSectionCode)) {
-          return;
-        }
-        break;
-      }
-      case kNameSectionCode:
-        // TODO(titzer): report out of place name section as a warning.
-        // Be lenient with placement of name section. All except first
-        // occurrence are ignored.
-      case kSourceMappingURLSectionCode:
-        // sourceMappingURL is a custom section and currently can occur anywhere
-        // in the module. In case of multiple sourceMappingURL sections, all
-        // except the first occurrence are ignored.
-      case kDebugInfoSectionCode:
-        // .debug_info is a custom section containing core DWARF information
-        // if produced by compiler. Its presence likely means that Wasm was
-        // built in a debug mode.
-      case kExternalDebugInfoSectionCode:
-        // external_debug_info is a custom section containing a reference to an
-        // external symbol file.
-      case kCompilationHintsSectionCode:
-        // TODO(frgossen): report out of place compilation hints section as a
-        // warning.
-        // Be lenient with placement of compilation hints section. All except
-        // first occurrence after function section and before code section are
-        // ignored.
-        break;
-      case kBranchHintsSectionCode:
-        // TODO(yuri): report out of place branch hints section as a
-        // warning.
-        // Be lenient with placement of compilation hints section. All except
-        // first occurrence after function section and before code section are
-        // ignored.
+        check_order(kMemorySectionCode, kGlobalSectionCode);
         break;
       default:
-        next_ordered_section_ = section_code + 1;
         break;
     }
+  }
+
+  void DecodeSection(SectionCode section_code,
+                     base::Vector<const uint8_t> bytes, uint32_t offset,
+                     bool verify_functions = true) {
+    if (failed()) return;
+    Reset(bytes, offset);
+    TRACE("Section: %s\n", SectionName(section_code));
+    TRACE("Decode Section %p - %p\n", bytes.begin(), bytes.end());
+
+    CheckSectionOrder(section_code);
 
     switch (section_code) {
       case kUnknownSectionCode:
@@ -956,7 +925,6 @@ class ModuleDecoderImpl : public Decoder {
       ConstantExpression init = consume_init_expr(module_.get(), type);
       module_->globals.push_back({type, mutability, init, {0}, false, false});
     }
-    if (ok()) CalculateGlobalOffsets(module_.get());
   }
 
   void DecodeExportSection() {
@@ -1099,7 +1067,9 @@ class ModuleDecoderImpl : public Decoder {
   }
 
   void DecodeCodeSection(bool verify_functions) {
-    StartCodeSection();
+    // Make sure global offset were calculated before they get accessed during
+    // function compilation.
+    CalculateGlobalOffsets(module_.get());
     uint32_t code_section_start = pc_offset();
     uint32_t functions_count = consume_u32v("functions count");
     CheckFunctionsCount(functions_count, code_section_start);
@@ -1117,15 +1087,15 @@ class ModuleDecoderImpl : public Decoder {
       DecodeFunctionBody(i, size, offset, verify_functions);
     }
     DCHECK_GE(pc_offset(), code_section_start);
-    set_code_section(code_section_start, pc_offset() - code_section_start);
+    module_->code = {code_section_start, pc_offset() - code_section_start};
   }
 
-  void StartCodeSection() {
-    if (ok()) {
+  void StartCodeSection(WireBytesRef section_bytes) {
+    CheckSectionOrder(kCodeSectionCode);
     // Make sure global offset were calculated before they get accessed during
     // function compilation.
     CalculateGlobalOffsets(module_.get());
-    }
+    module_->code = section_bytes;
   }
 
   bool CheckFunctionsCount(uint32_t functions_count, uint32_t error_offset) {
@@ -1512,10 +1482,6 @@ class ModuleDecoderImpl : public Decoder {
     return result;
   }
 
-  void set_code_section(uint32_t offset, uint32_t size) {
-    module_->code = {offset, size};
-  }
-
   // Decodes an entire module.
   ModuleResult DecodeModule(Counters* counters, AccountingAllocator* allocator,
                             bool verify_functions = true) {

src/wasm/module-decoder.cc

@@ -168,7 +168,9 @@ void ModuleDecoder::DecodeFunctionBody(uint32_t index, uint32_t length,
   impl_->DecodeFunctionBody(index, length, offset, verify_functions);
 }
 
-void ModuleDecoder::StartCodeSection() { impl_->StartCodeSection(); }
+void ModuleDecoder::StartCodeSection(WireBytesRef section_bytes) {
+  impl_->StartCodeSection(section_bytes);
+}
 
 bool ModuleDecoder::CheckFunctionsCount(uint32_t functions_count,
                                         uint32_t error_offset) {
@@ -179,10 +181,6 @@ ModuleResult ModuleDecoder::FinishDecoding(bool verify_functions) {
   return impl_->FinishDecoding(verify_functions);
 }
 
-void ModuleDecoder::set_code_section(uint32_t offset, uint32_t size) {
-  return impl_->set_code_section(offset, size);
-}
-
 size_t ModuleDecoder::IdentifyUnknownSection(ModuleDecoder* decoder,
                                              base::Vector<const uint8_t> bytes,
                                              uint32_t offset,

src/wasm/module-decoder.h

@@ -152,7 +152,7 @@ class ModuleDecoder {
                      base::Vector<const uint8_t> bytes, uint32_t offset,
                      bool verify_functions = true);
 
-  void StartCodeSection();
+  void StartCodeSection(WireBytesRef section_bytes);
 
   bool CheckFunctionsCount(uint32_t functions_count, uint32_t error_offset);
 
@@ -161,8 +161,6 @@ class ModuleDecoder {
 
   ModuleResult FinishDecoding(bool verify_functions = true);
 
-  void set_code_section(uint32_t offset, uint32_t size);
-
   const std::shared_ptr<WasmModule>& shared_module() const;
 
   WasmModule* module() const { return shared_module().get(); }

src/wasm/wasm-module.h

@@ -41,8 +41,8 @@ class ErrorThrower;
 // Reference to a string in the wire bytes.
 class WireBytesRef {
  public:
-  WireBytesRef() : WireBytesRef(0, 0) {}
-  WireBytesRef(uint32_t offset, uint32_t length)
+  constexpr WireBytesRef() = default;
+  constexpr WireBytesRef(uint32_t offset, uint32_t length)
       : offset_(offset), length_(length) {
     DCHECK_IMPLIES(offset_ == 0, length_ == 0);
     DCHECK_LE(offset_, offset_ + length_);  // no uint32_t overflow.
@@ -55,8 +55,8 @@ class WireBytesRef {
   bool is_set() const { return offset_ != 0; }
 
  private:
-  uint32_t offset_;
-  uint32_t length_;
+  uint32_t offset_ = 0;
+  uint32_t length_ = 0;
 };
 
 // Static representation of a wasm function.

test/cctest/wasm/test-streaming-compilation.cc

@@ -176,6 +176,9 @@ class TestResolver : public CompilationResultResolver {
     Handle<String> str =
         Object::ToString(isolate_, error_reason).ToHandleChecked();
     error_message_->assign(str->ToCString().get());
+    // Print the error message, for easier debugging on tests that unexpectedly
+    // fail compilation.
+    PrintF("Compilation failed: %s\n", error_message_->c_str());
   }
 
  private:
@@ -590,6 +593,58 @@ STREAM_TEST(TestErrorInCodeSectionDetectedByModuleDecoder) {
   CHECK(tester.IsPromiseRejected());
 }
 
+STREAM_TEST(TestSectionOrderErrorWithEmptyCodeSection) {
+  // Valid: Export, then Code.
+  const uint8_t valid[] = {WASM_MODULE_HEADER, SECTION(Export, ENTRY_COUNT(0)),
+                           SECTION(Code, ENTRY_COUNT(0))};
+  // Invalid: Code, then Export.
+  const uint8_t invalid[] = {WASM_MODULE_HEADER, SECTION(Code, ENTRY_COUNT(0)),
+                             SECTION(Export, ENTRY_COUNT(0))};
+
+  StreamTester tester_valid(isolate);
+  tester_valid.OnBytesReceived(valid, arraysize(valid));
+  tester_valid.FinishStream();
+  tester_valid.RunCompilerTasks();
+  CHECK(tester_valid.IsPromiseFulfilled());
+
+  StreamTester tester_invalid(isolate);
+  tester_invalid.OnBytesReceived(invalid, arraysize(invalid));
+  tester_invalid.FinishStream();
+  tester_invalid.RunCompilerTasks();
+  CHECK(tester_invalid.IsPromiseRejected());
+  CHECK_NE(std::string::npos,
+           tester_invalid.error_message().find("unexpected section <Export>"));
+}
+
+STREAM_TEST(TestSectionOrderErrorWithNonEmptyCodeSection) {
+  // Valid: Export, then Code.
+  const uint8_t valid[] = {
+      WASM_MODULE_HEADER, SECTION(Type, ENTRY_COUNT(1), SIG_ENTRY_v_v),
+      SECTION(Function, ENTRY_COUNT(1), SIG_INDEX(0)),
+      SECTION(Export, ENTRY_COUNT(0)),
+      SECTION(Code, ENTRY_COUNT(1), ADD_COUNT(WASM_NO_LOCALS, kExprEnd))};
+  // Invalid: Code, then Export.
+  const uint8_t invalid[] = {
+      WASM_MODULE_HEADER, SECTION(Type, ENTRY_COUNT(1), SIG_ENTRY_v_v),
+      SECTION(Function, ENTRY_COUNT(1), SIG_INDEX(0)),
+      SECTION(Code, ENTRY_COUNT(1), ADD_COUNT(WASM_NO_LOCALS, kExprEnd)),
+      SECTION(Export, ENTRY_COUNT(0))};
+
+  StreamTester tester_valid(isolate);
+  tester_valid.OnBytesReceived(valid, arraysize(valid));
+  tester_valid.FinishStream();
+  tester_valid.RunCompilerTasks();
+  CHECK(tester_valid.IsPromiseFulfilled());
+
+  StreamTester tester_invalid(isolate);
+  tester_invalid.OnBytesReceived(invalid, arraysize(invalid));
+  tester_invalid.FinishStream();
+  tester_invalid.RunCompilerTasks();
+  CHECK(tester_invalid.IsPromiseRejected());
+  CHECK_NE(std::string::npos,
+           tester_invalid.error_message().find("unexpected section <Export>"));
+}
+
 // Test an error in the code section, found by the StreamingDecoder. The error
 // is an invalid function body size, so that there are not enough bytes in the
 // code section for the function body.

test/inspector/debugger/wasm-script-code-offset-streaming-expected.txt

@@ -4,7 +4,7 @@ Running test: test
 Wasm script parsed: ID 0, startColumn: 0, endColumn: 29, codeOffset: 0
 Wasm script parsed: ID 1, startColumn: 0, endColumn: 29, codeOffset: 0
 Wasm script parsed: ID 2, startColumn: 0, endColumn: 32, codeOffset: 31
-Wasm script parsed: ID 3, startColumn: 0, endColumn: 32, codeOffset: 0
+Wasm script parsed: ID 3, startColumn: 0, endColumn: 32, codeOffset: 31
 Wasm script parsed: ID 4, startColumn: 0, endColumn: 40, codeOffset: 36
 Wasm script parsed: ID 5, startColumn: 0, endColumn: 40, codeOffset: 36
 Wasm script parsed: ID 6, startColumn: 0, endColumn: 44, codeOffset: 36