Skip to content

fix packet encoding/decoding which seems to break after 2 billions packets sent#1743

Merged
ghedo merged 3 commits intocloudflare:masterfrom
frochet:fix_packet_encoding
Jun 26, 2024
Merged

fix packet encoding/decoding which seems to break after 2 billions packets sent#1743
ghedo merged 3 commits intocloudflare:masterfrom
frochet:fix_packet_encoding

Conversation

@frochet
Copy link
Copy Markdown
Contributor

@frochet frochet commented Mar 13, 2024

Hello,

Long story short, I've been using the encoding/decoding logic of packet number for another purpose than encoding pn, and observed that it would start decoding incorrect values when reaching 2^31.

I dived into RFC9000, on the Appendix for the algorithmic details of encoding/decoding, and observed that the encoding algorithm in quiche does not match the encoding algorithm suggested by the RFC. I suppose there is a reason for this, but it eludes me.

So, this pull request does an implementation close to what is suggested (hopefully more efficient), and fixes my problem, and the bug with packet numbers too. I've also added test cases for packet encoding/decoding using example values from the RFC.

This passes all unit tests, and a simple integration test in which I downloaded a 5 GiB file through the H3 module using quiche-[client/server]. However, I am unsure about:

  1. Why the largest acked value on a path can be bigger than the current packet number on that path.
  2. Why largest_acked values were initialized to UINT64::MAX; I changed this to 0, and removed a condition checking for UIN64::MAX. I had to do this, because pkt_num_len() is now using information from the largest_acked value.
  3. Whether the original pkt encoding is a bug, a forgotten todo, or something else. The fact that there were no test cases suggests a forgotten todo?

@frochet frochet requested a review from a team as a code owner March 13, 2024 10:32
@frochet frochet force-pushed the fix_packet_encoding branch 2 times, most recently from f6577ec to e9fb607 Compare March 13, 2024 11:19
Comment thread quiche/src/packet.rs Outdated
// computes ceil of num_unacked.log2()
let min_bits = u64::BITS - num_unacked.leading_zeros();
// get the num len in bytes
Ok((min_bits as f32 / 8.).ceil() as usize)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok((num_bits + 7) / 8 as usize) should be faster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right! Good catch. The Result is not useful anymore either, and I removed it.

@frochet
Copy link
Copy Markdown
Contributor Author

frochet commented Jun 25, 2024

@LPardue @ghedo I've other pull requests with potential quality of life improvements (nothing big) from my ongoing project using quiche. Are those welcome? If external contributions are not, I prefer not to spam :-)

(e.g., support of recvmmsg() for apps/src/client.rs used in HTTP download)

@ghedo
Copy link
Copy Markdown
Member

ghedo commented Jun 25, 2024

@frochet contributions are generally welcome (and btw, thank you very much for this PR!), but review might take some time due to reviewers (mostly me and Lucas) being busy or not available (or in other cases we might forget unfortunately :/).

However, "quality of life" can be subjective :) so it might be better to discuss things a bit in advance just to avoid you doing a bunch of work that might not get merged. Do you happen to be on the quicdev Slack channel? We have a room for quiche there which might be easier for this kind of communication than sending emails or creating a bunch of issues on GitHub. If you are not on there yet, it should be relatively easy to join, see https://github.com/quicwg/base-drafts/blob/main/CONTRIBUTING.md#following-discussion

Speaking of this PR, could you rebae it please? Looks like it might need some changes due to some refactoring in the recovery code that landed in the meantime.

@ghedo
Copy link
Copy Markdown
Member

ghedo commented Jun 25, 2024

@frochet also, specifically about recvmmsg() support, @evanrittenhouse has been pulling all that UDP socket logic into its own crate #1764 so that is probably a better place to look into for that change.

@frochet frochet force-pushed the fix_packet_encoding branch from caaeb39 to 986dcfb Compare June 26, 2024 07:19
@frochet
Copy link
Copy Markdown
Contributor Author

frochet commented Jun 26, 2024

I've asked for a quicdev slack invite. Thank you for the guidance :-)

Rebase done! It looks like some changes in Master addressed the first two questions in the first post as well, so great!

@ghedo ghedo merged commit 6eb0850 into cloudflare:master Jun 26, 2024
@ghedo
Copy link
Copy Markdown
Member

ghedo commented Jun 26, 2024

Merged, thanks @frochet! And sorry it took so long 😞

@ghedo
Copy link
Copy Markdown
Member

ghedo commented Jun 26, 2024

And also thanks @vanc for the review and suggestions!

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 this pull request may close these issues.

3 participants