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.

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

encoding/binary: Uvarint and ReadUvarint are no longer consistent #54146

Closed
dsnet opened this issue Jul 30, 2022 · 2 comments
Closed

encoding/binary: Uvarint and ReadUvarint are no longer consistent #54146

dsnet opened this issue Jul 30, 2022 · 2 comments
Labels
FrozenDueToAge

Comments

@dsnet
Copy link
Member

dsnet commented Jul 30, 2022

Ever since ReadUvarint was modified in #41185 to limit reading to only 10 bytes, this made it such that Uvarint and ReadUvarint disagree about the definition of a varint. We should put the 10-byte limit in Uvarint as well and treat it as an overflow condition. This can occur for denormalized varints where a small integer uses more bytes to encode than necessary.

For comparison, protowire.ConsumeVarint holds to the stricter definition of a varint where it only handles up to 10 bytes and then treats any additional bytes as an overflow condition.
@ulikunitz
Copy link
Contributor

Uvarint in 1.18.5 handles MaxVarintLen64=10. Do you have an example showing different behavior of ReadUvarint and Uvarint?

@dsnet
Copy link
Member Author

dsnet commented Aug 1, 2022

Ah my mistake. I must have been reading an older version of the code. I see the check there.

@dsnet dsnet closed this as completed Aug 1, 2022
@golang golang locked and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dsnet @ulikunitz @gopherbot