Skip to content
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

Jump to bottom

fix(rdb): incorrect type conversion during RDB loads the int8 encoding #2547

Merged
merged 4 commits into from
Sep 21, 2024
Merged

fix(rdb): incorrect type conversion during RDB loads the int8 encoding #2547

merged 4 commits into from
Sep 21, 2024

Conversation

fstd2
Copy link
Contributor

@fstd2 fstd2 commented Sep 20, 2024

Resolve #2546

@fstd2 fstd2 mentioned this pull request Sep 20, 2024
Closed
2 tasks
torwig
torwig reviewed Sep 20, 2024
View reviewed changes
src/storage/rdb.cc
@@ -157,7 +157,7 @@ StatusOr<std::string> RDB::loadEncodedString() {
unsigned char buf[4] = {0};
if (len == RDBEncInt8) {
auto next = GET_OR_RET(stream_->ReadByte());
return std::to_string(static_cast<int>(next));
return std::to_string(static_cast<int8_t>(next));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why int if the length is 8?

Copy link
Contributor

@torwig torwig Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Did you try uint8_t? However, I'm not sure that it's the right type.

Copy link
Member

Choose a reason for hiding this comment

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

It should be correct to use int8_t here since the encoding is int8. Cast it to int will drop the sign bit.

@git-hulk
Copy link
Member

@fstd2 It would be great if you could add a Go test case to prevent breaking in the future.

@fstd2
Copy link
Contributor Author

fstd2 commented Sep 20, 2024

@fstd2 It would be great if you could add a Go test case to prevent breaking in the future.

I'm a beginner, don't know how to do..

@git-hulk
Copy link
Member

@fstd2 No worry, I will do this later if you don't know how to. Thank you!

@git-hulk git-hulk changed the title fix(rdb): handle RDBEncInt8 type conversion correctly during RDB loadEncodedString fix(rdb): incorrect type conversion during RDB loads the int8 encoding Sep 21, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Sep 21, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
45.9% Coverage on New Code (required ≥ 50%)

See analysis details on SonarCloud

@git-hulk git-hulk merged commit 8c42aa0 into apache:unstable Sep 21, 2024
32 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@torwig torwig torwig left review comments

@git-hulk git-hulk git-hulk approved these changes

@PragmaTwice PragmaTwice PragmaTwice approved these changes

Assignees
No one assigned
Labels
bug type bug release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inconsistent behavior when using RESTORE with dumped negative integers
4 participants
@fstd2 @git-hulk @torwig @PragmaTwice