[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/565282Reviewed-by: Ben Titzer <titzer@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46860}

src/wasm/decoder.h

@@ -37,6 +37,34 @@ 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,
@@ -48,7 +76,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;
@@ -125,13 +153,13 @@ class Decoder {
   }
 
   // Reads a LEB128 variable-length unsigned 32-bit integer and advances {pc_}.
-  uint32_t consume_u32v(const char* name = nullptr) {
+  uint32_t consume_u32v(const char* name = "uint32_t") {
     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 = nullptr) {
+  int32_t consume_i32v(const char* name = "int32_t") {
     uint32_t length = 0;
     return read_leb<int32_t, true, true, true>(pc_, &length, name);
   }
@@ -151,40 +179,27 @@ 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) {
-      errorf(pc_, "reading %d bytes would underflow/overflow", size);
+      error(pc_) << "reading " << size << " bytes would underflow/overflow";
       return false;
     } else if (pc_ < start_ || end_ < (pc_ + size)) {
-      errorf(pc_, "expected %d bytes, fell off end", size);
+      error(pc_) << "expected " << size << " bytes, fell off end";
       return false;
     } else {
       return true;
     }
   }
 
-  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, ...) {
+  // ErrorFormatter sets internal error state in its destructor.
+  ErrorFormatter error(const byte* pc = nullptr) {
     // Only report the first error.
-    if (!ok()) return;
+    if (!ok()) return {nullptr, nullptr};
 #if DEBUG
     if (FLAG_wasm_break_on_decoder_error) {
       base::OS::DebugBreak();
     }
 #endif
-    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();
+
+    return {pc ? pc : pc_, this};
   }
 
   // Behavior triggered on first error, overridden in subclasses.
@@ -233,9 +248,11 @@ class Decoder {
 
   const byte* start() const { return start_; }
   const byte* pc() const { return pc_; }
-  uint32_t pc_offset() const {
-    return static_cast<uint32_t>(pc_ - start_) + buffer_offset_;
+  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 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.
@@ -318,7 +335,7 @@ class Decoder {
     *length = byte_index + (at_end ? 0 : 1);
     if (checked && (at_end || (b & 0x80))) {
       TRACE_IF(trace, at_end ? "<end> " : "<length overflow> ");
-      errorf(pc, "expected %s", name);
+      error(pc) << "expected " << name;
       result = 0;
     }
     if (is_last_byte) {
@@ -338,7 +355,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->errorf(pc + 1 + len, "expected table index 0, found %u",
-                      table_index);
+      decoder->error(pc + 1 + len)
+          << "expected table index 0, found " << 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->errorf(pc + 1, "expected memory index 0, found %u", index);
+      decoder->error(pc + 1) << "expected memory index 0, found " << index;
     }
   }
 };
@@ -279,10 +279,9 @@ struct MemoryAccessOperand {
     alignment =
         decoder->read_u32v<checked>(pc + 1, &alignment_length, "alignment");
     if (!CHECKED_COND(alignment <= max_alignment)) {
-      decoder->errorf(pc + 1,
-                      "invalid alignment; expected maximum alignment is %u, "
-                      "actual alignment is %u",
-                      max_alignment, alignment);
+      decoder->error(pc + 1)
+          << "invalid alignment; expected maximum alignment is "
+          << max_alignment << ", actual alignment is " << alignment;
     }
     unsigned offset_length;
     offset = decoder->read_u32v<checked>(pc + 1 + alignment_length,

src/wasm/streaming-decoder.cc

@@ -237,35 +237,31 @@ StreamingDecoder::DecodeVarInt32::Next(StreamingDecoder* streaming) {
     return nullptr;
   }
   if (value() > max_value_) {
-    streaming->decoder()->errorf(buffer(), "size > maximum function size: %zu",
-                                 value());
+    streaming->decoder()->error(buffer())
+        << "size > maximum function size: " << 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->errorf(buffer(),
-                    "expected magic word %02x %02x %02x %02x, "
-                    "found %02x %02x %02x %02x",
-                    BYTES(kWasmMagic), BYTES(magic_word));
+    decoder->error(buffer())
+        << "expected magic word " << AsHexBytes(kWasmMagic, 4) << ", found "
+        << AsHexBytes(magic_word, 4);
   }
   uint32_t magic_version = decoder->consume_u32("wasm version");
   if (magic_version != kWasmVersion) {
-    decoder->errorf(buffer(),
-                    "expected version %02x %02x %02x %02x, "
-                    "found %02x %02x %02x %02x",
-                    BYTES(kWasmVersion), BYTES(magic_version));
+    decoder->error(buffer())
+        << "expected version " << AsHexBytes(kWasmVersion, 4) << ", found "
+        << AsHexBytes(magic_version, 4);
   }
 }
-#undef BYTES
 
 std::unique_ptr<StreamingDecoder::DecodingState>
 StreamingDecoder::DecodeModuleHeader::Next(StreamingDecoder* streaming) {
@@ -309,7 +305,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>();
   }
 
@@ -330,17 +326,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()->errorf(buffer(), "Invalid function length (0)");
+    streaming->decoder()->error(buffer()) << "Invalid function length (0)";
     return nullptr;
   } else if (buffer_offset() + bytes_needed() + value() >
              section_buffer()->length()) {
-    streaming->decoder()->errorf(buffer(), "not enough code section bytes");
+    streaming->decoder()->error(buffer()) << "not enough code section bytes";
     return nullptr;
   }
 
@@ -360,9 +356,9 @@ StreamingDecoder::DecodeFunctionBody::Next(StreamingDecoder* streaming) {
       streaming->decoder()->Reset(
           section_buffer()->bytes(),
           section_buffer()->bytes() + section_buffer()->length());
-      streaming->decoder()->errorf(
-          section_buffer()->bytes() + buffer_offset() + size(),
-          "not all code section bytes were used");
+      streaming->decoder()->error(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,3 +56,20 @@ 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));
+})();