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.

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

UdpClientStream fails to parse large responses #1973

Closed
nmittler opened this issue Jun 20, 2023 · 5 comments · Fixed by #1975
Closed

UdpClientStream fails to parse large responses #1973

nmittler opened this issue Jun 20, 2023 · 5 comments · Fixed by #1975

Comments

@nmittler
Copy link
Contributor

nmittler commented Jun 20, 2023

Describe the bug
I'm trying to build a local test for truncation, and ran into an issue. The test uses UdpClientStream to call a local server (Catalog + InMemoryAuthority) which responds a large record set resulting in a UDP payload of 2080 bytes (verified via WireShark). Running with tracing enabled, I see:

2023-06-20T17:59:48.758321Z  WARN trust_dns_proto::udp::udp_client_stream: dropped malformed message waiting for id: 9851 err: unexpected end of input reached

Digging in a little further, I see that the client is only seeing 2048 of the 2080 bytes. The MTU for lo0 is 16384, so it's not being truncated down the stack (Wireshark confirms).

The problem appears to be thatUdpClientStream explicitly limits the size of the reply buffer to 2048. Since each read assumes a complete UDP packet, parsing fails.

To Reproduce
See description.

Expected behavior
Replies larger than 2048 bytes should be supported by UdpClientStream.

Docs for tokio UdpSocket indicate that reads should generally be done with the max UDP packet size of 65536. It may be a larger buffer than we'd like in general, but it would at least be correct.

System:

  • OS: mac OSX
  • Architecture: arm64
  • Version: 13.4
  • rustc version: 1.70.0

Version:
Crate: client, server
Version: 0.22.0

Additional context
NA

@nmittler
Copy link
Contributor Author

@bluejekyll

Related to this is the fact that DNS truncation (TC=1) doesn't appear to be happening in my test. EDNS has max_payload set to 512 (the default), yet the response still comes through without truncation.

I believe that issue may be related to the fact that BinEncoder uses a MaximalBuf with a maximum of u16::max_value(). IIUC, this could be why there is no truncation for a payload of 2080.

I'm happy to raise this as a separate bug if you think it's a real issue.

@djc
Copy link
Collaborator

djc commented Jun 20, 2023

Maybe run a little git blame on the UdpClientStream code to figure out if there were changes to the size of the buffer? It's tricky because I guess we only have one shot to receive, but we might not want to keep a 64k buffer allocated throughout the lifetime of the stream when it's unlikely we'll use the entire size of the buffer.

@nmittler
Copy link
Contributor Author

nmittler commented Jun 20, 2023

@djc Yeah that was my thought as well. Perhaps we could use the EDNS max_payload value from the request if available, to trim this down a bit? And/or we could use the MTU of the network interface ... this would likely only be large-ish for a loopback interface.

FYI, the 2048 value goes back all the way to the initial implementation: #46.

nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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.

Updated the code to use the MTU for the local interface, if available. Otherwise default to the maximum packet size.

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

@bluejekyll @djc I threw together a WIP PR: #1975. Right now it just uses MTU, but we can expand it. Should probably also have a proper e2e test specifically for this problem.

nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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.

Updated the code to use the MTU for the local interface, if available. Otherwise default to the maximum packet size.

Fixes: hickory-dns#1973
nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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.

Updated the code to use the MTU for the local interface, if available. Otherwise default to the maximum packet size.

Fixes: hickory-dns#1973
@djc
Copy link
Collaborator

djc commented Jun 21, 2023

I guess the MTU is accurate for loopback interfaces at least, but I suppose actual network path MTUs would be constrained by any number of other interfaces on the path to the remote, so it's maybe not very meaningful? Additionally at least the crates I've looked at so far for retrieving the MTU are pretty heavyweight dependencies which IMO isn't very attractive.

nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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 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.

Fixes: hickory-dns#1973
nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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 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.

Fixes: hickory-dns#1973
nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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 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.

Fixes: hickory-dns#1973
nmittler added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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 added a commit to nmittler/trust-dns that referenced this issue Jun 21, 2023
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 added a commit to nmittler/trust-dns that referenced this issue Jun 25, 2023
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 added a commit to nmittler/trust-dns that referenced this issue Jun 25, 2023
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 added a commit to nmittler/trust-dns that referenced this issue Jun 25, 2023
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 added a commit to nmittler/trust-dns that referenced this issue Jun 26, 2023
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 added a commit to nmittler/trust-dns that referenced this issue Jun 26, 2023
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
bluejekyll pushed a commit that referenced this issue Jun 27, 2023
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. #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
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 a pull request may close this issue.

2 participants