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

source/common/stats/fake_symbol_table_impl.h

@@ -64,13 +64,10 @@ 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
@@ -81,7 +78,7 @@ class FakeSymbolTableImpl : public SymbolTable {
     for (uint32_t i = 0; i < num_names; ++i) {
       auto& name = names[i];
       size_t sz = name.size();
-      p = SymbolTableImpl::writeLengthReturningNext(sz, p);
+      p = SymbolTableImpl::Encoding::writeEncodingReturningNext(sz, p);
       if (!name.empty()) {
         memcpy(p, name.data(), sz * sizeof(uint8_t));
         p += sz;
@@ -143,8 +140,9 @@ class FakeSymbolTableImpl : public SymbolTable {
 
   StoragePtr encodeHelper(absl::string_view name) const {
     ASSERT(!absl::EndsWith(name, "."));
-    auto bytes = std::make_unique<Storage>(name.size() + StatNameSizeEncodingBytes);
-    uint8_t* buffer = SymbolTableImpl::writeLengthReturningNext(name.size(), bytes.get());
+    auto bytes = std::make_unique<Storage>(SymbolTableImpl::Encoding::totalSizeBytes(name.size()));
+    uint8_t* buffer =
+        SymbolTableImpl::Encoding::writeEncodingReturningNext(name.size(), bytes.get());
     memcpy(buffer, name.data(), name.size());
     return bytes;
   }

source/common/stats/symbol_table_impl.cc

@@ -16,6 +16,13 @@ namespace Stats {
 static const uint32_t SpilloverMask = 0x80;
 static const uint32_t Low7Bits = 0x7f;
 
+uint64_t StatName::dataSize() const {
+  if (size_and_data_ == nullptr) {
+    return 0;
+  }
+  return SymbolTableImpl::Encoding::decodeNumber(size_and_data_).first;
+}
+
 #ifndef ENVOY_CONFIG_COVERAGE
 void StatName::debugPrint() {
   if (size_and_data_ == nullptr) {
@@ -39,10 +46,19 @@ void StatName::debugPrint() {
 SymbolTableImpl::Encoding::~Encoding() {
   // Verifies that moveToStorage() was called on this encoding. Failure
   // to call moveToStorage() will result in leaks symbols.
-  ASSERT(vec_.empty());
+  ASSERT(storage_ == nullptr);
 }
 
-void SymbolTableImpl::Encoding::addSymbol(Symbol symbol) {
+uint64_t SymbolTableImpl::Encoding::encodingSizeBytes(uint64_t number) {
+  uint64_t num_bytes = 0;
+  do {
+    ++num_bytes;
+    number >>= 7;
+  } while (number != 0);
+  return num_bytes;
+}
+
+uint8_t* SymbolTableImpl::Encoding::writeEncodingReturningNext(uint64_t number, uint8_t* bytes) {
   // UTF-8-like encoding where a value 127 or less gets written as a single
   // byte. For higher values we write the low-order 7 bits with a 1 in
   // the high-order bit. Then we right-shift 7 bits and keep adding more bytes
@@ -51,45 +67,59 @@ void SymbolTableImpl::Encoding::addSymbol(Symbol symbol) {
   // When decoding, we stop consuming uint8_t when we see a uint8_t with
   // high-order bit 0.
   do {
-    if (symbol < (1 << 7)) {
-      vec_.push_back(symbol); // symbols <= 127 get encoded in one byte.
+    if (number < (1 << 7)) {
+      *bytes++ = number; // number <= 127 get encoded in one byte.
     } else {
-      vec_.push_back((symbol & Low7Bits) | SpilloverMask); // symbols >= 128 need spillover bytes.
+      *bytes++ = ((number & Low7Bits) | SpilloverMask); // number >= 128 need spillover bytes.
     }
-    symbol >>= 7;
-  } while (symbol != 0);
+    number >>= 7;
+  } while (number != 0);
+  return bytes;
+}
+
+void SymbolTableImpl::Encoding::addSymbols(const std::vector<Symbol>& symbols) {
+  ASSERT(data_bytes_required_ == 0);
+  for (Symbol symbol : symbols) {
+    data_bytes_required_ += encodingSizeBytes(symbol);
+  }
+  storage_ = std::make_unique<Storage>(data_bytes_required_);
+  uint8_t* bytes = storage_.get();
+  for (Symbol symbol : symbols) {
+    bytes = writeEncodingReturningNext(symbol, bytes);
+  }
+  ASSERT(static_cast<uint64_t>(bytes - storage_.get()) == data_bytes_required_);
+}
+
+std::pair<uint64_t, uint64_t> SymbolTableImpl::Encoding::decodeNumber(const uint8_t* encoding) {
+  uint64_t number = 0;
+  uint64_t uc = SpilloverMask;
+  const uint8_t* start = encoding;
+  for (uint32_t shift = 0; (uc & SpilloverMask) != 0; ++encoding, shift += 7) {
+    uc = static_cast<uint32_t>(*encoding);
+    number |= (uc & Low7Bits) << shift;
+  }
+  return std::make_pair(number, encoding - start);
 }
 
 SymbolVec SymbolTableImpl::Encoding::decodeSymbols(const SymbolTable::Storage array,
                                                    uint64_t size) {
   SymbolVec symbol_vec;
-  Symbol symbol = 0;
-  for (uint32_t shift = 0; size > 0; --size, ++array) {
-    uint32_t uc = static_cast<uint32_t>(*array);
-
-    // Inverse addSymbol encoding, walking down the bytes, shifting them into
-    // symbol, until a byte with a zero high order bit indicates this symbol is
-    // complete and we can move to the next one.
-    symbol |= (uc & Low7Bits) << shift;
-    if ((uc & SpilloverMask) == 0) {
-      symbol_vec.push_back(symbol);
-      shift = 0;
-      symbol = 0;
-    } else {
-      shift += 7;
-    }
+  symbol_vec.reserve(size);
+  while (size > 0) {
+    std::pair<uint64_t, uint64_t> symbol_consumed = decodeNumber(array);
+    symbol_vec.push_back(symbol_consumed.first);
+    size -= symbol_consumed.second;
+    array += symbol_consumed.second;
   }
   return symbol_vec;
 }
 
-uint64_t SymbolTableImpl::Encoding::moveToStorage(SymbolTable::Storage symbol_array) {
-  const uint64_t sz = dataBytesRequired();
-  symbol_array = writeLengthReturningNext(sz, symbol_array);
-  if (sz != 0) {
-    memcpy(symbol_array, vec_.data(), sz * sizeof(uint8_t));
+void SymbolTableImpl::Encoding::moveToStorage(SymbolTable::Storage symbol_array) {
+  symbol_array = writeEncodingReturningNext(data_bytes_required_, symbol_array);
+  if (data_bytes_required_ != 0) {
+    memcpy(symbol_array, storage_.get(), data_bytes_required_);
   }
-  vec_.clear(); // Logically transfer ownership, enabling empty assert on destruct.
-  return sz + StatNameSizeEncodingBytes;
+  storage_.reset(); // Logically transfer ownership, enabling empty assert on destruct.
 }
 
 SymbolTableImpl::SymbolTableImpl()
@@ -129,9 +159,7 @@ void SymbolTableImpl::addTokensToEncoding(const absl::string_view name, Encoding
   }
 
   // Now efficiently encode the array of 32-bit symbols into a uint8_t array.
-  for (Symbol symbol : symbols) {
-    encoding.addSymbol(symbol);
-  }
+  encoding.addSymbols(symbols);
 }
 
 uint64_t SymbolTableImpl::numSymbols() const {
@@ -456,8 +484,8 @@ SymbolTable::StoragePtr SymbolTableImpl::join(const StatNameVec& stat_names) con
   for (StatName stat_name : stat_names) {
     num_bytes += stat_name.dataSize();
   }
-  auto bytes = std::make_unique<Storage>(num_bytes + StatNameSizeEncodingBytes);
-  uint8_t* p = writeLengthReturningNext(num_bytes, bytes.get());
+  auto bytes = std::make_unique<Storage>(Encoding::totalSizeBytes(num_bytes));
+  uint8_t* p = SymbolTableImpl::Encoding::writeEncodingReturningNext(num_bytes, bytes.get());
   for (StatName stat_name : stat_names) {
     const uint64_t stat_name_bytes = stat_name.dataSize();
     memcpy(p, stat_name.data(), stat_name_bytes);
@@ -486,7 +514,8 @@ void SymbolTableImpl::populateList(const absl::string_view* names, uint32_t num_
   uint8_t* p = &storage[0];
   *p++ = num_names;
   for (auto& encoding : encodings) {
-    p += encoding.moveToStorage(p);
+    encoding.moveToStorage(p);
+    p += encoding.bytesRequired();
   }
 
   // This assertion double-checks the arithmetic where we computed

source/common/stats/symbol_table_impl.h

@@ -30,12 +30,6 @@ namespace Stats {
 /** A Symbol represents a string-token with a small index. */
 using Symbol = uint32_t;
 
-/**
- * We encode the byte-size of a StatName as its first two bytes.
- */
-constexpr uint64_t StatNameSizeEncodingBytes = 2;
-constexpr uint64_t StatNameMaxSize = 1 << (8 * StatNameSizeEncodingBytes); // 65536
-
 /** Transient representations of a vector of 32-bit symbols */
 using SymbolVec = std::vector<Symbol>;
 
@@ -93,7 +87,7 @@ class SymbolTableImpl : public SymbolTable {
      *
      * @param symbol the symbol to encode.
      */
-    void addSymbol(Symbol symbol);
+    void addSymbols(const std::vector<Symbol>& symbols);
 
     /**
      * Decodes a uint8_t array into a SymbolVec.
@@ -104,24 +98,61 @@ class SymbolTableImpl : public SymbolTable {
      * Returns the number of bytes required to represent StatName as a uint8_t
      * array, including the encoded size.
      */
-    uint64_t bytesRequired() const { return dataBytesRequired() + StatNameSizeEncodingBytes; }
+    uint64_t bytesRequired() const {
+      return data_bytes_required_ + encodingSizeBytes(data_bytes_required_);
+    }
 
     /**
      * @return the number of uint8_t entries we collected while adding symbols.
      */
-    uint64_t dataBytesRequired() const { return vec_.size(); }
+    uint64_t dataBytesRequired() const { return data_bytes_required_; }
 
     /**
      * Moves the contents of the vector into an allocated array. The array
      * must have been allocated with bytesRequired() bytes.
      *
      * @param array destination memory to receive the encoded bytes.
-     * @return uint64_t the number of bytes transferred.
      */
-    uint64_t moveToStorage(SymbolTable::Storage array);
+    void moveToStorage(SymbolTable::Storage array);
+
+    /**
+     * @param number A number to encode in a variable length byte-array.
+     * @return The number of bytes it would take to encode the number.
+     */
+    static uint64_t encodingSizeBytes(uint64_t number);
+
+    /**
+     * @param num_data_bytes The number of bytes in a data-block.
+     * @return The total number of bytes required for the data-block and its encoded size.
+     */
+    static uint64_t totalSizeBytes(uint64_t num_data_bytes) {
+      return encodingSizeBytes(num_data_bytes) + num_data_bytes;
+    }
+
+    /**
+     * Saves the specified number into the byte array, returning the next byte.
+     * There is no guarantee that bytes will be aligned, so we can't cast to a
+     * uint16_t* and assign, but must individually copy the bytes.
+     *
+     * Requires that the buffer be sized to accommodate encodingSizeBytes(number).
+     *
+     * @param number the number to write.
+     * @param bytes the pointer into which to write the length.
+     * @return the pointer to the next byte for writing the data.
+     */
+    static uint8_t* writeEncodingReturningNext(uint64_t number, uint8_t* buffer);
+
+    /**
+     * Decodes a byte-array containing a variable-length number.
+     *
+     * @param The encoded byte array, written previously by writeNumberReturningNext.
+     * @return A pair containing the decoded number, and the number of bytes consumed from encoding.
+     */
+    static std::pair<uint64_t, uint64_t> decodeNumber(const uint8_t* encoding);
 
   private:
-    std::vector<uint8_t> vec_;
+    uint64_t data_bytes_required_{0};
+    StoragePtr storage_;
   };
 
   SymbolTableImpl();
@@ -144,22 +175,6 @@ class SymbolTableImpl : public SymbolTable {
   void debugPrint() const override;
 #endif
 
-  /**
-   * Saves the specified length into the byte array, returning the next byte.
-   * There is no guarantee that bytes will be aligned, so we can't cast to a
-   * uint16_t* and assign, but must individually copy the bytes.
-   *
-   * @param length the length in bytes to write. Must be < StatNameMaxSize.
-   * @param bytes the pointer into which to write the length.
-   * @return the pointer to the next byte for writing the data.
-   */
-  static inline uint8_t* writeLengthReturningNext(uint64_t length, uint8_t* bytes) {
-    ASSERT(length < StatNameMaxSize);
-    *bytes++ = length & 0xff;
-    *bytes++ = length >> 8;
-    return bytes;
-  }
-
   StatNameSetPtr makeSet(absl::string_view name) override;
   void forgetSet(StatNameSet& stat_name_set) override;
   uint64_t getRecentLookups(const RecentLookupsFn&) const override;
@@ -362,18 +377,13 @@ class StatName {
    * @return uint64_t the number of bytes in the symbol array, excluding the two-byte
    *                  overhead for the size itself.
    */
-  uint64_t dataSize() const {
-    if (size_and_data_ == nullptr) {
-      return 0;
-    }
-    return size_and_data_[0] | (static_cast<uint64_t>(size_and_data_[1]) << 8);
-  }
+  uint64_t dataSize() const;
 
   /**
    * @return uint64_t the number of bytes in the symbol array, including the two-byte
    *                  overhead for the size itself.
    */
-  uint64_t size() const { return dataSize() + StatNameSizeEncodingBytes; }
+  uint64_t size() const { return SymbolTableImpl::Encoding::totalSizeBytes(dataSize()); }
 
   void copyToStorage(SymbolTable::Storage storage) { memcpy(storage, size_and_data_, size()); }
 
@@ -384,7 +394,9 @@ class StatName {
   /**
    * @return A pointer to the first byte of data (skipping over size bytes).
    */
-  const uint8_t* data() const { return size_and_data_ + StatNameSizeEncodingBytes; }
+  const uint8_t* data() const {
+    return size_and_data_ + SymbolTableImpl::Encoding::encodingSizeBytes(dataSize());
+  }
 
   /**
    * @return whether this is empty.