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

Overhaul handling of SRV DNS responses #81

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexdowad
Copy link
Contributor

@nils-ohlmeier, please don't merge this immediately... I am just opening this PR to allow for code review.

The new DNS test suite does (indirectly) use c-ares, but does not issue any DNS requests, except for localhost... the only function from c-ares which is used is ares_expand_name, which just operates on a buffer in memory and does not touch the network or have any other side effects. This does mean, however, that the tests can only run on a host which has c-ares installed. Probably I should add a check in check_dns.c to see if c-ares is installed.

I tried running the unit tests under Valgrind and it didn't detect any memory leaks, use of uninitialized memory, buffer overruns, etc.

When I am working on the PHP interpreter, I build it with ubsan enabled, and it has helped me to catch bugs. I notice that you are using -Wall -fstack-protector for building the tests, which is good, but I would also love to add ubsan. (-fsanitize=undefined -fno-sanitize-recover)

The behavior of CNAME is changed a bit here. In the previous code, if ca_tmpname matched the name in a CNAME record, ca_tmpname would be replaced by the value of the CNAME record... but even if it didn't match, the value of the CNAME record would still replace ca_tmpname, and if the value of the CNAME record was an IPv4 address, it would immediately be returned as the IP address to use.

Now, CNAMEs only have an effect if their name matches the value of a SRV record.

Do you think it is possible that a DNS server might first return a CNAME record and then follow it with the matching SRV record? If so, I need to modify my code to account for that possibility. Right now, it needs to see the SRV record before the CNAME, or else the CNAME will not have any effect.

@alexdowad
Copy link
Contributor Author

OK, I see that the CI checks can't pass if a test suite only runs on hosts with c-ares.

Just amended that so the new test suite still runs and 'passes', but doesn't actually test anything, on hosts without c-ares.

• The 'weight' field on SRV records is now respected
• Handling of CNAME is adjusted
• We also now have unit tests for the handling of SRV DNS responses
@alexdowad
Copy link
Contributor Author

Hmm, I was just thinking more about CNAMES...

I can imagine a scenario where we do a SRV lookup for a.b.c and the DNS server responds with just a CNAME redirecting us from a.b.c to something.else. If that happens, would the right thing to do be: 1) do a recursive SRV lookup for something.else, or 2) do an A/AAAA lookup for something.else and use the resulting IP address?

@nils-ohlmeier
Copy link
Owner

I can imagine a scenario where we do a SRV lookup for a.b.c and the DNS server responds with just a CNAME redirecting us from a.b.c to something.else. If that happens, would the right thing to do be: 1) do a recursive SRV lookup for something.else, or 2) do an A/AAAA lookup for something.else and use the resulting IP address?

Good question. I'm not a DNS expert. But I don't think that CNAMES can redirect to anything else then A/AAAA.

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.

2 participants