Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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.

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

I32 varints should be (de)serialized as 10 bytes of all-1s, not 5. #81

Closed
hans-tvs opened this issue Nov 8, 2024 · 2 comments
Closed

Comments

@hans-tvs
Copy link
Contributor

hans-tvs commented Nov 8, 2024

I submitted a PR recently to convert the problem from a panic to a runtime error, but I was under the mistaken impression that the protobuf data we were being given was occasionally broken. Instead, the Python and C++ canonical implementations, along with the docs, indicate that the appropriate order of operations is something like the following:

// potential_varint: []const u8 -- valid if at most 10 bytes have 1 as the MSB
const decoded_x: u64 = try decode_varint(potential_varint);
const interpreted_x: i32 = @intCast(@as(i64, @bitCast(decoded_x)));

As opposed to the current logic, which either truncates the extra 4 bytes of decoded data, or fails when any of those extra 4 bytes are 0.

This is likely a (small) problem throughout the codebase, not isolated to enums or any one feature.

@hans-tvs hans-tvs closed this as completed Nov 8, 2024
@hans-tvs
Copy link
Contributor Author

hans-tvs commented Nov 8, 2024

Sorry about the issue noise; I misread several things when posting this.

@menduz
Copy link
Collaborator

menduz commented Nov 8, 2024

false alarm?

Repository owner locked and limited conversation to collaborators Nov 8, 2024
@Arwalk Arwalk converted this issue into discussion #82 Nov 8, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants