Fix use after move in data block hash index (#11505)

Summary:
Fix a use-after-move issue in block.cc and added some unit tests.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11505

Test Plan:
```
make all check
./block_test
```

Reviewed By: ltamasi

Differential Revision: D46506188

Pulled By: jowlyzhang

fbshipit-source-id: 316ed8ddd221c00b2bce2cf9fd47eea686cd74a5

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(),
+            data_, static_cast<uint16_t>(size_ - sizeof(uint32_t)), /*chop off
-            static_cast<uint16_t>(contents.data.size() -
+                                                                NUM_RESTARTS*/
-                                  sizeof(uint32_t)), /*chop off
-                                                 NUM_RESTARTS*/
             &map_offset);
 
         restart_offset_ = map_offset - num_restarts_ * sizeof(uint32_t);

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);
   }

table/block_based/block_test.cc

@@ -76,9 +76,13 @@ void GenerateRandomKVs(std::vector<std::string> *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<bool, test::UserDefinedTimestampTestMode>> {
+                      std::tuple<bool, test::UserDefinedTimestampTestMode,
+                                 BlockBasedTableOptions::DataBlockIndexType>> {
  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<std::string> keys;
   std::vector<std::string> values;
+  BlockBasedTableOptions::DataBlockIndexType index_type =
+      isUDTEnabled() ? BlockBasedTableOptions::kDataBlockBinarySearch
+                     : dataBlockIndexType();
   BlockBuilder builder(16, keyUseDeltaEncoding(),
-                       false /* use_value_delta_encoding */,
+                       false /* use_value_delta_encoding */, index_type,
-                       BlockBasedTableOptions::kDataBlockBinarySearch,
                        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<BlockBuilder> *builder,
+BlockContents GetBlockContents(
-                               const std::vector<std::string> &keys,
+    std::unique_ptr<BlockBuilder> *builder,
-                               const std::vector<std::string> &values,
+    const std::vector<std::string> &keys,
-                               bool key_use_delta_encoding, size_t ts_sz,
+    const std::vector<std::string> &values, bool key_use_delta_encoding,
-                               bool should_persist_udt,
+    size_t ts_sz, bool should_persist_udt, const int /*prefix_group_size*/ = 1,
-                               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 */,
+                       false /* use_value_delta_encoding */, dblock_index_type,
-                       BlockBasedTableOptions::kDataBlockBinarySearch,
                        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<BlockBuilder> 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<BlockBuilder> builder;
-  auto contents =
+
-      GetBlockContents(&builder, keys, values, keyUseDeltaEncoding(),
+  auto contents = GetBlockContents(
-                       isUDTEnabled(), shouldPersistUDT(), kPrefixGroup);
+      &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::Combine(
-                       ::testing::ValuesIn(test::GetUDTTestModes())));
+        ::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.

unreleased_history/bug_fixes/use_after_move_in_block.md

@@ -0,0 +1 @@
+*Fix a use-after-move bug in block.cc.