Fix lastLeq bug

ConflictSet.cpp

@@ -544,7 +544,9 @@ Node *&getOrCreateChild(Node *&self, uint8_t index) {
 }
 
 // Precondition - an entry for index must exist in the node
-[[maybe_unused]] void eraseChild(Node *self, uint8_t index) {
+void eraseChild(Node *self, uint8_t index) {
+  free(getChildExists(self, index));
+
   if (self->type == Type::Node4) {
     auto *self4 = static_cast<Node4 *>(self);
     int nodeIndex = getNodeIndex(self4, index);
@@ -654,7 +656,8 @@ std::string_view getSearchPath(Arena &arena, Node *n) {
   }
   std::reverse(result.begin(), result.end());
   if (result.size() > 0) {
-    return std::string_view((const char *)&result[0], result.size()); // NOLINT
+    return std::string_view((const char *)&result[0],
+                            result.size()); // NOLINT
   } else {
     return std::string_view();
   }
@@ -672,8 +675,10 @@ Iterator lastLeq(Node *n, const std::span<const uint8_t> key) {
         }
         if (c > 0) {
           n = prevPhysical(n);
+          goto scanBackward;
+        } else {
+          goto downRightSpine;
         }
-        goto outerLoop;
       }
       if (commonLen == n->partialKeyLen) {
         // partial key matches
@@ -682,7 +687,7 @@ Iterator lastLeq(Node *n, const std::span<const uint8_t> key) {
         // n is the first physical node greater than remaining, and there's no
         // eq node
         n = prevPhysical(n);
-        goto outerLoop;
+        goto scanBackward;
       }
     }
     {
@@ -696,7 +701,7 @@ Iterator lastLeq(Node *n, const std::span<const uint8_t> key) {
       if (n->entryPresent) {
         return {n, 0};
       } else {
-        goto outerLoop;
+        goto scanBackward;
       }
     } else {
       int c = getChildLeq(n, remaining[0]);
@@ -704,22 +709,29 @@ Iterator lastLeq(Node *n, const std::span<const uint8_t> key) {
         n = getChildExists(n, c);
         remaining = remaining.subspan(1, remaining.size() - 1);
       } else {
-        // The physical node corresponding to search path `key` does not exist.
+        if (c >= 0) {
-        // Let's find the physical node corresponding to the highest search key
+          n = getChildExists(n, c);
-        // (not necessarily present) less than key
+          goto downRightSpine;
-        // Move down the right spine
+        } else {
-        for (;;) {
+          goto scanBackward;
-          if (c >= 0) {
-            n = getChildExists(n, c);
-          } else {
-            goto outerLoop;
-          }
-          c = getChildLeq(n, 255);
         }
       }
     }
   }
-outerLoop:
+downRightSpine:
+  // The physical node corresponding to search path `key` does not
+  // exist. Let's find the physical node corresponding to the highest
+  // search key (not necessarily present) less than key.
+  // Move down the right spine
+  for (;;) {
+    int c = getChildLeq(n, 255);
+    if (c >= 0) {
+      n = getChildExists(n, c);
+    } else {
+      goto scanBackward;
+    }
+  }
+scanBackward:
   // Iterate backwards along existing physical nodes until we find a present
   // entry
   for (; !n->entryPresent; n = prevPhysical(n)) {
@@ -802,6 +814,21 @@ void destroyTree(Node *root) {
   }
 }
 
+void invalidateMax(Node *self) {
+  int64_t expectedMax = std::numeric_limits<int64_t>::lowest();
+  if (self->entryPresent) {
+    expectedMax = std::max(expectedMax, self->entry.pointVersion);
+    expectedMax = std::max(expectedMax, self->entry.rangeVersion);
+  }
+  for (int i = getChildGeq(self, 0); i >= 0; i = getChildGeq(self, i + 1)) {
+    expectedMax = std::max(expectedMax, getChildExists(self, i)->maxVersion);
+  }
+  self->maxVersion = expectedMax;
+  if (self->parent != nullptr) {
+    invalidateMax(self->parent);
+  }
+}
+
 struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
   void check(const ReadRange *reads, Result *result, int count) const {
     for (int i = 0; i < count; ++i) {
@@ -816,8 +843,8 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
           lastLeq(root, std::span<const uint8_t>(r.begin.p, r.begin.len));
 #if DEBUG_VERBOSE && !defined(NDEBUG)
       Arena arena;
-      printf("LastLeq for `%s' got `%s'\n", printable(r.begin).c_str(),
+      fprintf(stderr, "LastLeq for `%s' got `%s'\n", printable(r.begin).c_str(),
-             printable(getSearchPath(arena, l)).c_str());
+              printable(getSearchPath(arena, l)).c_str());
 #endif
       assert(l != nullptr);
       assert(l->entryPresent);
@@ -839,22 +866,25 @@ struct __attribute__((visibility("hidden"))) ConflictSet::Impl {
           n->entryPresent = true;
           n->entry.pointVersion = p->entry.rangeVersion;
           n->entry.rangeVersion = p->entry.rangeVersion;
-          n->maxVersion = p->entry.rangeVersion;
         }
 
         auto *end = n;
         n = insert(&root, std::span<const uint8_t>(w.begin.p, w.begin.len),
-                   w.writeVersion);
+                   std::numeric_limits<int64_t>::lowest());
+        auto *begin = n;
         n->entryPresent = true;
         n->entry.pointVersion = w.writeVersion;
         n->entry.rangeVersion = w.writeVersion;
         for (n = nextLogical(n); n != end;) {
           auto *old = n;
           n = nextLogical(n);
+          old->entryPresent = false;
           if (old->numChildren == 0 && old->parent != nullptr) {
             eraseChild(old->parent, old->parentsIndex);
           }
         }
+        invalidateMax(begin);
+        invalidateMax(end);
       } else {
         auto *n =
             insert(&root, std::span<const uint8_t>(w.begin.p, w.begin.len),
@@ -1138,14 +1168,23 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
   TestDriver<ConflictSet::Impl> driver{data, size};
   static_assert(driver.kMaxKeyLen > Node::kPartialKeyMaxLen);
 
-  do {
+  for (;;) {
+    bool done = driver.next();
+    if (!driver.ok) {
+      debugPrintDot(stdout, driver.cs.root);
+      fflush(stdout);
+      abort();
+    }
     bool success = checkCorrectness(driver.cs.root, driver.refImpl);
     if (!success) {
       debugPrintDot(stdout, driver.cs.root);
       fflush(stdout);
       abort();
     }
-  } while (!driver.next());
+    if (done) {
+      break;
+    }
+  }
 
   return 0;
 }

Internal.h

@@ -7,6 +7,7 @@
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
+#include <inttypes.h>
 #include <map>
 #include <set>
 #include <span>
@@ -14,7 +15,7 @@
 #include <utility>
 #include <vector>
 
-#define DEBUG_VERBOSE 1
+#define DEBUG_VERBOSE 0
 
 // This header contains code that we want to reuse outside of ConflictSet.cpp or
 // want to exclude from coverage since it's only testing related.
@@ -451,6 +452,19 @@ template <class ConflictSetImpl> struct TestDriver {
 
   constexpr static auto kMaxKeyLen = 24;
 
+  bool ok = true;
+
+  static const char *resultToStr(ConflictSet::Result r) {
+    switch (r) {
+    case ConflictSet::Commit:
+      return "commit";
+    case ConflictSet::Conflict:
+      return "conflict";
+    case ConflictSet::TooOld:
+      return "too old";
+    }
+  }
+
   // Call until it returns true, for "done". Check internal invariants etc
   // between calls to next.
   bool next() {
@@ -537,10 +551,12 @@ template <class ConflictSetImpl> struct TestDriver {
       refImpl.check(reads, results2, numReads);
       for (int i = 0; i < numReads; ++i) {
         if (results1[i] != results2[i]) {
-          fprintf(stderr, "Expected %d, got %d for read of %s at version %d\n",
+          fprintf(stderr,
-                  results2[i], results1[i], printable(reads[i].begin).c_str(),
+                  "Expected %s, got %s for read of %s at version %" PRId64 "\n",
-                  int(reads[i].readVersion));
+                  resultToStr(results2[i]), resultToStr(results1[i]),
-          abort();
+                  printable(reads[i].begin).c_str(), reads[i].readVersion);
+          ok = false;
+          return true;
         }
       }
     }

TestDriver.cpp

@@ -13,5 +13,8 @@ int main(int argc, char **argv) {
     TestDriver<ConflictSet> driver{(const uint8_t *)str.data(), str.size()};
     while (!driver.next())
       ;
+    if (!driver.ok) {
+      abort();
+    }
   }
 }

corpus/5067455bd470921ea92a315e2404b0b0438804dd

@@ -1 +0,0 @@
-<EFBFBD><EFBFBD><EFBFBD><EFBFBD><EFBFBD><EFBFBD><EFBFBD>'<27><><EFBFBD><EFBFBD><EFBFBD><07>‚<EFBFBD><C282><EFBFBD><EFBFBD><EFBFBD><16>