stats: fix a bug where empty StatName cannot initialize storage correctly

source/common/stats/symbol_table.cc

@@ -261,6 +261,12 @@ void SymbolTable::Encoding::appendToMemBlock(StatName stat_name,
   }
 }
 
+void SymbolTable::Encoding::appendStatNameDataTo(StatName stat_name,
+                                                 MemBlockBuilder<uint8_t>& mem_block) {
+  auto sv = stat_name.dataAsStringView();
+  mem_block.appendData(absl::MakeSpan(reinterpret_cast<const uint8_t*>(sv.data()), sv.size()));
+}
+
 SymbolTable::SymbolTable()
     // Have to be explicitly initialized, if we want to use the ABSL_GUARDED_BY macro.
     : next_symbol_(FirstValidSymbol), monotonic_counter_(FirstValidSymbol) {}
@@ -551,7 +557,7 @@ StatNameStorage::StatNameStorage(absl::string_view name, SymbolTable& table)
 StatNameStorage::StatNameStorage(StatName src, SymbolTable& table) {
   const size_t size = src.size();
   MemBlockBuilder<uint8_t> storage(size); // Note: MemBlockBuilder takes uint64_t.
-  src.copyToMemBlock(storage);
+  SymbolTable::Encoding::appendToMemBlock(src, storage);
   setBytes(storage.release());
   table.incRefCount(statName());
 }
@@ -666,7 +672,7 @@ SymbolTable::StoragePtr SymbolTable::join(const StatNameVec& stat_names) const {
   MemBlockBuilder<uint8_t> mem_block(Encoding::totalSizeBytes(num_bytes));
   Encoding::appendEncoding(num_bytes, mem_block);
   for (StatName stat_name : stat_names) {
-    stat_name.appendDataToMemBlock(mem_block);
+    Encoding::appendStatNameDataTo(stat_name, mem_block);
   }
   ASSERT(mem_block.capacityRemaining() == 0);
   return mem_block.release();

source/common/stats/symbol_table.h

@@ -178,6 +178,14 @@ class SymbolTable final {
      */
     static void appendEncoding(uint64_t number, MemBlockBuilder<uint8_t>& mem_block);
 
+    /**
+     * Appends stat_name's data into storage.
+     *
+     * @param stat_name the stat_name to append.
+     * @param mem_block the memory block to append to.
+     */
+    static void appendStatNameDataTo(StatName stat_name, MemBlockBuilder<uint8_t>& mem_block);
+
     /**
      * Appends stat_name's bytes into mem_block, which must have been allocated to
      * allow for stat_name.size() bytes.
@@ -633,36 +641,15 @@ class StatName {
   size_t dataSize() const { return dataAsStringView().size(); }
 
   /**
-   * @return size_t the number of bytes in the symbol array, including the
-   *                  overhead for the size itself.
+   * @return size_t the number of bytes in the symbol array, including the overhead for the size
+   * itself. NOTE: this will return at least one byte even when there is no backing storage, to
+   * indicate the encoded length prefix when encoding the StatName into a StatNameList and so on.
+   * However, you can NEVER assume that data()/dataIncludingSize() will return non-nullptr just
+   * because this returns non-zero; the empty()/dataSize() APIs should be used to check whether the
+   * StatName has valid data.
    */
   size_t size() const { return SymbolTable::Encoding::totalSizeBytes(dataSize()); }
 
-  /**
-   * Copies the entire StatName representation into a MemBlockBuilder, including
-   * the length metadata at the beginning. The MemBlockBuilder must not have
-   * any other data in it.
-   *
-   * @param mem_block_builder the builder to receive the storage.
-   */
-  void copyToMemBlock(MemBlockBuilder<uint8_t>& mem_block_builder) {
-    ASSERT(mem_block_builder.size() == 0);
-    mem_block_builder.appendData(absl::MakeSpan(size_and_data_, size()));
-  }
-
-  /**
-   * Appends the data portion of the StatName representation into a
-   * MemBlockBuilder, excluding the length metadata. This is appropriate for
-   * join(), where several stat-names are combined, and we only need the
-   * aggregated length metadata.
-   *
-   * @param mem_block_builder the builder to receive the storage.
-   */
-  void appendDataToMemBlock(MemBlockBuilder<uint8_t>& storage) {
-    auto sv = dataAsStringView();
-    storage.appendData(absl::MakeSpan(reinterpret_cast<const uint8_t*>(sv.data()), sv.size()));
-  }
-
 #ifndef ENVOY_CONFIG_COVERAGE
   void debugPrint();
 #endif
@@ -695,7 +682,6 @@ class StatName {
    */
   bool startsWith(StatName symbolic_prefix) const;
 
-private:
   /**
    * Casts the raw data as a string_view, decoding the varint length prefix.
    * All accessors that need the data pointer and/or size should delegate to
@@ -709,6 +695,7 @@ class StatName {
     return {reinterpret_cast<const char*>(size_and_data_ + prefix_size), data_size};
   }
 
+private:
   const uint8_t* size_and_data_{nullptr};
 };
 

test/common/stats/symbol_table_impl_test.cc

@@ -701,6 +701,15 @@ TEST_F(StatNameTest, StorageCopy) {
   EXPECT_NE(a.data(), c.data());
 }
 
+TEST_F(StatNameTest, StorageFromEmptyStatName) {
+  StatName empty;
+  StatNameStorage b_storage(empty, table_);
+  const StatName b = b_storage.statName();
+  EXPECT_EQ(empty, b);
+  EXPECT_NE(empty.data(), b.data());
+  b_storage.free(table_);
+}
+
 TEST_F(StatNameTest, AddingToPoolViaStatNamePreservesDynamicSegments) {
   const StatNameDynamicStorage tag_name("tag", table_);
   const StatNameDynamicStorage tag_value("value", table_);