Merge cockroachdb#35505

35505: libroach: prevent infinite loop in reverse-scan r=ajkr a=ajkr

Fixed and added a test case for infinite loop leading to OOM in reverse-scan
optimization to use `SeekForPrev()` after N `Prev()`s fail to find a new
key. It could happen when that optimization was used on a key that also
had a write intent.

Release note: None


Co-authored-by: Andrew Kryczka <[email protected]>

Makefile

@@ -606,7 +606,8 @@ $(LIBROACH_DIR)/Makefile: $(C_DEPS_DIR)/libroach-rebuild | bin/.submodules-initi
 	mkdir -p $(LIBROACH_DIR)
 	@# NOTE: If you change the CMake flags below, bump the version in
 	@# $(C_DEPS_DIR)/libroach-rebuild. See above for rationale.
-	cd $(LIBROACH_DIR) && cmake $(xcmake-flags) $(LIBROACH_SRC_DIR) -DCMAKE_BUILD_TYPE=Release \
+	cd $(LIBROACH_DIR) && cmake $(xcmake-flags) $(LIBROACH_SRC_DIR) \
+		-DCMAKE_BUILD_TYPE=$(if $(ENABLE_LIBROACH_ASSERTIONS),Debug,Release) \
 		-DPROTOBUF_LIB=$(LIBPROTOBUF) -DROCKSDB_LIB=$(LIBROCKSDB) \
 		-DJEMALLOC_LIB=$(LIBJEMALLOC) -DSNAPPY_LIB=$(LIBSNAPPY) \
 		-DCRYPTOPP_LIB=$(LIBCRYPTOPP)

c-deps/libroach/mvcc.h

@@ -545,10 +545,12 @@ template <bool reverse> class mvccScanner {
   bool iterSeekReverse(const rocksdb::Slice& key) {
     clearPeeked();
 
-    // SeekForPrev positions the iterator at the key that is less than
-    // key. NB: the doc comment on SeekForPrev suggests it positions
-    // less than or equal, but this is a lie.
+    // `SeekForPrev` positions the iterator at the last key that is less than or
+    // equal to `key` AND strictly less than `ReadOptions::iterate_upper_bound`.
     iter_rep_->SeekForPrev(key);
+    if (iter_rep_->Valid() && key.compare(iter_rep_->key()) == 0) {
+      iter_rep_->Prev();
+    }
     if (!updateCurrent()) {
       return false;
     }

pkg/storage/engine/mvcc_test.go

@@ -2917,6 +2917,43 @@ func TestMVCCReverseScanFirstKeyInFuture(t *testing.T) {
 	}
 }
 
+// Exposes a bug where the reverse MVCC scan can get stuck in an infinite loop
+// until we OOM. It happened in the code path optimized to use `SeekForPrev()`
+// after N `Prev()`s do not reach another logical key. Further, a write intent
+// needed to be present on the logical key to make it conflict with our chosen
+// `SeekForPrev()` target (logical key + '\0').
+func TestMVCCReverseScanSeeksOverRepeatedKeys(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	ctx := context.Background()
+	engine := createTestEngine()
+	defer engine.Close()
+
+	// 10 is the value of `kMaxItersBeforeSeek` at the time this test case was
+	// written. Repeat the key enough times to make sure the `SeekForPrev()`
+	// optimization will be used.
+	for i := 1; i <= 10; i++ {
+		if err := MVCCPut(ctx, engine, nil, testKey2, hlc.Timestamp{WallTime: int64(i)}, value2, nil); err != nil {
+			t.Fatal(err)
+		}
+	}
+	txn1ts := makeTxn(*txn1, hlc.Timestamp{WallTime: 11})
+	if err := MVCCPut(ctx, engine, nil, testKey2, txn1ts.OrigTimestamp, value2, txn1ts); err != nil {
+		t.Fatal(err)
+	}
+
+	kvs, _, _, err := MVCCScan(ctx, engine, testKey1, testKey3, math.MaxInt64,
+		hlc.Timestamp{WallTime: 1}, MVCCScanOptions{Reverse: true})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(kvs) != 1 ||
+		!bytes.Equal(kvs[0].Key, testKey2) ||
+		!bytes.Equal(kvs[0].Value.RawBytes, value2.RawBytes) {
+		t.Fatal("unexpected scan results")
+	}
+}
+
 func TestMVCCResolveTxn(t *testing.T) {
 	defer leaktest.AfterTest(t)()