Check CRC cordrep child nodes for nullptr.

Some time ago the invariant for CRC cordreps was relaxed to allow for nullptr values on empty cords with an explicit empty CRC value. The CordzInfo analysis never checked for nullptr values causing cord sampling to crash if the sampling happened to include a (very unlikely) empty Cord value.

PiperOrigin-RevId: 558202613
Change-Id: Ib0e1eadd08047167e4df5d3035b36dca2c285a0d

absl/strings/internal/cordz_info.cc

@@ -33,8 +33,6 @@ namespace absl {
 ABSL_NAMESPACE_BEGIN
 ABSL_NAMESPACE_BEGIN
 namespace cord_internal {
 namespace cord_internal {
 
 
-using ::absl::base_internal::SpinLockHolder;
-
 #ifdef ABSL_INTERNAL_NEED_REDUNDANT_CONSTEXPR_DECL
 #ifdef ABSL_INTERNAL_NEED_REDUNDANT_CONSTEXPR_DECL
 constexpr size_t CordzInfo::kMaxStackDepth;
 constexpr size_t CordzInfo::kMaxStackDepth;
 #endif
 #endif
@@ -79,6 +77,8 @@ class CordRepAnalyzer {
   // adds the results to `statistics`. Note that node counts and memory sizes
   // adds the results to `statistics`. Note that node counts and memory sizes
   // are not initialized, computed values are added to any existing values.
   // are not initialized, computed values are added to any existing values.
   void AnalyzeCordRep(const CordRep* rep) {
   void AnalyzeCordRep(const CordRep* rep) {
+    ABSL_ASSERT(rep != nullptr);
+
     // Process all linear nodes.
     // Process all linear nodes.
     // As per the class comments, use refcout - 1 on the top level node, as the
     // As per the class comments, use refcout - 1 on the top level node, as the
     // top level node is assumed to be referenced only for analysis purposes.
     // top level node is assumed to be referenced only for analysis purposes.
@@ -86,7 +86,7 @@ class CordRepAnalyzer {
     RepRef repref{rep, (refcount > 1) ? refcount - 1 : 1};
     RepRef repref{rep, (refcount > 1) ? refcount - 1 : 1};
 
 
     // Process the top level CRC node, if present.
     // Process the top level CRC node, if present.
-    if (repref.rep->tag == CRC) {
+    if (repref.tag() == CRC) {
       statistics_.node_count++;
       statistics_.node_count++;
       statistics_.node_counts.crc++;
       statistics_.node_counts.crc++;
       memory_usage_.Add(sizeof(CordRepCrc), repref.refcount);
       memory_usage_.Add(sizeof(CordRepCrc), repref.refcount);
@@ -96,15 +96,17 @@ class CordRepAnalyzer {
     // Process all top level linear nodes (substrings and flats).
     // Process all top level linear nodes (substrings and flats).
     repref = CountLinearReps(repref, memory_usage_);
     repref = CountLinearReps(repref, memory_usage_);
 
 
-    if (repref.rep != nullptr) {
-      if (repref.rep->tag == RING) {
+    switch (repref.tag()) {
+      case CordRepKind::RING:
         AnalyzeRing(repref);
         AnalyzeRing(repref);
-      } else if (repref.rep->tag == BTREE) {
+        break;
+      case CordRepKind::BTREE:
         AnalyzeBtree(repref);
         AnalyzeBtree(repref);
-      } else {
-        // We should have either a concat, btree, or ring node if not null.
-        assert(false);
-      }
+        break;
+      default:
+        // We should have either a btree or ring node if not null.
+        ABSL_ASSERT(repref.tag() == CordRepKind::UNUSED_0);
+        break;
     }
     }
 
 
     // Adds values to output
     // Adds values to output
@@ -122,11 +124,19 @@ class CordRepAnalyzer {
     const CordRep* rep;
     const CordRep* rep;
     size_t refcount;
     size_t refcount;
 
 
-    // Returns a 'child' RepRef which contains the cumulative reference count of
-    // this instance multiplied by the child's reference count.
+    // Returns a 'child' RepRef which contains the cumulative reference count
+    // of this instance multiplied by the child's reference count. Returns a
+    // nullptr RepRef value with a refcount of 0 if `child` is nullptr.
     RepRef Child(const CordRep* child) const {
     RepRef Child(const CordRep* child) const {
+      if (child == nullptr) return RepRef{nullptr, 0};
       return RepRef{child, refcount * child->refcount.Get()};
       return RepRef{child, refcount * child->refcount.Get()};
     }
     }
+
+    // Returns the tag of this rep, or UNUSED_0 if this instance is null
+    constexpr CordRepKind tag() const {
+      ABSL_ASSERT(rep == nullptr || rep->tag != CordRepKind::UNUSED_0);
+      return rep ? static_cast<CordRepKind>(rep->tag) : CordRepKind::UNUSED_0;
+    }
   };
   };
 
 
   // Memory usage values
   // Memory usage values
@@ -167,7 +177,7 @@ class CordRepAnalyzer {
   // buffers where we count children unrounded.
   // buffers where we count children unrounded.
   RepRef CountLinearReps(RepRef rep, MemoryUsage& memory_usage) {
   RepRef CountLinearReps(RepRef rep, MemoryUsage& memory_usage) {
     // Consume all substrings
     // Consume all substrings
-    while (rep.rep->tag == SUBSTRING) {
+    while (rep.tag() == SUBSTRING) {
       statistics_.node_count++;
       statistics_.node_count++;
       statistics_.node_counts.substring++;
       statistics_.node_counts.substring++;
       memory_usage.Add(sizeof(CordRepSubstring), rep.refcount);
       memory_usage.Add(sizeof(CordRepSubstring), rep.refcount);
@@ -175,7 +185,7 @@ class CordRepAnalyzer {
     }
     }
 
 
     // Consume possible FLAT
     // Consume possible FLAT
-    if (rep.rep->tag >= FLAT) {
+    if (rep.tag() >= FLAT) {
       size_t size = rep.rep->flat()->AllocatedSize();
       size_t size = rep.rep->flat()->AllocatedSize();
       CountFlat(size);
       CountFlat(size);
       memory_usage.Add(size, rep.refcount);
       memory_usage.Add(size, rep.refcount);
@@ -183,7 +193,7 @@ class CordRepAnalyzer {
     }
     }
 
 
     // Consume possible external
     // Consume possible external
-    if (rep.rep->tag == EXTERNAL) {
+    if (rep.tag() == EXTERNAL) {
       statistics_.node_count++;
       statistics_.node_count++;
       statistics_.node_counts.external++;
       statistics_.node_counts.external++;
       size_t size = rep.rep->length + sizeof(CordRepExternalImpl<intptr_t>);
       size_t size = rep.rep->length + sizeof(CordRepExternalImpl<intptr_t>);

absl/strings/internal/cordz_info_statistics_test.cc

@@ -452,8 +452,7 @@ TEST(CordzInfoStatisticsTest, BtreeNodeShared) {
 TEST(CordzInfoStatisticsTest, Crc) {
 TEST(CordzInfoStatisticsTest, Crc) {
   RefHelper ref;
   RefHelper ref;
   auto* left = Flat(1000);
   auto* left = Flat(1000);
-  auto* crc =
-      ref.NeedsUnref(CordRepCrc::New(left, crc_internal::CrcCordState()));
+  auto* crc = ref.NeedsUnref(CordRepCrc::New(left, {}));
 
 
   CordzStatistics expected;
   CordzStatistics expected;
   expected.size = left->length;
   expected.size = left->length;
@@ -467,6 +466,20 @@ TEST(CordzInfoStatisticsTest, Crc) {
   EXPECT_THAT(SampleCord(crc), EqStatistics(expected));
   EXPECT_THAT(SampleCord(crc), EqStatistics(expected));
 }
 }
 
 
+TEST(CordzInfoStatisticsTest, EmptyCrc) {
+  RefHelper ref;
+  auto* crc = ref.NeedsUnref(CordRepCrc::New(nullptr, {}));
+
+  CordzStatistics expected;
+  expected.size = 0;
+  expected.estimated_memory_usage = SizeOf(crc);
+  expected.estimated_fair_share_memory_usage = expected.estimated_memory_usage;
+  expected.node_count = 1;
+  expected.node_counts.crc = 1;
+
+  EXPECT_THAT(SampleCord(crc), EqStatistics(expected));
+}
+
 TEST(CordzInfoStatisticsTest, ThreadSafety) {
 TEST(CordzInfoStatisticsTest, ThreadSafety) {
   Notification stop;
   Notification stop;
   static constexpr int kNumThreads = 8;
   static constexpr int kNumThreads = 8;
@@ -497,6 +510,7 @@ TEST(CordzInfoStatisticsTest, ThreadSafety) {
         InlineData cords[2];
         InlineData cords[2];
         std::minstd_rand gen;
         std::minstd_rand gen;
         std::uniform_int_distribution<int> coin_toss(0, 1);
         std::uniform_int_distribution<int> coin_toss(0, 1);
+        std::uniform_int_distribution<int> dice_roll(1, 6);
 
 
         while (!stop.HasBeenNotified()) {
         while (!stop.HasBeenNotified()) {
           for (InlineData& cord : cords) {
           for (InlineData& cord : cords) {
@@ -523,6 +537,18 @@ TEST(CordzInfoStatisticsTest, ThreadSafety) {
                     rep = CordRepBtree::Create(rep);
                     rep = CordRepBtree::Create(rep);
                   }
                   }
                 }
                 }
+
+                // Maybe CRC this cord
+                if (dice_roll(gen) == 6) {
+                  if (dice_roll(gen) == 6) {
+                    // Empty CRC rep
+                    CordRep::Unref(rep);
+                    rep = CordRepCrc::New(nullptr, {});
+                  } else {
+                    // Regular CRC rep
+                    rep = CordRepCrc::New(rep, {});
+                  }
+                }
                 cord.make_tree(rep);
                 cord.make_tree(rep);
 
 
                 // 50/50 sample
                 // 50/50 sample