From 124af42830663fa2d07e335db2968413b0ef955e Mon Sep 17 00:00:00 2001
From: Sergei Politov <spolitov@gmail.com>
Date: Thu, 1 Jul 2021 23:25:59 +0300
Subject: [PATCH] [#9162] Fix stack overflow in filtering iterator

Summary:
Filtering iterator could make a call to Next/Prev while checking filter.
But those methods also would check filter.
So, iterating over one skipped entry results in adding two more stack frames, one for Next() or Prev() and one more for ApplyFilter(), and the stack grows proportionally to the number of skipped entries, resulting in a potential stack overflow.

Actually we should call Next/Prev of the underlying iterator in this case, as done in this diff. This approach would only utilize a constant amount of space on the stack and not linear space based on the number of skipped entries.

Test Plan: ybd debug --gtest_filter BackupTxnTest.PointInTimeBigSkipRestore

Reviewers: bogdan, mbautin

Reviewed By: mbautin

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D12143
---
 src/yb/client/backup-txn-test.cc          | 32 +++++++++++++++++++++++
 src/yb/rocksdb/table/filtering_iterator.h |  4 +--
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/src/yb/client/backup-txn-test.cc b/src/yb/client/backup-txn-test.cc
index 10c3d63e6d4f..fd6b9e00b61e 100644
--- a/src/yb/client/backup-txn-test.cc
+++ b/src/yb/client/backup-txn-test.cc
@@ -205,6 +205,38 @@ TEST_F(BackupTxnTest, PointInTimeRestore) {
   ASSERT_NO_FATALS(VerifyData(/* num_transactions=*/ 1, WriteOpType::INSERT));
 }
 
+TEST_F(BackupTxnTest, PointInTimeBigSkipRestore) {
+  constexpr int kNumWrites = RegularBuildVsSanitizers(100000, 100);
+  constexpr int kKey = 123;
+
+  std::vector<std::future<FlushStatus>> futures;
+  auto session = CreateSession();
+  ASSERT_OK(WriteRow(session, kKey, 0));
+  auto hybrid_time = cluster_->mini_tablet_server(0)->server()->Clock()->Now();
+  for (size_t r = 1; r <= kNumWrites; ++r) {
+    ASSERT_OK(WriteRow(session, kKey, r, WriteOpType::INSERT, client::Flush::kFalse));
+    futures.push_back(session->FlushFuture());
+  }
+
+  int good = 0;
+  for (auto& future : futures) {
+    if (future.get().status.ok()) {
+      ++good;
+    }
+  }
+
+  LOG(INFO) << "Total good: " << good;
+
+  auto snapshot_id = ASSERT_RESULT(CreateSnapshot());
+  ASSERT_OK(VerifySnapshot(snapshot_id, SysSnapshotEntryPB::COMPLETE));
+
+  ASSERT_OK(RestoreSnapshot(snapshot_id, hybrid_time));
+
+  auto value = ASSERT_RESULT(SelectRow(session, kKey));
+  ASSERT_EQ(value, 0);
+}
+
+
 TEST_F(BackupTxnTest, Persistence) {
   LOG(INFO) << "Write data";
 
diff --git a/src/yb/rocksdb/table/filtering_iterator.h b/src/yb/rocksdb/table/filtering_iterator.h
index 4d7a7b207560..5e7669e53aa7 100644
--- a/src/yb/rocksdb/table/filtering_iterator.h
+++ b/src/yb/rocksdb/table/filtering_iterator.h
@@ -103,9 +103,9 @@ class FilteringIterator : public InternalIterator {
         break;
       }
       if (!backward) {
-        Next();
+        iterator_->Next();
       } else {
-        Prev();
+        iterator_->Prev();
       }
     }
   }