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

[Regression] Local DNS now defaults to tcp_only mode, causing UDP-only clients to fail #1281

Closed
spyophobia opened this issue Aug 28, 2023 · 3 comments · Fixed by #1285
Closed

Comments

@spyophobia
Copy link
Contributor

Steps to reproduce

cargo test --features dns-over-https,dns-over-tls,local-dns,local-http-rustls,local-redir,local-tun dns_relay -- --nocapture

Observe that the test hangs indefinitely.

Environments tested

  1. My local machine: x86_64, ArchLinux, Rust 1.72.0 (x86_64-unknown-linux-gnu)
  2. My COPR build: x86_64 & aarch64, RHEL 7-9 & Fedora 37-Rawhide (containerized), Rust 1.72.0 (x86_64-unknown-linux-gnu / aarch64-unknown-linux-gnu)

Things tried

  1. Disable dns_relay test: cargo test --features dns-over-https,dns-over-tls,local-dns,local-http-rustls,local-redir,local-tun -- --skip dns_relay; all other tests complete successfully
  2. Disable firewall: sudo systemctl stop firewalld; no effect

Debugging info

  • I inserted some printlns and it seems like it's stuck here:

let n = c.recv(&mut buf).await.unwrap();

  • A git bisect pinpointed the issue to 8d3461b.
@spyophobia
Copy link
Contributor Author

spyophobia commented Aug 28, 2023

Two issues here:

  1. Test is failing.
  2. Can we institute some kind of timeout for all tests? This way the tests fail instead of hanging forever. Unfortunately cargo doesn't yet support timeouts (rfc-2798), so maybe use something like ntest?

@spyophobia
Copy link
Contributor Author

spyophobia commented Aug 31, 2023

I just noticed today that local DNS is just not working at all in 1.16.0. Connections are all getting refused. So this is a serious regression. I'll take a look at the problematic commit and see what I can do.

@spyophobia spyophobia changed the title Test dns_relay hangs indefinitely [Regression] Local DNS is broken in 1.16.0; test dns_relay hangs indefinitely Aug 31, 2023
@spyophobia
Copy link
Contributor Author

Ah okay. After some investigation it seems like the issue is less severe than I thought.

So in 1.15.4, the local DNS client will always create a TCP and a UDP server regardless of the mode configured:

pub async fn run(self, bind_addr: &ServerAddr, balancer: PingBalancer) -> io::Result<()> {
let client = Arc::new(DnsClient::new(self.context.clone(), balancer, self.mode));
let tcp_fut = self.run_tcp_server(bind_addr, client.clone());
let udp_fut = self.run_udp_server(bind_addr, client);
tokio::pin!(tcp_fut, udp_fut);
match future::select(tcp_fut, udp_fut).await {
Either::Left((res, ..)) => res,
Either::Right((res, ..)) => res,
}
}

But in 1.16.0, the mode is now respected:

let mut tcp_server = None;
if self.mode.enable_tcp() {
let server = DnsTcpServer::new(
self.context.clone(),
&self.bind_addr,
local_addr.clone(),
remote_addr.clone(),
client.clone(),
)
.await?;
tcp_server = Some(server);
}
let mut udp_server = None;
if self.mode.enable_udp() {
let server = DnsUdpServer::new(self.context, &self.bind_addr, local_addr, remote_addr, client).await?;
udp_server = Some(server);
}

This is a good change in theory; unfortunately it had the side-effect of changing the default behavior from "TCP and UDP" to "TCP only", and AFAIK, was not documented in the changelog. What's worse is that lots of DNS clients only support UDP (in particular, Windows; this is how I initially noticed the problem; my Windows VM was having DNS problems while my Linux VMs were fine), which made it look like the feature has stopped working entirely.

@spyophobia spyophobia changed the title [Regression] Local DNS is broken in 1.16.0; test dns_relay hangs indefinitely [Regression] Local DNS now defaults to tcp_only mode, causing UDP-only clients to fail Aug 31, 2023
spyophobia added a commit to spyophobia/shadowsocks-rust that referenced this issue Aug 31, 2023
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.

1 participant