Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix GetMergeOperands() heap-use-after-free on flushed memtable #9805

Closed
wants to merge 2 commits into from
Closed

Fix GetMergeOperands() heap-use-after-free on flushed memtable #9805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
599ff0f
Fix GetMergeOperands() heap-use-after-free on flushed memtable
ajkr Apr 5, 2022
480d408
test slightly more
ajkr Apr 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions db/db_impl/db_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1889,6 +1889,8 @@ Status DBImpl::GetImpl(const ReadOptions& read_options, const Slice& key,
return s;
}
}
TEST_SYNC_POINT("DBImpl::GetImpl:PostMemTableGet:0");
TEST_SYNC_POINT("DBImpl::GetImpl:PostMemTableGet:1");
PinnedIteratorsManager pinned_iters_mgr;
if (!done) {
PERF_TIMER_GUARD(get_from_output_files_time);
Expand All @@ -1906,8 +1908,6 @@ Status DBImpl::GetImpl(const ReadOptions& read_options, const Slice& key,
{
PERF_TIMER_GUARD(get_post_process_time);

ReturnAndCleanupSuperVersion(cfd, sv);

RecordTick(stats_, NUMBER_KEYS_READ);
size_t size = 0;
if (s.ok()) {
Expand All @@ -1934,6 +1934,8 @@ Status DBImpl::GetImpl(const ReadOptions& read_options, const Slice& key,
PERF_COUNTER_ADD(get_read_bytes, size);
}
RecordInHistogram(stats_, BYTES_PER_READ, size);

ReturnAndCleanupSuperVersion(cfd, sv);
}
return s;
}
Expand Down
38 changes: 37 additions & 1 deletion db/db_merge_operand_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class DBMergeOperandTest : public DBTestBase {
: DBTestBase("db_merge_operand_test", /*env_do_fsync=*/true) {}
};

TEST_F(DBMergeOperandTest, MergeOperandReadAfterFreeBug) {
TEST_F(DBMergeOperandTest, CacheEvictedMergeOperandReadAfterFreeBug) {
// There was a bug of reading merge operands after they are mistakely freed
// in DB::GetMergeOperands, which is surfaced by cache full.
// See PR#9507 for more.
Expand Down Expand Up @@ -86,6 +86,42 @@ TEST_F(DBMergeOperandTest, MergeOperandReadAfterFreeBug) {
ASSERT_EQ(values[3].ToString(), "v4");
}

TEST_F(DBMergeOperandTest, FlushedMergeOperandReadAfterFreeBug) {
// Repro for a bug where a memtable containing a merge operand could be
// deleted before the merge operand was saved to the result.
auto options = CurrentOptions();
options.merge_operator = MergeOperators::CreateStringAppendOperator();
Reopen(options);

ASSERT_OK(Merge("key", "value"));

ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->LoadDependency(
{{"DBImpl::GetImpl:PostMemTableGet:0",
"DBMergeOperandTest::FlushedMergeOperandReadAfterFreeBug:PreFlush"},
{"DBMergeOperandTest::FlushedMergeOperandReadAfterFreeBug:PostFlush",
"DBImpl::GetImpl:PostMemTableGet:1"}});
ROCKSDB_NAMESPACE::SyncPoint::GetInstance()->EnableProcessing();

auto flush_thread = port::Thread([&]() {
TEST_SYNC_POINT(
"DBMergeOperandTest::FlushedMergeOperandReadAfterFreeBug:PreFlush");
ASSERT_OK(Flush());
TEST_SYNC_POINT(
"DBMergeOperandTest::FlushedMergeOperandReadAfterFreeBug:PostFlush");
});

PinnableSlice value;
GetMergeOperandsOptions merge_operands_info;
merge_operands_info.expected_max_number_of_operands = 1;
int number_of_operands;
ASSERT_OK(db_->GetMergeOperands(ReadOptions(), db_->DefaultColumnFamily(),
"key", &value, &merge_operands_info,
&number_of_operands));
ASSERT_EQ(1, number_of_operands);

flush_thread.join();
}

TEST_F(DBMergeOperandTest, GetMergeOperandsBasic) {
Options options;
options.create_if_missing = true;
Expand Down