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.

Sign up for GitHub

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

Jump to bottom

Fix truncation for UDP #1975

Merged
merged 1 commit into from Jun 27, 2023
Merged

Fix truncation for UDP #1975

merged 1 commit into from Jun 27, 2023

Conversation

nmittler
Copy link
Contributor

@nmittler nmittler commented Jun 21, 2023 •
edited

This fixes a couple of issues for UDP on both the client and server:

  • Previously, the UdpClientStream was using a fixed 2048 for the size of the receive buffer. This can cause problems on interfaces with a larger MTU. PR 1096 adjusted this value on the server side to 4096 (the maximum as recommended by RFC6891). This PR sets a constant that is shared by the UDP client and server. Additionally, the client uses EDNS in the request to further trim down the buffer size.
  • The Server previously was not setting a maximum for the BinEncoder, which defaults to u16::MAX (i.e. effectively no truncation for UDP). This PR sets an appropriate maximum for the BinEncoder based on the response EDNS and protocol being used.

Fixes: #1973

@nmittler nmittler mentioned this pull request Jun 21, 2023
Closed
@nmittler nmittler force-pushed the recv_buf branch 2 times, most recently from a110ac2 to d1b8126 Compare June 21, 2023 01:05
djc
djc reviewed Jun 21, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
@nmittler
Copy link
Contributor Author

nmittler commented Jun 21, 2023 •
edited

@djc removed the use of MTU for now. I'm now using a combination of a larger constant as well as EDNS from the request.

@nmittler nmittler force-pushed the recv_buf branch 2 times, most recently from 710cace to 15498b9 Compare June 21, 2023 15:58
@nmittler
Copy link
Contributor Author

nmittler commented Jun 21, 2023 •
edited

@djc @bluejekyll I've expanded this to include a new integration test for truncation that was failing for UDP with large messages (as described in #1973) and added some changes that get it working. PTAL.

@nmittler nmittler changed the title [WIP] Improve UDP client receive buffer size [WIP] Fix truncation for UDP Jun 21, 2023
@nmittler nmittler changed the title [WIP] Fix truncation for UDP Fix truncation for UDP Jun 21, 2023
@nmittler nmittler mentioned this pull request Jun 21, 2023
Merged
bluejekyll
bluejekyll reviewed Jun 25, 2023
View reviewed changes
Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Overall this looks good. See my one question. I need to look over some of the other changes in detail. But I think this is looking good. Thanks for putting this together!

crates/server/src/server/response_handler.rs Outdated Show resolved Hide resolved
tests/integration-tests/tests/truncation_tests.rs Outdated Show resolved Hide resolved
@nmittler nmittler force-pushed the recv_buf branch 3 times, most recently from bc447c2 to 5bed981 Compare June 25, 2023 21:03
@nmittler
Fix truncation for UDP
96681da 
This fixes a couple of issues for UDP on both the client and server:

* Previously, the UdpClientStream was using a fixed `2048` for the size of the receive buffer. This can cause problems on interfaces with a larger MTU. hickory-dns#1096 adjusted this value on the server side to 4096 (the maximum as recommended by RFC6891). This PR sets a constant that is shared by the UDP client and server. Additionally, the client uses EDNS in the request to further trim down the buffer size.
* The Server previously was not setting a maximum for the `BinEncoder`, which defaults to `u16::MAX` (i.e. effectively no truncation for UDP). This PR sets an appropriate maximum for the `BinEncoder` based on the response EDNS and protocol being used.

Fixes: hickory-dns#1973
@nmittler
Copy link
Contributor Author

@djc @bluejekyll any ideas what's going on with the audit issue? I don't believe it's related to my PR.

@djc
Copy link
Collaborator

djc commented Jun 26, 2023

It's unrelated, just need an OpenSSL upgrade.

@nmittler
Copy link
Contributor Author

nmittler commented Jun 26, 2023 •
edited

It's unrelated, just need an OpenSSL upgrade.

@djc can this be force-merged? Or should we hold off until the openssl is upgraded?

@djc
Copy link
Collaborator

djc commented Jun 27, 2023

It's unrelated, just need an OpenSSL upgrade.

@djc can this be force-merged? Or should we hold off until the openssl is upgraded?

It can be force-merged, though only @bluejekyll currently has those privileges.

bluejekyll
bluejekyll approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning all of this up. looks good.

@bluejekyll bluejekyll merged commit 0a1306b into hickory-dns:main Jun 27, 2023
17 of 18 checks passed
@t0m t0m mentioned this pull request Feb 17, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

@bluejekyll bluejekyll bluejekyll approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UdpClientStream fails to parse large responses
3 participants
@nmittler @djc @bluejekyll