This is an automated email from the ASF dual-hosted git repository.
twice pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new d6ea22a69 fix(bloom): crash due to pinnable slice in bloom filter
(#2843)
d6ea22a69 is described below
commit d6ea22a69fd86ddb0fedd297a550ce02433d4f83
Author: mwish <[email protected]>
AuthorDate: Tue Mar 25 11:43:13 2025 +0800
fix(bloom): crash due to pinnable slice in bloom filter (#2843)
---
src/types/redis_bloom_chain.cc | 25 +++++++++++++++------
tests/cppunit/types/bloom_chain_test.cc | 40 ++++++++++++++++++++++++++++-----
2 files changed, 53 insertions(+), 12 deletions(-)
diff --git a/src/types/redis_bloom_chain.cc b/src/types/redis_bloom_chain.cc
index 7dea8189b..cc2e45a23 100644
--- a/src/types/redis_bloom_chain.cc
+++ b/src/types/redis_bloom_chain.cc
@@ -195,6 +195,20 @@ rocksdb::Status BloomChain::InsertCommon(engine::Context
&ctx, const Slice &user
if (exist) {
(*rets)[i] = BloomFilterAddResult::kExist;
} else {
+ auto pinnable_slice_from_string = [](std::string data) ->
rocksdb::PinnableSlice {
+ // This is a workaround for the issue that PinnableSlice does not
support
+ // constructing from a temporary string.
+ rocksdb::PinnableSlice slice;
+ *slice.GetSelf() = std::move(data);
+ slice.PinSelf();
+ return slice;
+ };
+ auto strip_string_from_pinnable_slice = [](rocksdb::PinnableSlice
&slice) -> std::string {
+ if (slice.GetSelf() != nullptr) {
+ return std::move(*slice.GetSelf());
+ }
+ return slice.ToString();
+ };
if (metadata.size + 1 > metadata.GetCapacity()) {
if (metadata.IsScaling()) {
s = batch->Put(bf_key_list.back(),
bf_data_list.back().ToStringView());
@@ -202,20 +216,17 @@ rocksdb::Status BloomChain::InsertCommon(engine::Context
&ctx, const Slice &user
std::string bf_data;
s = createBloomFilterInBatch(ns_key, &metadata, batch, &bf_data);
if (!s.ok()) return s;
- rocksdb::PinnableSlice pin_slice;
- *pin_slice.GetSelf() = std::move(bf_data);
- pin_slice.PinSelf();
- bf_data_list.push_back(std::move(pin_slice));
+
bf_data_list.push_back(pinnable_slice_from_string(std::move(bf_data)));
bf_key_list.push_back(getBFKey(ns_key, metadata, metadata.n_filters
- 1));
} else {
(*rets)[i] = BloomFilterAddResult::kFull;
continue;
}
}
- std::string data = bf_data_list.back().ToString();
+ auto &bf_data = bf_data_list.back();
+ std::string data = strip_string_from_pinnable_slice(bf_data);
bloomAdd(item_hash_list[i], data);
- *bf_data_list.back().GetSelf() = std::move(data);
- bf_data_list.back().PinSelf();
+ bf_data = pinnable_slice_from_string(std::move(data));
(*rets)[i] = BloomFilterAddResult::kOk;
metadata.size += 1;
}
diff --git a/tests/cppunit/types/bloom_chain_test.cc
b/tests/cppunit/types/bloom_chain_test.cc
index bf8048014..cace78d45 100644
--- a/tests/cppunit/types/bloom_chain_test.cc
+++ b/tests/cppunit/types/bloom_chain_test.cc
@@ -27,10 +27,13 @@
class RedisBloomChainTest : public TestBase {
protected:
- explicit RedisBloomChainTest() { sb_chain_ =
std::make_unique<redis::BloomChain>(storage_.get(), "sb_chain_ns"); }
+ explicit RedisBloomChainTest() = default;
~RedisBloomChainTest() override = default;
- void SetUp() override { key_ = "test_sb_chain_key"; }
+ void SetUp() override {
+ key_ = "test_sb_chain_key";
+ sb_chain_ = std::make_unique<redis::BloomChain>(storage_.get(),
"sb_chain_ns");
+ }
void TearDown() override {}
std::unique_ptr<redis::BloomChain> sb_chain_;
@@ -48,8 +51,6 @@ TEST_F(RedisBloomChainTest, Reserve) {
s = sb_chain_->Reserve(*ctx_, key_, capacity, error_rate, expansion);
EXPECT_FALSE(s.ok());
EXPECT_EQ(s.ToString(), "Invalid argument: the key already exists");
-
- s = sb_chain_->Del(*ctx_, key_);
}
TEST_F(RedisBloomChainTest, BasicAddAndTest) {
@@ -79,5 +80,34 @@ TEST_F(RedisBloomChainTest, BasicAddAndTest) {
EXPECT_TRUE(s.ok());
EXPECT_EQ(exist, false);
}
- s = sb_chain_->Del(*ctx_, key_);
+}
+
+TEST_F(RedisBloomChainTest, DuplicateInsert) {
+ std::string insert_items[] = {"item1", "item2", "item3", "item101",
"item202", "303"};
+ for (const auto& insert_item : insert_items) {
+ redis::BloomFilterAddResult ret = redis::BloomFilterAddResult::kOk;
+ auto s = sb_chain_->Add(*ctx_, key_, insert_item, &ret);
+ EXPECT_TRUE(s.ok());
+ EXPECT_EQ(ret, redis::BloomFilterAddResult::kOk);
+ }
+ std::vector<std::string> arrays;
+ for (int64_t idx = 0; idx < 1000; ++idx) {
+ arrays.push_back("itemx" + std::to_string(idx));
+ }
+ std::vector<redis::BloomFilterAddResult> results;
+ results.resize(arrays.size());
+ auto s = sb_chain_->MAdd(*ctx_, key_, arrays, &results);
+ EXPECT_TRUE(s.ok());
+ arrays.clear();
+ for (int64_t idx = 1000; idx < 2000; ++idx) {
+ arrays.push_back("itemx" + std::to_string(idx));
+ }
+ s = sb_chain_->MAdd(*ctx_, key_, arrays, &results);
+ EXPECT_TRUE(s.ok());
+ arrays.clear();
+ for (int64_t idx = 2000; idx < 3000; ++idx) {
+ arrays.push_back("itemx" + std::to_string(idx));
+ }
+ s = sb_chain_->MAdd(*ctx_, key_, arrays, &results);
+ EXPECT_TRUE(s.ok());
}
