udpsession may drop correctly decoded packets #296
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently, I started using this KCP-based project, and while testing over localhost (where packets are decoded locally and re-sent to another endpoint), I occasionally observe that the number of decoded/recovered packets exceeds the number of packets actually received. In some cases, although the counts match, the contents of the recovered packets appear to be truncated. I'm only using the FEC component from KCP and not KCP itself, but it seems to me that the FEC decoder should be usable independently.
During unit testing, I noticed that the value obtained from:
sz := binary.LittleEndian.Uint16(r)
is often larger than the length of the recovered slice r itself.
for example:
recoverd: [132 0 10 0 49 95 85 120 107 65 88 79 111 122 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]
Here, sz = 132, which is clearly larger than the length of the packet.
The original test string is only 10 characters long: "1_UxkAXOoz" (I add a uint16 to specified it's length)
another example:
[102 0 7 0 49 95 79 107 114 68 112 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0]
for string "1_OkrDp"
If I add a defensive check like
if int(sz) <= len(r)
then such packets will be dropped. However, from what I can see, the packet contents appear to be fully and correctly recovered. If I don’t include this check, all of my test cases pass successfully, and I haven’t encountered any instances of incorrect recovery.
This leads me to believe that this part of the code logic might be problematic (or too strict), and I'm wondering whether the length calculation here should be revisited.