Revert "[wasm] Allow for arbitrarily long error messages"

This reverts commit 072d0e3e.

Reason for revert: Performance regressions (https://crbug.com/749041).

Original change's description:
> [wasm] Allow for arbitrarily long error messages
> 
> We currently have a fixed limit of 256 characters for error messages
> generated in the decoder. However, we sometimes embed names in it,
> which makes it easy to generate a crash by using long names (e.g. for
> exports) in invalid wasm modules.
> This CL fixes this by switching to a stream based interface, allowing
> to pass arbitrary objects to be printed. With this interface, we can
> easily limit the length of output later.
> 
> R=​titzer@chromium.org
> 
> Bug: chromium:740023
> Change-Id: I2848c31c63a015157e2a3a9458b54e523060cd69
> Reviewed-on: https://chromium-review.googlesource.com/565282
> Reviewed-by: Ben Titzer <titzer@chromium.org>
> Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#46860}

TBR=titzer@chromium.org,clemensh@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:740023, chromium:749041
Change-Id: I005a60d55dcf01d350230f8d98f715bab9c43886
Reviewed-on: https://chromium-review.googlesource.com/593807
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47008}

src/wasm/decoder.h

@@ -37,34 +37,6 @@ namespace wasm {
 // a buffer of bytes.
 class Decoder {
  public:
-  class ErrorFormatter {
-   public:
-    // Allow move, but nothing else.
-    MOVE_ONLY_NO_DEFAULT_CONSTRUCTOR(ErrorFormatter);
-    ~ErrorFormatter() {
-      if (!decoder_) return;
-      DCHECK(decoder_->ok());
-
-      decoder_->error_offset_ = decoder_->pc_offset(pc_);
-      decoder_->error_msg_ = message_builder_.str();
-      decoder_->onFirstError();
-    }
-
-    template <typename T>
-    ErrorFormatter& operator<<(T&& arg) {
-      if (decoder_) message_builder_ << std::forward<T>(arg);
-      return *this;
-    }
-
-   private:
-    friend class Decoder;
-    std::ostringstream message_builder_;
-    const byte* pc_;
-    Decoder* decoder_;
-    ErrorFormatter(const byte* pc, Decoder* decoder)
-        : pc_(pc), decoder_(decoder) {}
-  };
-
   Decoder(const byte* start, const byte* end, uint32_t buffer_offset = 0)
       : start_(start), pc_(start), end_(end), buffer_offset_(buffer_offset) {}
   Decoder(const byte* start, const byte* pc, const byte* end,
@@ -76,7 +48,7 @@ class Decoder {
   inline bool check(const byte* pc, uint32_t length, const char* msg) {
     DCHECK_LE(start_, pc);
     if (V8_UNLIKELY(pc + length > end_)) {
-      error(pc) << msg;
+      error(pc, msg);
       return false;
     }
     return true;
@@ -153,13 +125,13 @@ class Decoder {
   }
 
   // Reads a LEB128 variable-length unsigned 32-bit integer and advances {pc_}.
-  uint32_t consume_u32v(const char* name = "uint32_t") {
+  uint32_t consume_u32v(const char* name = nullptr) {
     uint32_t length = 0;
     return read_leb<uint32_t, true, true, true>(pc_, &length, name);
   }
 
   // Reads a LEB128 variable-length signed 32-bit integer and advances {pc_}.
-  int32_t consume_i32v(const char* name = "int32_t") {
+  int32_t consume_i32v(const char* name = nullptr) {
     uint32_t length = 0;
     return read_leb<int32_t, true, true, true>(pc_, &length, name);
   }
@@ -179,27 +151,40 @@ class Decoder {
   bool checkAvailable(int size) {
     intptr_t pc_overflow_value = std::numeric_limits<intptr_t>::max() - size;
     if (size < 0 || (intptr_t)pc_ > pc_overflow_value) {
-      error(pc_) << "reading " << size << " bytes would underflow/overflow";
+      errorf(pc_, "reading %d bytes would underflow/overflow", size);
       return false;
     } else if (pc_ < start_ || end_ < (pc_ + size)) {
-      error(pc_) << "expected " << size << " bytes, fell off end";
+      errorf(pc_, "expected %d bytes, fell off end", size);
       return false;
     } else {
       return true;
     }
   }
 
-  // ErrorFormatter sets internal error state in its destructor.
-  ErrorFormatter error(const byte* pc = nullptr) {
+  void error(const char* msg) { errorf(pc_, "%s", msg); }
+
+  void error(const byte* pc, const char* msg) { errorf(pc, "%s", msg); }
+
+  // Sets internal error state.
+  void PRINTF_FORMAT(3, 4) errorf(const byte* pc, const char* format, ...) {
     // Only report the first error.
-    if (!ok()) return {nullptr, nullptr};
+    if (!ok()) return;
 #if DEBUG
     if (FLAG_wasm_break_on_decoder_error) {
       base::OS::DebugBreak();
     }
 #endif
-
-    return {pc ? pc : pc_, this};
+    constexpr int kMaxErrorMsg = 256;
+    EmbeddedVector<char, kMaxErrorMsg> buffer;
+    va_list arguments;
+    va_start(arguments, format);
+    int len = VSNPrintF(buffer, format, arguments);
+    CHECK_LT(0, len);
+    va_end(arguments);
+    error_msg_.assign(buffer.start(), len);
+    DCHECK_GE(pc, start_);
+    error_offset_ = static_cast<uint32_t>(pc - start_) + buffer_offset_;
+    onFirstError();
   }
 
   // Behavior triggered on first error, overridden in subclasses.
@@ -248,11 +233,9 @@ class Decoder {
 
   const byte* start() const { return start_; }
   const byte* pc() const { return pc_; }
-  uint32_t pc_offset(const byte* pc) const {
-    DCHECK_GE(pc, start_);
-    return static_cast<uint32_t>(pc - start_) + buffer_offset_;
+  uint32_t pc_offset() const {
+    return static_cast<uint32_t>(pc_ - start_) + buffer_offset_;
   }
-  uint32_t pc_offset() const { return pc_offset(pc_); }
   uint32_t buffer_offset() const { return buffer_offset_; }
   // Takes an offset relative to the module start and returns an offset relative
   // to the current buffer of the decoder.
@@ -335,7 +318,7 @@ class Decoder {
     *length = byte_index + (at_end ? 0 : 1);
     if (checked && (at_end || (b & 0x80))) {
       TRACE_IF(trace, at_end ? "<end> " : "<length overflow> ");
-      error(pc) << "expected " << name;
+      errorf(pc, "expected %s", name);
       result = 0;
     }
     if (is_last_byte) {
@@ -355,7 +338,7 @@ class Decoder {
       if (!checked) {
         DCHECK(valid_extra_bits);
       } else if (!valid_extra_bits) {
-        error(pc) << "extra bits in varint";
+        error(pc, "extra bits in varint");
         result = 0;
       }
     }

src/wasm/function-body-decoder-impl.h

@@ -101,11 +101,11 @@ struct BlockTypeOperand {
     } else {
       // Handle multi-value blocks.
       if (!CHECKED_COND(FLAG_experimental_wasm_mv)) {
-        decoder->error(pc + 1) << "invalid block arity > 1";
+        decoder->error(pc + 1, "invalid block arity > 1");
         return;
       }
       if (!CHECKED_COND(val == kMultivalBlock)) {
-        decoder->error(pc + 1) << "invalid block type";
+        decoder->error(pc + 1, "invalid block type");
         return;
       }
       // Decode and check the types vector of the block.
@@ -123,7 +123,7 @@ struct BlockTypeOperand {
         val = decoder->read_u8<checked>(pc + offset, "block type");
         decode_local_type(val, &type);
         if (!CHECKED_COND(type != kWasmStmt)) {
-          decoder->error(pc + offset) << "invalid block type";
+          decoder->error(pc + offset, "invalid block type");
           return;
         }
       }
@@ -187,8 +187,8 @@ struct CallIndirectOperand {
     index = decoder->read_u32v<checked>(pc + 1, &len, "signature index");
     table_index = decoder->read_u8<checked>(pc + 1 + len, "table index");
     if (!CHECKED_COND(table_index == 0)) {
-      decoder->error(pc + 1 + len)
-          << "expected table index 0, found " << table_index;
+      decoder->errorf(pc + 1 + len, "expected table index 0, found %u",
+                      table_index);
     }
     length = 1 + len;
   }
@@ -211,7 +211,7 @@ struct MemoryIndexOperand {
   inline MemoryIndexOperand(Decoder* decoder, const byte* pc) {
     index = decoder->read_u8<checked>(pc + 1, "memory index");
     if (!CHECKED_COND(index == 0)) {
-      decoder->error(pc + 1) << "expected memory index 0, found " << index;
+      decoder->errorf(pc + 1, "expected memory index 0, found %u", index);
     }
   }
 };
@@ -279,9 +279,10 @@ struct MemoryAccessOperand {
     alignment =
         decoder->read_u32v<checked>(pc + 1, &alignment_length, "alignment");
     if (!CHECKED_COND(alignment <= max_alignment)) {
-      decoder->error(pc + 1)
-          << "invalid alignment; expected maximum alignment is "
-          << max_alignment << ", actual alignment is " << alignment;
+      decoder->errorf(pc + 1,
+                      "invalid alignment; expected maximum alignment is %u, "
+                      "actual alignment is %u",
+                      max_alignment, alignment);
     }
     unsigned offset_length;
     offset = decoder->read_u32v<checked>(pc + 1 + alignment_length,

src/wasm/streaming-decoder.cc

@@ -237,31 +237,35 @@ StreamingDecoder::DecodeVarInt32::Next(StreamingDecoder* streaming) {
     return nullptr;
   }
   if (value() > max_value_) {
-    streaming->decoder()->error(buffer())
-        << "size > maximum function size: " << value();
+    streaming->decoder()->errorf(buffer(), "size > maximum function size: %zu",
+                                 value());
     return nullptr;
   }
 
   return NextWithValue(streaming);
 }
 
+#define BYTES(x) (x & 0xff), (x >> 8) & 0xff, (x >> 16) & 0xff, (x >> 24) & 0xff
 // Decode the module header. The error state of the decoder stores the result.
 void StreamingDecoder::DecodeModuleHeader::CheckHeader(Decoder* decoder) {
   // TODO(ahaas): Share code with the module-decoder.
   decoder->Reset(buffer(), buffer() + size());
   uint32_t magic_word = decoder->consume_u32("wasm magic");
   if (magic_word != kWasmMagic) {
-    decoder->error(buffer())
-        << "expected magic word " << AsHexBytes(kWasmMagic, 4) << ", found "
-        << AsHexBytes(magic_word, 4);
+    decoder->errorf(buffer(),
+                    "expected magic word %02x %02x %02x %02x, "
+                    "found %02x %02x %02x %02x",
+                    BYTES(kWasmMagic), BYTES(magic_word));
   }
   uint32_t magic_version = decoder->consume_u32("wasm version");
   if (magic_version != kWasmVersion) {
-    decoder->error(buffer())
-        << "expected version " << AsHexBytes(kWasmVersion, 4) << ", found "
-        << AsHexBytes(magic_version, 4);
+    decoder->errorf(buffer(),
+                    "expected version %02x %02x %02x %02x, "
+                    "found %02x %02x %02x %02x",
+                    BYTES(kWasmVersion), BYTES(magic_version));
   }
 }
+#undef BYTES
 
 std::unique_ptr<StreamingDecoder::DecodingState>
 StreamingDecoder::DecodeModuleHeader::Next(StreamingDecoder* streaming) {
@@ -305,7 +309,7 @@ StreamingDecoder::DecodeNumberOfFunctions::NextWithValue(
     memcpy(section_buffer_->bytes() + section_buffer_->payload_offset(),
            buffer(), bytes_needed());
   } else {
-    streaming->decoder()->error() << "Invalid code section length";
+    streaming->decoder()->error("Invalid code section length");
     return base::make_unique<DecodeSectionID>();
   }
 
@@ -326,17 +330,17 @@ StreamingDecoder::DecodeFunctionLength::NextWithValue(
   if (section_buffer_->length() >= buffer_offset_ + bytes_needed()) {
     memcpy(section_buffer_->bytes() + buffer_offset_, buffer(), bytes_needed());
   } else {
-    streaming->decoder()->error() << "Invalid code section length";
+    streaming->decoder()->error("Invalid code section length");
     return base::make_unique<DecodeSectionID>();
   }
 
   // {value} is the length of the function.
   if (value() == 0) {
-    streaming->decoder()->error(buffer()) << "Invalid function length (0)";
+    streaming->decoder()->errorf(buffer(), "Invalid function length (0)");
     return nullptr;
   } else if (buffer_offset() + bytes_needed() + value() >
              section_buffer()->length()) {
-    streaming->decoder()->error(buffer()) << "not enough code section bytes";
+    streaming->decoder()->errorf(buffer(), "not enough code section bytes");
     return nullptr;
   }
 
@@ -356,9 +360,9 @@ StreamingDecoder::DecodeFunctionBody::Next(StreamingDecoder* streaming) {
       streaming->decoder()->Reset(
           section_buffer()->bytes(),
           section_buffer()->bytes() + section_buffer()->length());
-      streaming->decoder()->error(section_buffer()->bytes() + buffer_offset() +
-                                  size())
-          << "not all code section bytes were used";
+      streaming->decoder()->errorf(
+          section_buffer()->bytes() + buffer_offset() + size(),
+          "not all code section bytes were used");
       return nullptr;
     }
     return base::make_unique<DecodeSectionID>();

test/mjsunit/wasm/export-global.js

@@ -56,20 +56,3 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
       () => builder.instantiate(), WebAssembly.CompileError,
       /Duplicate export name 'foo' for global 0 and function 0/);
 })();
-
-(function veryLongExportName() {
-  // Regression test for crbug.com/740023.
-  var export_name = 'abc';
-  while (export_name.length < 8192) {
-    export_name = export_name.concat(export_name);
-  }
-  var builder = new WasmModuleBuilder();
-  var global = builder.addGlobal(kWasmI64, false);
-  global.exportAs(export_name);
-  global.exportAs(export_name);
-  var error_msg =
-      'Duplicate export name \'' + export_name + '\' for global 0 and global 0';
-  assertThrows(
-      () => builder.instantiate(), WebAssembly.CompileError,
-      new RegExp(error_msg));
-})();