[wasm] Fix/improve StringBuilder buffer growth

This includes several changes:
- avoid a very-unlikely-but-theoretically-possible OOB write
- avoid a somewhat-likely memory leak
- grow the buffer less aggressively for medium-length strings

Change-Id: I877f43d7e2e7cd4778ba8c7c7525ba988301f750
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3771900
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/main@{#81808}

src/wasm/string-builder.h

@@ -30,13 +30,16 @@ class StringBuilder {
   StringBuilder& operator=(const StringBuilder&) = delete;
   ~StringBuilder() {
     for (char* chunk : chunks_) delete[] chunk;
+    if (on_growth_ == kReplacePreviousChunk && start_ != stack_buffer_) {
+      delete[] start_;
+    }
   }
 
   // Reserves space for {n} characters and returns a pointer to its beginning.
   // Clients *must* write all {n} characters after calling this!
   // Don't call this directly, use operator<< overloads instead.
   char* allocate(size_t n) {
-    if (remaining_bytes_ < n) Grow();
+    if (remaining_bytes_ < n) Grow(n);
     char* result = cursor_;
     cursor_ += n;
     remaining_bytes_ -= n;
@@ -68,14 +71,25 @@ class StringBuilder {
   void start_here() { start_ = cursor_; }
 
  private:
-  void Grow() {
+  void Grow(size_t requested) {
     size_t used = length();
-    // Safety net for super-long strings/lines.
+    size_t required = used + requested;
-    size_t chunk_size = used < kChunkSize ? kChunkSize : used * 2;
+    size_t chunk_size;
+    if (on_growth_ == kKeepOldChunks) {
+      // Usually grow by kChunkSize, unless super-long lines need even more.
+      chunk_size = required < kChunkSize ? kChunkSize : required * 2;
+    } else {
+      // When we only have one chunk, always (at least) double its size
+      // when it grows, to minimize both wasted memory and growth effort.
+      chunk_size = required * 2;
+    }
+
     char* new_chunk = new char[chunk_size];
     memcpy(new_chunk, start_, used);
     if (on_growth_ == kKeepOldChunks) {
       chunks_.push_back(new_chunk);
+    } else if (start_ != stack_buffer_) {
+      delete[] start_;
     }
     start_ = new_chunk;
     cursor_ = new_chunk + used;

test/unittests/BUILD.gn

@@ -517,6 +517,7 @@ v8_source_set("unittests_sources") {
       "wasm/module-decoder-unittest.cc",
       "wasm/simd-shuffle-unittest.cc",
       "wasm/streaming-decoder-unittest.cc",
+      "wasm/string-builder-unittest.cc",
       "wasm/subtyping-unittest.cc",
       "wasm/wasm-code-manager-unittest.cc",
       "wasm/wasm-compiler-unittest.cc",

test/unittests/wasm/string-builder-unittest.cc

@@ -0,0 +1,43 @@
+// Copyright 2022 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.
+
+#include "src/wasm/string-builder.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace v8::internal::wasm {
+namespace string_builder_unittest {
+
+TEST(StringBuilder, Simple) {
+  StringBuilder sb;
+  sb << "foo"
+     << "bar" << -42 << "\n";
+  EXPECT_STREQ(std::string(sb.start(), sb.length()).c_str(), "foobar-42\n");
+}
+
+TEST(StringBuilder, DontLeak) {
+  // Should be bigger than StringBuilder::kStackSize = 256.
+  constexpr size_t kMoreThanStackBufferSize = 300;
+  StringBuilder sb;
+  const char* on_stack = sb.start();
+  sb.allocate(kMoreThanStackBufferSize);
+  const char* on_heap = sb.start();
+  // If this fails, then kMoreThanStackBufferSize was too small.
+  ASSERT_NE(on_stack, on_heap);
+  // Still don't leak on further growth.
+  sb.allocate(kMoreThanStackBufferSize * 4);
+}
+
+TEST(StringBuilder, SuperLongStrings) {
+  // Should be bigger than StringBuilder::kChunkSize = 1024 * 1024.
+  constexpr size_t kMoreThanChunkSize = 2 * 1024 * 1024;
+  StringBuilder sb;
+  char* s = sb.allocate(kMoreThanChunkSize);
+  for (size_t i = 0; i < kMoreThanChunkSize; i++) {
+    s[i] = 'a';
+  }
+}
+
+}  // namespace string_builder_unittest
+}  // namespace v8::internal::wasm