Fix asan failure caused by range tombstone start key use-after-free (…

…#11106) Summary: the `last_tombstone_start_user_key` variable in `BuildTable()` and in `CompactionOutputs::AddRangeDels()` may point to a start key that is freed if user-defined timestamp is enabled. This was causing ASAN failure and this PR fixes this issue. Pull Request resolved: facebook/rocksdb#11106 Test Plan: Added UT for repro. Reviewed By: ajkr Differential Revision: D42590862 Pulled By: cbi42 fbshipit-source-id: c493265ececdf89636d801d55ae929806c4d4b2c
awitten1 · Jan 19, 2023 · e9d6a0d · e9d6a0d
1 parent bd4b8d6
commit e9d6a0d
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 4 deletions.
diff --git a/db/builder.cc b/db/builder.cc
@@ -253,15 +253,15 @@ Status BuildTable(
         if (version) {
           if (last_tombstone_start_user_key.empty() ||
               ucmp->CompareWithoutTimestamp(last_tombstone_start_user_key,
-                                            tombstone.start_key_) < 0) {
+                                            range_del_it->start_key()) < 0) {
             SizeApproximationOptions approx_opts;
             approx_opts.files_size_error_margin = 0.1;
             meta->compensated_range_deletion_size += versions->ApproximateSize(
                 approx_opts, version, kv.first.Encode(), tombstone_end.Encode(),
                 0 /* start_level */, -1 /* end_level */,
                 TableReaderCaller::kFlush);
           }
-          last_tombstone_start_user_key = tombstone.start_key_;
+          last_tombstone_start_user_key = range_del_it->start_key();
         }
       }
     }

diff --git a/db/compaction/compaction_outputs.cc b/db/compaction/compaction_outputs.cc
@@ -616,8 +616,8 @@ Status CompactionOutputs::AddRangeDels(
       bool start_user_key_changed =
           last_tombstone_start_user_key.empty() ||
           ucmp->CompareWithoutTimestamp(last_tombstone_start_user_key,
-                                        tombstone.start_key_) < 0;
-      last_tombstone_start_user_key = tombstone.start_key_;
+                                        it->start_key()) < 0;
+      last_tombstone_start_user_key = it->start_key();
       // Range tombstones are truncated at file boundaries
       if (icmp.Compare(tombstone_start, meta.smallest) < 0) {
         tombstone_start = meta.smallest;

diff --git a/db/db_with_timestamp_basic_test.cc b/db/db_with_timestamp_basic_test.cc
@@ -3870,6 +3870,34 @@ TEST_F(DBBasicTestWithTimestamp, MergeAfterDeletion) {
 
   Close();
 }
+
+TEST_F(DBBasicTestWithTimestamp, RangeTombstoneApproximateSize) {
+  // Test code path for calculating range tombstone compensated size
+  // during flush and compaction.
+  Options options = CurrentOptions();
+  const size_t kTimestampSize = Timestamp(0, 0).size();
+  TestComparator test_cmp(kTimestampSize);
+  options.comparator = &test_cmp;
+  DestroyAndReopen(options);
+  // So that the compaction below is non-bottommost and will calcualte
+  // compensated range tombstone size.
+  ASSERT_OK(db_->Put(WriteOptions(), Key(1), Timestamp(1, 0), "val"));
+  ASSERT_OK(Flush());
+  MoveFilesToLevel(5);
+  ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), Key(0),
+                             Key(1), Timestamp(1, 0)));
+  ASSERT_OK(db_->DeleteRange(WriteOptions(), db_->DefaultColumnFamily(), Key(1),
+                             Key(2), Timestamp(2, 0)));
+  ASSERT_OK(Flush());
+  ASSERT_OK(dbfull()->RunManualCompaction(
+      static_cast_with_check<ColumnFamilyHandleImpl>(db_->DefaultColumnFamily())
+          ->cfd(),
+      0 /* input_level */, 1 /* output_level */, CompactRangeOptions(),
+      nullptr /* begin */, nullptr /* end */, true /* exclusive */,
+      true /* disallow_trivial_move */,
+      std::numeric_limits<uint64_t>::max() /* max_file_num_to_ignore */,
+      "" /*trim_ts*/));
+}
 }  // namespace ROCKSDB_NAMESPACE
 
 int main(int argc, char** argv) {