From 0ca1d3c546c7109e5e25ee0ec83fc3e5e0bc0fc3 Mon Sep 17 00:00:00 2001
From: Weaselbot <weaselbot@weaselab.dev>
Date: Tue, 10 Nov 2026 15:43:58 -0500
Subject: [PATCH] Guard memcpy/memcmp calls in SkipList.cpp against null
 pointers

keyAfter() copied the caller's key with an unguarded memcpy, so a zero-length
key spelled with a null pointer (which the header allows -- it only constrains
Key::len) became memcpy(dst, nullptr, 0). That is undefined behavior even
though the count is 0, and keyAfter() runs for every point read and point
write, so any empty-key point operation tripped UBSan builds.

Commit 2231c09 fixed this pattern in operator<(const KeyInfo&),
SkipList::less and copyToArena, but missed keyAfter(). Apply the same guard
here, and guard the remaining unguarded memcmp calls in the same file
(compare(), Finger::found() and the point-read fast path in
CheckMax::advance()) via a small equalBytes() helper.

Add test_empty_key_null_pointer, which bisimulates both implementations on an
empty key spelled with a null pointer. It fails with a UBSan trap on
SkipList.cpp:34 before this change and passes after.
---
 SkipList.cpp         | 20 ++++++++++++++++----
 test_conflict_set.py | 21 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/SkipList.cpp b/SkipList.cpp
index d64d550..aa6cd96 100644
--- a/SkipList.cpp
+++ b/SkipList.cpp
@@ -31,7 +31,9 @@
 std::span<const uint8_t> keyAfter(Arena &arena, std::span<const uint8_t> key) {
   auto result =
       std::span<uint8_t>(new (arena) uint8_t[key.size() + 1], key.size() + 1);
-  memcpy(result.data(), key.data(), key.size());
+  if (key.size() > 0) {
+    memcpy(result.data(), key.data(), key.size());
+  }
   result[result.size() - 1] = 0;
   return result;
 }
@@ -50,6 +52,14 @@ using Version = int64_t;
 #define force_inline inline __attribute__((always_inline))
 using StringRef = std::span<const uint8_t>;
 
+// memcmp, but safe for zero-length ranges: memcmp is undefined behavior unless
+// both pointers are valid, even when the count is 0, and spans for the empty
+// key are allowed to have a null data().
+static force_inline bool equalBytes(const uint8_t *a, const uint8_t *b,
+                                    int size) {
+  return size == 0 || memcmp(a, b, size) == 0;
+}
+
 struct KeyRangeRef {
   StringRef begin;
   StringRef end;
@@ -180,7 +190,8 @@ void sortPoints(std::vector<KeyInfo> &points) {
 }
 
 static int compare(const StringRef &a, const StringRef &b) {
-  int c = memcmp(a.data(), b.data(), std::min(a.size(), b.size()));
+  int n = std::min(a.size(), b.size());
+  int c = n > 0 ? memcmp(a.data(), b.data(), n) : 0;
   if (c < 0)
     return -1;
   if (c > 0)
@@ -371,7 +382,7 @@ public:
       Node *n = finger[0]->getNext(
           0); // or alreadyChecked, but that is more easily invalidated
       if (n && n->length() == value.size() &&
-          !memcmp(n->value(), value.data(), value.size()))
+          equalBytes(n->value(), value.data(), value.size()))
         return n;
       else
         return nullptr;
@@ -695,7 +706,8 @@ private:
           s = nextS;
           if (start.finished()) {
             if (nextS->length() == start.value.size() &&
-                !memcmp(nextS->value(), start.value.data(), start.value.size()))
+                equalBytes(nextS->value(), start.value.data(),
+                           start.value.size()))
               return noConflict();
             else
               return conflict();
diff --git a/test_conflict_set.py b/test_conflict_set.py
index 924e89e..36cf49a 100644
--- a/test_conflict_set.py
+++ b/test_conflict_set.py
@@ -1,4 +1,6 @@
 import struct
+
+import conflict_set
 from conflict_set import *
 
 
@@ -251,6 +253,25 @@ def test_merge_child_node48():
         cs.addWrites(1, write(b"\x00" * 8, b"\x00" * 10))
 
 
+def test_empty_key_null_pointer():
+    # Regression test for issue #96: the header only constrains the length of a
+    # key, so a zero-length key may be spelled with a null pointer. Both
+    # implementations must agree on such a key, and must not pass the null
+    # pointer to memcpy/memcmp, which is undefined behavior even when the length
+    # is 0 (keyAfter() in SkipList.cpp did, on every empty-key point operation).
+    null_key = conflict_set._Key(None, 0)
+    with DebugConflictSet() as cs:
+        cs.addWrites(1, WriteRange(null_key, null_key))
+        assert cs.check(ReadRange(null_key, null_key, 0)) == [Result.CONFLICT]
+        assert cs.check(ReadRange(null_key, null_key, 1)) == [Result.COMMIT]
+
+    # The same operations spelled with a non-null empty key must agree.
+    with DebugConflictSet() as cs:
+        cs.addWrites(1, write(b""))
+        assert cs.check(read(0, b"")) == [Result.CONFLICT]
+        assert cs.check(read(1, b"")) == [Result.COMMIT]
+
+
 def test_fixup_256_heterogeneous_versions():
     # Regression test for issue #89: recomputing the maxVersion of a Node256
     # must consider all of its children, not just those with an index < 16.