-
Notifications
You must be signed in to change notification settings - Fork 618
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
Prohibited using of zero and negative filed number in ProtoNumber and zero field numbers in input bytes #2766
Conversation
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Streams.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt
Outdated
Show resolved
Hide resolved
@@ -45,8 +45,12 @@ internal open class ProtobufDecoder( | |||
* If we have reasonably small count of elements, try to build sequential | |||
* array for the fast-path. Fast-path implies that elements are not marked with @ProtoId | |||
* explicitly or are monotonic and incremental (maybe, 1-indexed) | |||
* | |||
* Since the library allows the use of fields with proto ID 0, |
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.
Is this still true? IIRC this function is not used for enums. Although it won't hurt to initialize everything with -1 just to be sure
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.
Yes, this line isn't actual now, in this version I forbade the use of field number 0.
@@ -45,8 +45,12 @@ internal open class ProtobufDecoder( | |||
* If we have reasonably small count of elements, try to build sequential | |||
* array for the fast-path. Fast-path implies that elements are not marked with @ProtoId | |||
* explicitly or are monotonic and incremental (maybe, 1-indexed) | |||
* | |||
* Since the library allows the use of fields with proto ID 0, | |||
* it is necessary to initialize all elements, because there will always be one extra element |
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.
I don't get where 'one extra element' is coming from. What if there are exactly 32 properties in the class? What if there is like 5, there are much more extra elements than one?
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.
if you look at the creation of the cache, there will be IntArray(elements + 1)
.
This + 1
- is the extra element
.
This is because arrays are numbered from 0, and in protobuf fields start from 1, so in order to address properties from 1 to 32, we need 33 elements (element 0 should not have been used at all)
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/ProtobufDecoding.kt
Outdated
Show resolved
Hide resolved
formats/protobuf/commonMain/src/kotlinx/serialization/protobuf/internal/Helpers.kt
Outdated
Show resolved
Hide resolved
|
||
@Test | ||
fun testName() { | ||
println(X::class.qualifiedName) |
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.
Unrelated?
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.
Deleted
@@ -45,8 +45,12 @@ internal open class ProtobufDecoder( | |||
* If we have reasonably small count of elements, try to build sequential | |||
* array for the fast-path. Fast-path implies that elements are not marked with @ProtoId | |||
* explicitly or are monotonic and incremental (maybe, 1-indexed) | |||
* | |||
* Initialize all elements, because there will always be one extra element as arrays are numbered from 0 | |||
* but in protobuf field number starts from 1. |
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.
Either next line should be a complete sentence (it doesn't look like one now), or the dot shouldn't be here
…/internal/Helpers.kt Co-authored-by: Leonid Startsev <sandwwraith@users.noreply.github.com>
Also optimized skipping of size delimited fields - removed the creation of an byte array in case of skipping
Fixes #2649