From cca141ecf8634a42b5eb548cb0ac3a6b77d783c1 Mon Sep 17 00:00:00 2001 From: yiwu-arbug Date: Fri, 12 Apr 2019 17:03:08 -0700 Subject: [PATCH] Fix crash with memtable prefix bloom and key out of prefix extractor domain (#5190) Summary: Before using prefix extractor `InDomain()` should be check. All uses in memtable.cc didn't check `InDomain()`. Pull Request resolved: https://github.com/facebook/rocksdb/pull/5190 Differential Revision: D14923773 Pulled By: miasantreble fbshipit-source-id: b3ad60bcca5f3a1a2b929a6eb34b0b7ba6326f04 --- HISTORY.md | 2 ++ db/db_bloom_filter_test.cc | 19 +++++++++++++++++++ db/memtable.cc | 17 +++++++++++------ 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index c23e6b4b0..4278504f7 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -8,10 +8,12 @@ ### Public API Change * Change the behavior of OptimizeForPointLookup(): move away from hash-based block-based-table index, and use whole key memtable filtering. * Change the behavior of OptimizeForSmallDb(): use a 16MB block cache, put index and filter blocks into it, and cost the memtable size to it. DBOptions.OptimizeForSmallDb() and ColumnFamilyOptions.OptimizeForSmallDb() start to take an optional cache object. + ### Bug Fixes * Fix a bug in 2PC where a sequence of txn prepare, memtable flush, and crash could result in losing the prepared transaction. * Fix a bug in Encryption Env which could cause encrypted files to be read beyond file boundaries. * Fix a race condition between WritePrepared::Get and ::Put with duplicate keys. +* Fix crash when memtable prefix bloom is enabled and read/write a key out of domain of prefix extractor. ## 6.1.0 (3/27/2019) ### New Features diff --git a/db/db_bloom_filter_test.cc b/db/db_bloom_filter_test.cc index 39dd20bb2..de612d04b 100644 --- a/db/db_bloom_filter_test.cc +++ b/db/db_bloom_filter_test.cc @@ -836,6 +836,25 @@ TEST_F(DBBloomFilterTest, MemtableWholeKeyBloomFilter) { ASSERT_EQ(1, get_perf_context()->bloom_memtable_hit_count); } +TEST_F(DBBloomFilterTest, MemtablePrefixBloomOutOfDomain) { + constexpr size_t kPrefixSize = 8; + const std::string kKey = "key"; + assert(kKey.size() < kPrefixSize); + Options options = CurrentOptions(); + options.prefix_extractor.reset(NewFixedPrefixTransform(kPrefixSize)); + options.memtable_prefix_bloom_size_ratio = 0.25; + Reopen(options); + ASSERT_OK(Put(kKey, "v")); + ASSERT_EQ("v", Get(kKey)); + std::unique_ptr iter(dbfull()->NewIterator(ReadOptions())); + iter->Seek(kKey); + ASSERT_TRUE(iter->Valid()); + ASSERT_EQ(kKey, iter->key()); + iter->SeekForPrev(kKey); + ASSERT_TRUE(iter->Valid()); + ASSERT_EQ(kKey, iter->key()); +} + #ifndef ROCKSDB_LITE class BloomStatsTestWithParam : public DBBloomFilterTest, diff --git a/db/memtable.cc b/db/memtable.cc index 16b5c8ee0..1ffd83cac 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -318,8 +318,9 @@ class MemTableIterator : public InternalIterator { PERF_COUNTER_ADD(seek_on_memtable_count, 1); if (bloom_) { // iterator should only use prefix bloom filter - if (!bloom_->MayContain( - prefix_extractor_->Transform(ExtractUserKey(k)))) { + Slice user_key(ExtractUserKey(k)); + if (prefix_extractor_->InDomain(user_key) && + !bloom_->MayContain(prefix_extractor_->Transform(user_key))) { PERF_COUNTER_ADD(bloom_memtable_miss_count, 1); valid_ = false; return; @@ -334,8 +335,9 @@ class MemTableIterator : public InternalIterator { PERF_TIMER_GUARD(seek_on_memtable_time); PERF_COUNTER_ADD(seek_on_memtable_count, 1); if (bloom_) { - if (!bloom_->MayContain( - prefix_extractor_->Transform(ExtractUserKey(k)))) { + Slice user_key(ExtractUserKey(k)); + if (prefix_extractor_->InDomain(user_key) && + !bloom_->MayContain(prefix_extractor_->Transform(user_key))) { PERF_COUNTER_ADD(bloom_memtable_miss_count, 1); valid_ = false; return; @@ -518,7 +520,8 @@ bool MemTable::Add(SequenceNumber s, ValueType type, std::memory_order_relaxed); } - if (bloom_filter_ && prefix_extractor_) { + if (bloom_filter_ && prefix_extractor_ && + prefix_extractor_->InDomain(key)) { bloom_filter_->Add(prefix_extractor_->Transform(key)); } if (bloom_filter_ && moptions_.memtable_whole_key_filtering) { @@ -551,7 +554,8 @@ bool MemTable::Add(SequenceNumber s, ValueType type, post_process_info->num_deletes++; } - if (bloom_filter_ && prefix_extractor_) { + if (bloom_filter_ && prefix_extractor_ && + prefix_extractor_->InDomain(key)) { bloom_filter_->AddConcurrently(prefix_extractor_->Transform(key)); } if (bloom_filter_ && moptions_.memtable_whole_key_filtering) { @@ -771,6 +775,7 @@ bool MemTable::Get(const LookupKey& key, std::string* value, Status* s, } else { assert(prefix_extractor_); may_contain = + !prefix_extractor_->InDomain(user_key) || bloom_filter_->MayContain(prefix_extractor_->Transform(user_key)); } }