-
Notifications
You must be signed in to change notification settings - Fork 75
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
Forbid negative uvarint values #226
Conversation
When reading over code, I noticed that it was possible to provide a 10-byte uvarint (variable length integer) instead of the 9-byte maximum. The functions did not properly catch it. Also, fix what I believe is a typo in the ByteArray version. It checked the wrong variable (result vs index) which would almost always result in a null value. Might as well throw an exception there like the other implementation. Fixes #225.
Sorry for the delay on this one - I haven't forgotten it, just not an area I'm particularly familiar with so harder to review. |
No worries, I totally understand. There's no rush. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, dug into the specs and this looks right to me - sorry it took so long. I've just added an additional test to confirm that we can successfully encode and decode Long.MAX_VALUE
to ensure that the maximum length of 9 bytes is decoded.
@ajsutton Just so you know, jvm-libp2p/src/test/kotlin/io/libp2p/etc/types/UvarintTest.kt Lines 28 to 33 in 5c3943d
jvm-libp2p/src/test/kotlin/io/libp2p/etc/types/UvarintTest.kt Lines 61 to 66 in 5c3943d
|
Oh darn I didn't spot that. I'll have to clean that up. Thanks. |
When reading over code, I noticed that it was possible to provide a 10-byte
uvarint (variable length integer) instead of the 9-byte maximum. The functions
did not properly catch it.
Also, fix what I believe is a typo in the ByteArray version. It checked the
wrong variable (result vs index) which would almost always result in a null
value. Might as well throw an exception there like the other implementation.
Fixes #225.