From 2c3959b0660c39519c86cedde79c3bfe49bfcfac Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 10:58:01 +0300 Subject: [PATCH 1/6] fix: bitops_family crash fixed --- src/server/bitops_family.cc | 6 +++++- src/server/bitops_family_test.cc | 13 +++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index c550d8cdd495..02e743296dfb 100644 --- a/src/server/bitops_family.cc +++ b/src/server/bitops_family.cc @@ -138,7 +138,11 @@ constexpr int32_t GetByteIndex(uint32_t offset) noexcept { } uint8_t GetByteValue(string_view str, uint32_t offset) { - return static_cast(str[GetByteIndex(offset)]); + int32_t byte_index = GetByteIndex(offset); + if (byte_index >= static_cast(str.size())) { + return 0; + } + return static_cast(str[byte_index]); } constexpr bool CheckBitStatus(uint8_t byte, uint32_t offset) { diff --git a/src/server/bitops_family_test.cc b/src/server/bitops_family_test.cc index 17bcca6a0161..ddc1f9ddb85a 100644 --- a/src/server/bitops_family_test.cc +++ b/src/server/bitops_family_test.cc @@ -805,4 +805,17 @@ TEST_F(BitOpsFamilyTest, BitFieldOperations) { ASSERT_THAT(Run({"bitfield", "foo", "get", "u1", "15"}), IntArg(1)); } +TEST_F(BitOpsFamilyTest, BitFieldLargeOffset) { + Run({"set", "foo", "bar"}); + + Run({"bitfield", "foo", "get", "u32", "0", "overflow", "fail", "incrby", "u32", "0", + "4294967295"}); + + auto resp = Run({"bitfield", "foo", "get", "u32", "4294967295"}); + EXPECT_EQ(resp, 825276672); + + resp = Run({"bitfield", "foo", "get", "u32", "8589934590"}); + EXPECT_EQ(resp, 412638336); +} + } // end of namespace dfly From 83a69eb456ea56bc56c13d70ca6e8211ee817917 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 12:08:10 +0300 Subject: [PATCH 2/6] fix: cleanup. approach has been changed --- src/server/bitops_family.cc | 18 ++++++++++++------ src/server/bitops_family_test.cc | 4 ++-- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index 02e743296dfb..cb40917b4383 100644 --- a/src/server/bitops_family.cc +++ b/src/server/bitops_family.cc @@ -138,11 +138,7 @@ constexpr int32_t GetByteIndex(uint32_t offset) noexcept { } uint8_t GetByteValue(string_view str, uint32_t offset) { - int32_t byte_index = GetByteIndex(offset); - if (byte_index >= static_cast(str.size())) { - return 0; - } - return static_cast(str[byte_index]); + return static_cast(str[GetByteIndex(offset)]); } constexpr bool CheckBitStatus(uint8_t byte, uint32_t offset) { @@ -733,7 +729,12 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { auto last_byte_offset = GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1); uint32_t lsb = attr_.offset + attr_.encoding_bit_size - 1; - if (last_byte_offset > total_bytes) { + if (last_byte_offset >= total_bytes) { + return {}; + } + + int32_t byte_index = GetByteIndex(offset); + if (byte_index >= total_bytes) { return {}; } @@ -742,6 +743,11 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { int64_t result = 0; for (size_t i = 0; i < attr_.encoding_bit_size; ++i) { + int32_t lsb_byte_index = GetByteIndex(lsb); + if (lsb_byte_index >= total_bytes) { + return {}; + } + uint8_t byte{GetByteValue(bytes, lsb)}; int32_t index = GetNormalizedBitIndex(lsb); int64_t old_bit = CheckBitStatus(byte, index); diff --git a/src/server/bitops_family_test.cc b/src/server/bitops_family_test.cc index ddc1f9ddb85a..31f0c9505283 100644 --- a/src/server/bitops_family_test.cc +++ b/src/server/bitops_family_test.cc @@ -812,10 +812,10 @@ TEST_F(BitOpsFamilyTest, BitFieldLargeOffset) { "4294967295"}); auto resp = Run({"bitfield", "foo", "get", "u32", "4294967295"}); - EXPECT_EQ(resp, 825276672); + EXPECT_THAT(resp, ArgType(RespExpr::NIL)); resp = Run({"bitfield", "foo", "get", "u32", "8589934590"}); - EXPECT_EQ(resp, 412638336); + EXPECT_THAT(resp, ArgType(RespExpr::NIL)); } } // end of namespace dfly From 3d1adca89fe26f099f070e0d4469cdf9556cf529 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 12:39:33 +0300 Subject: [PATCH 3/6] fix: review comment has been fixed --- src/server/bitops_family.cc | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index cb40917b4383..046b617ea15e 100644 --- a/src/server/bitops_family.cc +++ b/src/server/bitops_family.cc @@ -726,15 +726,9 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { const auto& bytes = *bitfield; const int32_t total_bytes = static_cast(bytes.size()); const size_t offset = attr_.offset; - auto last_byte_offset = GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1); - uint32_t lsb = attr_.offset + attr_.encoding_bit_size - 1; - if (last_byte_offset >= total_bytes) { - return {}; - } - int32_t byte_index = GetByteIndex(offset); - if (byte_index >= total_bytes) { + if (GetByteIndex(lsb) >= total_bytes || GetByteIndex(offset) >= total_bytes) { return {}; } @@ -743,11 +737,6 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { int64_t result = 0; for (size_t i = 0; i < attr_.encoding_bit_size; ++i) { - int32_t lsb_byte_index = GetByteIndex(lsb); - if (lsb_byte_index >= total_bytes) { - return {}; - } - uint8_t byte{GetByteValue(bytes, lsb)}; int32_t index = GetNormalizedBitIndex(lsb); int64_t old_bit = CheckBitStatus(byte, index); From 2dc92ae8238331c4d7a0d65b8d5f727956a439c8 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 16:19:28 +0300 Subject: [PATCH 4/6] fix: revert changes --- src/server/bitops_family.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index 046b617ea15e..c550d8cdd495 100644 --- a/src/server/bitops_family.cc +++ b/src/server/bitops_family.cc @@ -726,9 +726,10 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { const auto& bytes = *bitfield; const int32_t total_bytes = static_cast(bytes.size()); const size_t offset = attr_.offset; - uint32_t lsb = attr_.offset + attr_.encoding_bit_size - 1; + auto last_byte_offset = GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1); - if (GetByteIndex(lsb) >= total_bytes || GetByteIndex(offset) >= total_bytes) { + uint32_t lsb = attr_.offset + attr_.encoding_bit_size - 1; + if (last_byte_offset > total_bytes) { return {}; } From bc2cc17f08df2486250df830eeb72f99979d6d4b Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Thu, 24 Apr 2025 17:26:23 +0300 Subject: [PATCH 5/6] fix: another fix --- src/server/bitops_family.cc | 21 +++++++++++++++------ src/server/bitops_family_test.cc | 16 ++++++++++------ 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index c550d8cdd495..7105f74596d2 100644 --- a/src/server/bitops_family.cc +++ b/src/server/bitops_family.cc @@ -728,9 +728,17 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { const size_t offset = attr_.offset; auto last_byte_offset = GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1); + if (GetByteIndex(offset) >= total_bytes && attr_.encoding_bit_size > 0) { + return 0; + } + + const string* result_str = bitfield; + string buff; uint32_t lsb = attr_.offset + attr_.encoding_bit_size - 1; - if (last_byte_offset > total_bytes) { - return {}; + if (last_byte_offset >= total_bytes) { + buff = *bitfield; + buff.resize(last_byte_offset + 1, 0); + result_str = &buff; } const bool is_negative = @@ -738,7 +746,7 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { int64_t result = 0; for (size_t i = 0; i < attr_.encoding_bit_size; ++i) { - uint8_t byte{GetByteValue(bytes, lsb)}; + uint8_t byte{GetByteValue(*result_str, lsb)}; int32_t index = GetNormalizedBitIndex(lsb); int64_t old_bit = CheckBitStatus(byte, index); result |= old_bit << i; @@ -830,10 +838,11 @@ ResultType IncrBy::ApplyTo(Overflow ov, string* bitfield) { string& bytes = *bitfield; Get get(attr_); auto res = get.ApplyTo(ov, &bytes); + const int32_t total_bytes = static_cast(bytes.size()); + auto last_byte_offset = GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1); - if (!res) { - Set set(attr_, incr_value_); - return set.ApplyTo(ov, &bytes); + if (last_byte_offset >= total_bytes) { + bytes.resize(last_byte_offset + 1, 0); } if (!HandleOverflow(ov, &*res)) { diff --git a/src/server/bitops_family_test.cc b/src/server/bitops_family_test.cc index 31f0c9505283..82ec8b209709 100644 --- a/src/server/bitops_family_test.cc +++ b/src/server/bitops_family_test.cc @@ -808,14 +808,18 @@ TEST_F(BitOpsFamilyTest, BitFieldOperations) { TEST_F(BitOpsFamilyTest, BitFieldLargeOffset) { Run({"set", "foo", "bar"}); - Run({"bitfield", "foo", "get", "u32", "0", "overflow", "fail", "incrby", "u32", "0", - "4294967295"}); + auto resp = Run({"bitfield", "foo", "get", "u32", "0", "overflow", "fail", "incrby", "u32", "0", + "4294967295"}); + EXPECT_THAT(resp, RespArray(ElementsAre(IntArg(1650553344), ArgType(RespExpr::NIL)))); - auto resp = Run({"bitfield", "foo", "get", "u32", "4294967295"}); - EXPECT_THAT(resp, ArgType(RespExpr::NIL)); + resp = Run({"strlen", "foo"}); + EXPECT_THAT(resp, 4); - resp = Run({"bitfield", "foo", "get", "u32", "8589934590"}); - EXPECT_THAT(resp, ArgType(RespExpr::NIL)); + resp = Run({"get", "foo"}); + EXPECT_THAT(ToSV(resp.GetBuf()), Eq(std::string_view("bar\0", 4))); + + resp = Run({"bitfield", "foo", "get", "u32", "4294967295"}); + EXPECT_THAT(resp, 0); } } // end of namespace dfly From 5431198fe4b610c9ccd03b65ff6020ec370712e0 Mon Sep 17 00:00:00 2001 From: Volodymyr Yavdoshenko Date: Sun, 27 Apr 2025 13:51:48 +0300 Subject: [PATCH 6/6] fix: review comment was fixed --- src/server/bitops_family.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/server/bitops_family.cc b/src/server/bitops_family.cc index 7105f74596d2..9a58e82b8c7c 100644 --- a/src/server/bitops_family.cc +++ b/src/server/bitops_family.cc @@ -728,7 +728,7 @@ ResultType Get::ApplyTo(Overflow ov, const string* bitfield) { const size_t offset = attr_.offset; auto last_byte_offset = GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1); - if (GetByteIndex(offset) >= total_bytes && attr_.encoding_bit_size > 0) { + if (GetByteIndex(offset) >= total_bytes) { return 0; }