diff --git a/table/block_based/block.cc b/table/block_based/block.cc index e3e5c60d1..13e3397a1 100644 --- a/table/block_based/block.cc +++ b/table/block_based/block.cc @@ -1062,10 +1062,8 @@ Block::Block(BlockContents&& contents, size_t read_amp_bytes_per_bit, uint16_t map_offset; data_block_hash_index_.Initialize( - contents.data.data(), - static_cast(contents.data.size() - - sizeof(uint32_t)), /*chop off - NUM_RESTARTS*/ + data_, static_cast(size_ - sizeof(uint32_t)), /*chop off + NUM_RESTARTS*/ &map_offset); restart_offset_ = map_offset - num_restarts_ * sizeof(uint32_t); diff --git a/table/block_based/block_builder.cc b/table/block_based/block_builder.cc index 62bf2eec1..239402dc3 100644 --- a/table/block_based/block_builder.cc +++ b/table/block_based/block_builder.cc @@ -240,6 +240,10 @@ inline void BlockBuilder::AddWithLastKeyImpl(const Slice& key, // TODO(yuzhangyu): make user defined timestamp work with block hash index. if (data_block_hash_index_builder_.Valid()) { + // Only data blocks should be using `kDataBlockBinaryAndHash` index type. + // And data blocks should always be built with internal keys instead of + // user keys. + assert(!is_user_key_); data_block_hash_index_builder_.Add(ExtractUserKey(key), restarts_.size() - 1); } diff --git a/table/block_based/block_test.cc b/table/block_based/block_test.cc index 31dd151ac..3264371c1 100644 --- a/table/block_based/block_test.cc +++ b/table/block_based/block_test.cc @@ -76,9 +76,13 @@ void GenerateRandomKVs(std::vector *keys, } } +// Test Param 1): key use delta encoding. +// Test Param 2): user-defined timestamp test mode. +// Test Param 3): data block index type. class BlockTest : public testing::Test, public testing::WithParamInterface< - std::tuple> { + std::tuple> { public: bool keyUseDeltaEncoding() const { return std::get<0>(GetParam()); } bool isUDTEnabled() const { @@ -87,6 +91,10 @@ class BlockTest : public testing::Test, bool shouldPersistUDT() const { return test::ShouldPersistUDT(std::get<1>(GetParam())); } + + BlockBasedTableOptions::DataBlockIndexType dataBlockIndexType() const { + return std::get<2>(GetParam()); + } }; // block test @@ -100,9 +108,11 @@ TEST_P(BlockTest, SimpleTest) { std::vector keys; std::vector values; + BlockBasedTableOptions::DataBlockIndexType index_type = + isUDTEnabled() ? BlockBasedTableOptions::kDataBlockBinarySearch + : dataBlockIndexType(); BlockBuilder builder(16, keyUseDeltaEncoding(), - false /* use_value_delta_encoding */, - BlockBasedTableOptions::kDataBlockBinarySearch, + false /* use_value_delta_encoding */, index_type, 0.75 /* data_block_hash_table_util_ratio */, ts_sz, shouldPersistUDT(), false /* is_user_key */); int num_records = 100000; @@ -159,16 +169,16 @@ TEST_P(BlockTest, SimpleTest) { } // return the block contents -BlockContents GetBlockContents(std::unique_ptr *builder, - const std::vector &keys, - const std::vector &values, - bool key_use_delta_encoding, size_t ts_sz, - bool should_persist_udt, - const int /*prefix_group_size*/ = 1) { +BlockContents GetBlockContents( + std::unique_ptr *builder, + const std::vector &keys, + const std::vector &values, bool key_use_delta_encoding, + size_t ts_sz, bool should_persist_udt, const int /*prefix_group_size*/ = 1, + BlockBasedTableOptions::DataBlockIndexType dblock_index_type = + BlockBasedTableOptions::DataBlockIndexType::kDataBlockBinarySearch) { builder->reset( new BlockBuilder(1 /* restart interval */, key_use_delta_encoding, - false /* use_value_delta_encoding */, - BlockBasedTableOptions::kDataBlockBinarySearch, + false /* use_value_delta_encoding */, dblock_index_type, 0.75 /* data_block_hash_table_util_ratio */, ts_sz, should_persist_udt, false /* is_user_key */)); @@ -238,8 +248,13 @@ TEST_P(BlockTest, SimpleIndexHash) { 1 /* keys_share_prefix */, ts_sz); std::unique_ptr builder; + auto contents = GetBlockContents( - &builder, keys, values, keyUseDeltaEncoding(), ts_sz, shouldPersistUDT()); + &builder, keys, values, keyUseDeltaEncoding(), ts_sz, shouldPersistUDT(), + 1 /* prefix_group_size */, + isUDTEnabled() + ? BlockBasedTableOptions::DataBlockIndexType::kDataBlockBinarySearch + : dataBlockIndexType()); CheckBlockContents(std::move(contents), kMaxKey, keys, values, isUDTEnabled(), shouldPersistUDT()); @@ -260,9 +275,13 @@ TEST_P(BlockTest, IndexHashWithSharedPrefix) { kPrefixGroup /* keys_share_prefix */, ts_sz); std::unique_ptr builder; - auto contents = - GetBlockContents(&builder, keys, values, keyUseDeltaEncoding(), - isUDTEnabled(), shouldPersistUDT(), kPrefixGroup); + + auto contents = GetBlockContents( + &builder, keys, values, keyUseDeltaEncoding(), isUDTEnabled(), + shouldPersistUDT(), kPrefixGroup, + isUDTEnabled() + ? BlockBasedTableOptions::DataBlockIndexType::kDataBlockBinarySearch + : dataBlockIndexType()); CheckBlockContents(std::move(contents), kMaxKey, keys, values, isUDTEnabled(), shouldPersistUDT()); @@ -270,10 +289,18 @@ TEST_P(BlockTest, IndexHashWithSharedPrefix) { // Param 0: key use delta encoding // Param 1: user-defined timestamp test mode +// Param 2: data block index type. User-defined timestamp feature is not +// compatible with `kDataBlockBinaryAndHash` data block index type because the +// user comparator doesn't provide a `CanKeysWithDifferentByteContentsBeEqual` +// override. This combination is disabled. INSTANTIATE_TEST_CASE_P( P, BlockTest, - ::testing::Combine(::testing::Bool(), - ::testing::ValuesIn(test::GetUDTTestModes()))); + ::testing::Combine( + ::testing::Bool(), ::testing::ValuesIn(test::GetUDTTestModes()), + ::testing::Values( + BlockBasedTableOptions::DataBlockIndexType::kDataBlockBinarySearch, + BlockBasedTableOptions::DataBlockIndexType:: + kDataBlockBinaryAndHash))); // A slow and accurate version of BlockReadAmpBitmap that simply store // all the marked ranges in a set. diff --git a/unreleased_history/bug_fixes/use_after_move_in_block.md b/unreleased_history/bug_fixes/use_after_move_in_block.md new file mode 100644 index 000000000..019ef1627 --- /dev/null +++ b/unreleased_history/bug_fixes/use_after_move_in_block.md @@ -0,0 +1 @@ +*Fix a use-after-move bug in block.cc. \ No newline at end of file