From cdabbdf32c87ceac8adeb1ea0b9fa9a9166726f0 Mon Sep 17 00:00:00 2001
From: Andrew Kryczka <andrew.kryczka2@gmail.com>
Date: Wed, 24 Jul 2019 12:59:57 -0700
Subject: [PATCH] libroach: fix infinite loop in reverse `MVCCScan()`

It happened when multiple versions of a key existed as the smallest key
in the reverse-scanned range, and a certain number of those versions
were newer than the scan timestamp. See test case for full root-cause
details.

Fixes #38788.

Release note (bug fix): Fixed a potential infinite loop in queries
involving reverse scans.
---
 c-deps/libroach/mvcc.h          |  6 ++++
 pkg/storage/engine/mvcc_test.go | 56 +++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/c-deps/libroach/mvcc.h b/c-deps/libroach/mvcc.h
index d856be9a75ae..f44bb668cf68 100644
--- a/c-deps/libroach/mvcc.h
+++ b/c-deps/libroach/mvcc.h
@@ -403,6 +403,12 @@ template <bool reverse> class mvccScanner {
       if (!iterPeekPrev(&peeked_key)) {
         return false;
       }
+      if (peeked_key.empty()) {
+        // `iterPeekPrev()` may return true even when it did not find a key. This
+        // case is indicated by `peeked_key.empty()`. In that case there is not
+        // going to be any prev key, so we are done.
+        return false;
+      }
       if (peeked_key != key_buf_) {
         return backwardLatestVersion(peeked_key, i + 1);
       }
diff --git a/pkg/storage/engine/mvcc_test.go b/pkg/storage/engine/mvcc_test.go
index ccdc8b6cfded..b6fd66a4d408 100644
--- a/pkg/storage/engine/mvcc_test.go
+++ b/pkg/storage/engine/mvcc_test.go
@@ -2954,6 +2954,62 @@ func TestMVCCReverseScanSeeksOverRepeatedKeys(t *testing.T) {
 	}
 }
 
+// Exposes a bug where the reverse MVCC scan can get stuck in an infinite loop until we OOM.
+//
+// The bug happened in this scenario.
+// (1) reverse scan is positioned at the range's smallest key and calls `prevKey()`
+// (2) `prevKey()` peeks and sees newer versions of the same logical key
+//     `iters_before_seek_-1` times, moving the iterator backwards each time
+// (3) on the `iters_before_seek_`th peek, there are no previous keys found
+//
+// Then, the problem was `prevKey()` treated finding no previous key as if it had found a
+// new logical key with the empty string. It would use `backwardLatestVersion()` to find
+// the latest version of this empty string logical key. Due to condition (3),
+// `backwardLatestVersion()` would go directly to its seeking optimization rather than
+// trying to incrementally move backwards (if it had tried moving incrementally backwards,
+// it would've noticed it's out of bounds). The seek optimization would then seek to "\0",
+// which is the empty logical key with zero timestamp. Since we set RocksDB iterator lower
+// bound to be the lower bound of the range scan, this seek actually lands back at the
+// range's smallest key. It thinks it found a new key so it adds it to the result, and then
+// this whole process repeats ad infinitum.
+func TestMVCCReverseScanStopAtSmallestKey(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	run := func(numPuts int, ts int64) {
+		ctx := context.Background()
+		engine := createTestEngine()
+		defer engine.Close()
+
+		for i := 1; i <= numPuts; i++ {
+			if err := MVCCPut(ctx, engine, nil, testKey1, hlc.Timestamp{WallTime: int64(i)}, value1, nil); err != nil {
+				t.Fatal(err)
+			}
+		}
+
+		kvs, _, _, err := MVCCScan(ctx, engine, testKey1, testKey3, math.MaxInt64,
+			hlc.Timestamp{WallTime: ts}, MVCCScanOptions{Reverse: true})
+		if err != nil {
+			t.Fatal(err)
+		}
+		if len(kvs) != 1 ||
+			!bytes.Equal(kvs[0].Key, testKey1) ||
+			!bytes.Equal(kvs[0].Value.RawBytes, value1.RawBytes) {
+			t.Fatal("unexpected scan results")
+		}
+	}
+	// Satisfying (2) and (3) is incredibly intricate because of how `iters_before_seek_`
+	// is incremented/decremented heuristically. For example, at the time of writing, the
+	// infinitely looping cases are `numPuts == 6 && ts == 2`, `numPuts == 7 && ts == 3`,
+	// `numPuts == 8 && ts == 4`, `numPuts == 9 && ts == 5`, and `numPuts == 10 && ts == 6`.
+	// Tying our test case to the `iters_before_seek_` setting logic seems brittle so let's
+	// just brute force test a wide range of cases.
+	for numPuts := 1; numPuts <= 10; numPuts++ {
+		for ts := 1; ts <= 10; ts++ {
+			run(numPuts, int64(ts))
+		}
+	}
+
+}
+
 func TestMVCCResolveTxn(t *testing.T) {
 	defer leaktest.AfterTest(t)()