Skip to content

fix: bitops_family crash fixed #4989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 28, 2025
Merged

fix: bitops_family crash fixed #4989

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c3959b
fix: bitops_family crash fixed
vyavdoshenko Apr 24, 2025
83a69eb
fix: cleanup. approach has been changed
vyavdoshenko Apr 24, 2025
3d1adca
fix: review comment has been fixed
vyavdoshenko Apr 24, 2025
2dc92ae
fix: revert changes
vyavdoshenko Apr 24, 2025
bc2cc17
fix: another fix
vyavdoshenko Apr 24, 2025
0e45cd0
fix: merge branch 'main' into bobik/bitops_crash_fix
vyavdoshenko Apr 27, 2025
5431198
fix: review comment was fixed
vyavdoshenko Apr 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/server/bitops_family.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (last_byte_offset >= total_bytes) {
return {};
}
int32_t byte_index = GetByteIndex(offset);
if (byte_index >= total_bytes) {
if (last_byte_offset >= total_bytes) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sufficient. We check the expression above GetByteIndex(attr_.offset + attr_.encoding_bit_size - 1);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kostasrim
This is not enough.
I updated the condition, please take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kostasrim
Please, take a look again.

return {};
}

Expand All @@ -738,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 {};
}
Copy link
Contributor

@kostasrim kostasrim Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to check again here. The above check is sufficient -- we can remove this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


uint8_t byte{GetByteValue(bytes, lsb)};
int32_t index = GetNormalizedBitIndex(lsb);
int64_t old_bit = CheckBitStatus(byte, index);
Expand Down
13 changes: 13 additions & 0 deletions src/server/bitops_family_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_THAT(resp, ArgType(RespExpr::NIL));

resp = Run({"bitfield", "foo", "get", "u32", "8589934590"});
EXPECT_THAT(resp, ArgType(RespExpr::NIL));
}

} // end of namespace dfly
Loading