From e9d6a0d7ce763392b695851924100b09540a70a1 Mon Sep 17 00:00:00 2001 From: Changyu Bi Date: Wed, 18 Jan 2023 16:38:07 -0800 Subject: [PATCH] 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: https://github.com/facebook/rocksdb/pull/11106 Test Plan: Added UT for repro. Reviewed By: ajkr Differential Revision: D42590862 Pulled By: cbi42 fbshipit-source-id: c493265ececdf89636d801d55ae929806c4d4b2c --- db/builder.cc | 4 ++-- db/compaction/compaction_outputs.cc | 4 ++-- db/db_with_timestamp_basic_test.cc | 28 ++++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/db/builder.cc b/db/builder.cc index 4cea24bfd..a84bd5a45 100644 --- a/db/builder.cc +++ b/db/builder.cc @@ -253,7 +253,7 @@ 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( @@ -261,7 +261,7 @@ Status BuildTable( 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 index 01333d774..598bffb24 100644 --- 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 index 6ea1aaf46..420816923 100644 --- 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(db_->DefaultColumnFamily()) + ->cfd(), + 0 /* input_level */, 1 /* output_level */, CompactRangeOptions(), + nullptr /* begin */, nullptr /* end */, true /* exclusive */, + true /* disallow_trivial_move */, + std::numeric_limits::max() /* max_file_num_to_ignore */, + "" /*trim_ts*/)); +} } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) {