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

2 files changed, 53 insertions, 17 deletions

diff --git a/absl/strings/internal/cordz_info.cc b/absl/strings/internal/cordz_info.cc
index 515dfafb..b830c25c 100644
--- a/absl/strings/internal/cordz_info.cc
+++ b/absl/strings/internal/cordz_info.cc
@@ -33,8 +33,6 @@ namespace absl {
 ABSL_NAMESPACE_BEGIN
 namespace cord_internal {
 
-using ::absl::base_internal::SpinLockHolder;
-
 #ifdef ABSL_INTERNAL_NEED_REDUNDANT_CONSTEXPR_DECL
 constexpr size_t CordzInfo::kMaxStackDepth;
 #endif
@@ -79,6 +77,8 @@ class CordRepAnalyzer {
   // adds the results to `statistics`. Note that node counts and memory sizes
   // are not initialized, computed values are added to any existing values.
   void AnalyzeCordRep(const CordRep* rep) {
+    ABSL_ASSERT(rep != nullptr);
+
     // Process all linear nodes.
     // 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.
@@ -86,7 +86,7 @@ class CordRepAnalyzer {
     RepRef repref{rep, (refcount > 1) ? refcount - 1 : 1};
 
     // Process the top level CRC node, if present.
-    if (repref.rep->tag == CRC) {
+    if (repref.tag() == CRC) {
       statistics_.node_count++;
       statistics_.node_counts.crc++;
       memory_usage_.Add(sizeof(CordRepCrc), repref.refcount);
@@ -96,15 +96,17 @@ class CordRepAnalyzer {
     // Process all top level linear nodes (substrings and flats).
     repref = CountLinearReps(repref, memory_usage_);
 
-    if (repref.rep != nullptr) {
-      if (repref.rep->tag == RING) {
+    switch (repref.tag()) {
+      case CordRepKind::RING:
         AnalyzeRing(repref);
-      } else if (repref.rep->tag == BTREE) {
+        break;
+      case CordRepKind::BTREE:
         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
@@ -122,11 +124,19 @@ class CordRepAnalyzer {
     const CordRep* rep;
     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 {
+      if (child == nullptr) return RepRef{nullptr, 0};
       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
@@ -167,7 +177,7 @@ class CordRepAnalyzer {
   // buffers where we count children unrounded.
   RepRef CountLinearReps(RepRef rep, MemoryUsage& memory_usage) {
     // Consume all substrings
-    while (rep.rep->tag == SUBSTRING) {
+    while (rep.tag() == SUBSTRING) {
       statistics_.node_count++;
       statistics_.node_counts.substring++;
       memory_usage.Add(sizeof(CordRepSubstring), rep.refcount);
@@ -175,7 +185,7 @@ class CordRepAnalyzer {
     }
 
     // Consume possible FLAT
-    if (rep.rep->tag >= FLAT) {
+    if (rep.tag() >= FLAT) {
       size_t size = rep.rep->flat()->AllocatedSize();
       CountFlat(size);
       memory_usage.Add(size, rep.refcount);
@@ -183,7 +193,7 @@ class CordRepAnalyzer {
     }
 
     // Consume possible external
-    if (rep.rep->tag == EXTERNAL) {
+    if (rep.tag() == EXTERNAL) {
       statistics_.node_count++;
       statistics_.node_counts.external++;
       size_t size = rep.rep->length + sizeof(CordRepExternalImpl<intptr_t>);
diff --git a/absl/strings/internal/cordz_info_statistics_test.cc b/absl/strings/internal/cordz_info_statistics_test.cc
index 53d2f2ea..aad3b1ec 100644
--- a/absl/strings/internal/cordz_info_statistics_test.cc
+++ b/absl/strings/internal/cordz_info_statistics_test.cc
@@ -452,8 +452,7 @@ TEST(CordzInfoStatisticsTest, BtreeNodeShared) {
 TEST(CordzInfoStatisticsTest, Crc) {
   RefHelper ref;
   auto* left = Flat(1000);
-  auto* crc =
-      ref.NeedsUnref(CordRepCrc::New(left, crc_internal::CrcCordState()));
+  auto* crc = ref.NeedsUnref(CordRepCrc::New(left, {}));
 
   CordzStatistics expected;
   expected.size = left->length;
@@ -467,6 +466,20 @@ TEST(CordzInfoStatisticsTest, Crc) {
   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) {
   Notification stop;
   static constexpr int kNumThreads = 8;
@@ -497,6 +510,7 @@ TEST(CordzInfoStatisticsTest, ThreadSafety) {
         InlineData cords[2];
         std::minstd_rand gen;
         std::uniform_int_distribution<int> coin_toss(0, 1);
+        std::uniform_int_distribution<int> dice_roll(1, 6);
 
         while (!stop.HasBeenNotified()) {
           for (InlineData& cord : cords) {
@@ -523,6 +537,18 @@ TEST(CordzInfoStatisticsTest, ThreadSafety) {
                     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);
 
                 // 50/50 sample