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

parsing doesn't consider negative numbers #42

Open
ssoroka opened this issue Feb 7, 2022 · 1 comment
Open

parsing doesn't consider negative numbers #42

ssoroka opened this issue Feb 7, 2022 · 1 comment

Comments

@ssoroka
Copy link

ssoroka commented Feb 7, 2022

using ParseBase58 as an example, values should round-trip properly back to their original value, or error.

ok := "npL6MjP8Qfc"   //0x7fffffffffffffff
bad1 := "npL6MjP8Qfd" //0x7fffffffffffffff + 1 // overflows int64 and becomes negative
bad2 := "JPwcyDCgEuq" //0xffffffffffffffff + 1 // overflows uint64 and wraps back to 1
  • in the case of bad1 it parses "successfully" and returns a negative value, which later panics when Base58() cannot convert it back to a string (it doesn't handle negative cases). It should error on parsing, or negatives should be supported throughout (or the base type should be uint64).
  • in the case of bad2 it parses "successfully" and returns a value of 1, which base58() does not encode back to the original number, obviously. it should probably error because the original number is not valid.

These issues can come up when accepting requests with bad IDs from clients, and they should not be accepted as valid ids.

@ssoroka ssoroka changed the title parsing fails for negative numbers parsing doesn't consider negative numbers Feb 7, 2022
@bwmarrin
Copy link
Owner

bwmarrin commented Oct 3, 2022

Thanks for finding and reporting this :)

I think a good safe initial fix would be to check for this and error on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants