stats: Avoid asserts in fuzz-tests by eliminating arbitrary length limits, using the new MemBlock wrapper for memcpy

source/common/common/BUILD

@@ -113,6 +113,12 @@ envoy_cc_library(
     deps = [":assert_lib"],
 )
 
+envoy_cc_library(
+    name = "mem_block_builder_lib",
+    hdrs = ["mem_block_builder.h"],
+    deps = [":assert_lib"],
+)
+
 # Contains macros and helpers for dumpState utilities
 envoy_cc_library(
     name = "dump_state_utils",

source/common/common/mem_block_builder.h

@@ -0,0 +1,136 @@
+#pragma once
+
+#include <memory>
+#include <vector>
+
+#include "common/common/assert.h"
+
+namespace Envoy {
+
+// Manages a block of raw memory for objects of type T. T is generally expected
+// to be a POD, where it makes sense to memcpy over it. This class carries extra
+// member variables for tracking size, and a write-pointer to support safe
+// appends.
+//
+// MemBlockBuilder is used to safely write blocks of data into a memory
+// buffer. Due to two extra member variables, it is not optimal for storing in
+// data structures. The intended usage model is to release the raw assembled
+// memory block from the MemBlockBuilder for efficient storage.
+//
+// The goal for this class is to provide a usage model to replace direct usage
+// of memcpy with a pattern that is easy to validate for correctness by
+// inspection, asserts, and fuzzing, but when compiled for optimization is
+// roughly as efficient as raw memcpy.
+template <class T> class MemBlockBuilder {
+public:
+  // Constructs a MemBlockBuilder allowing for 'capacity' instances of T.
+  explicit MemBlockBuilder(uint64_t capacity)
+      : data_(std::make_unique<T[]>(capacity)), capacity_(capacity), capacity_remaining_(capacity),
+        write_ptr_(data_.get()) {}
+  MemBlockBuilder() : capacity_(0), capacity_remaining_(0), write_ptr_(nullptr) {}
+
+  /**
+   * Populates (or repopulates) the MemBlockBuilder to make it the specified
+   * capacity. This does not have resize semantics; when populate() is called any
+   * previous contents are erased.
+   *
+   * @param capcity The number of memory elements to allocate.
+   */
+  void populate(uint64_t capacity) {
+    data_ = std::make_unique<T[]>(capacity);
+    capacity_ = capacity;
+    capacity_remaining_ = capacity;
+    write_ptr_ = data_.get();
+  }
+
+  /**
+   * @return the capacity.
+   */
+  uint64_t capacity() const { return capacity_; }
+
+  /**
+   * Appends a single object of type T, moving an internal write-pointer
+   * forward. Asserts that there is room to write the object when compiled
+   * for debug.
+   *
+   * @param object the object to append.
+   */
+  void appendOne(T object) {
+    RELEASE_ASSERT(capacity_remaining_ >= 1, "insufficient capacity");
+    ASSERT(write_ptr_ < (data_.get() + capacity_));
+    *write_ptr_++ = object;
+    --capacity_remaining_;
+  }
+
+  /**
+   * Appends raw data, moving an internal write-pointer forward. Asserts
+   * that there is room to write the block when compiled for debug. It is
+   * the caller's responsibility to ensure that the input data is valid.
+   *
+   * @param data The block of objects to insert.
+   * @param size The number of elements in the block.
+   */
+  void appendData(const T* data, uint64_t size) {
+    RELEASE_ASSERT(capacity_remaining_ >= size, "insufficient capacity");
+    ASSERT((write_ptr_ + size) <= (data_.get() + capacity_));
+    if (size != 0) {
+      memcpy(write_ptr_, data, size * sizeof(T));
+    }
+    write_ptr_ += size;
+    capacity_remaining_ -= size;
+  }
+
+  /**
+   * Appends the contents of another memory block to this one.
+   *
+   * @param src the block to append.
+   */
+  void appendBlock(const MemBlockBuilder& src) { appendData(src.data_.get(), src.size()); }
+
+  /**
+   * @return the number of elements remaining in the MemBlockBuilder.
+   */
+  uint64_t capacityRemaining() const { return capacity_remaining_; }
+
+  /**
+   * Empties the contents of this.
+   */
+  void reset() {
+    data_.reset();
+    capacity_ = 0;
+    capacity_remaining_ = 0;
+    write_ptr_ = nullptr;
+  }
+
+  /**
+   * Returns the underlying storage as a unique pointer, clearing this.
+   *
+   * @return the transferred storage.
+   */
+  std::unique_ptr<T[]> release() {
+    write_ptr_ = nullptr;
+    capacity_ = 0;
+    capacity_remaining_ = 0;
+    return std::move(data_);
+  }
+
+  /**
+   * @return the populated data as a vector.
+   *
+   * This is exposed to help with unit testing.
+   */
+  std::vector<T> toVector() const { return std::vector<T>(data_.get(), write_ptr_); }
+
+  /**
+   * @return The number of elements the have been added to the builder.
+   */
+  uint64_t size() const { return write_ptr_ - data_.get(); }
+
+private:
+  std::unique_ptr<T[]> data_;
+  uint64_t capacity_;
+  uint64_t capacity_remaining_;
+  T* write_ptr_;
+};
+
+} // namespace Envoy

source/common/stats/BUILD

@@ -159,6 +159,7 @@ envoy_cc_library(
         "//include/envoy/stats:symbol_table_interface",
         "//source/common/common:assert_lib",
         "//source/common/common:logger_lib",
+        "//source/common/common:mem_block_builder_lib",
         "//source/common/common:stack_array",
         "//source/common/common:thread_lib",
         "//source/common/common:utility_lib",

source/common/stats/fake_symbol_table_impl.h

@@ -64,36 +64,31 @@ class FakeSymbolTableImpl : public SymbolTable {
     // than 256 of them.
     RELEASE_ASSERT(num_names < 256, "Maximum number elements in a StatNameList exceeded");
 
-    // First encode all the names. The '1' here represents the number of
-    // names. The num_names * StatNameSizeEncodingBytes reserves space for the
-    // lengths of each name.
-    size_t total_size_bytes = 1 + num_names * StatNameSizeEncodingBytes;
-
+    size_t total_size_bytes = 1; /* one byte for holding the number of names */
     for (uint32_t i = 0; i < num_names; ++i) {
-      total_size_bytes += names[i].size();
+      size_t name_size = names[i].size();
+      total_size_bytes += SymbolTableImpl::Encoding::totalSizeBytes(name_size);
     }
 
     // Now allocate the exact number of bytes required and move the encodings
     // into storage.
-    auto storage = std::make_unique<Storage>(total_size_bytes);
-    uint8_t* p = &storage[0];
-    *p++ = num_names;
+    MemBlockBuilder<uint8_t> mem_block(total_size_bytes);
+    mem_block.appendOne(num_names);
     for (uint32_t i = 0; i < num_names; ++i) {
       auto& name = names[i];
       size_t sz = name.size();
-      p = SymbolTableImpl::writeLengthReturningNext(sz, p);
+      SymbolTableImpl::Encoding::appendEncoding(sz, mem_block);
       if (!name.empty()) {
-        memcpy(p, name.data(), sz * sizeof(uint8_t));
-        p += sz;
+        mem_block.appendData(reinterpret_cast<const uint8_t*>(name.data()), sz);
       }
     }
 
     // This assertion double-checks the arithmetic where we computed
     // total_size_bytes. After appending all the encoded data into the
-    // allocated byte array, we should wind up with a pointer difference of
-    // total_size_bytes from the beginning of the allocation.
-    ASSERT(p == &storage[0] + total_size_bytes);
-    list.moveStorageIntoList(std::move(storage));
+    // allocated byte array, we should have exhausted all the memory
+    // we though we needed.
+    ASSERT(mem_block.capacityRemaining() == 0);
+    list.moveStorageIntoList(mem_block.release());
   }
 
   std::string toString(const StatName& stat_name) const override {
@@ -143,10 +138,10 @@ class FakeSymbolTableImpl : public SymbolTable {
 
   StoragePtr encodeHelper(absl::string_view name) const {
     name = StringUtil::removeTrailingCharacters(name, '.');
-    auto bytes = std::make_unique<Storage>(name.size() + StatNameSizeEncodingBytes);
-    uint8_t* buffer = SymbolTableImpl::writeLengthReturningNext(name.size(), bytes.get());
-    memcpy(buffer, name.data(), name.size());
-    return bytes;
+    MemBlockBuilder<uint8_t> mem_block(SymbolTableImpl::Encoding::totalSizeBytes(name.size()));
+    SymbolTableImpl::Encoding::appendEncoding(name.size(), mem_block);
+    mem_block.appendData(reinterpret_cast<const uint8_t*>(name.data()), name.size());
+    return mem_block.release();
   }
 };