From f6abb80746fcb4809619879d3760a7292c5bbcc7 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 1 Jul 2022 14:23:33 -0700 Subject: [PATCH 1/2] try other servers when receiving FORMERR previously, when an iterative query returned FORMERR, resolution would be stopped under the assumption that other servers for the same domain would likely have the same capabilities. this assumption is not correct; some domains have been reported for which some but not all servers will return FORMERR to a given query; retrying allows recursion to succeed. --- bin/tests/system/qmin/clean.sh | 2 +- bin/tests/system/qmin/tests.sh | 2 +- bin/tests/system/resolver/ans2/ans.pl | 2 ++ bin/tests/system/resolver/ans3/ans.pl | 3 +++ bin/tests/system/resolver/ns4/root.db | 3 +++ bin/tests/system/resolver/tests.sh | 8 ++++++++ lib/dns/resolver.c | 26 +++++++------------------- 7 files changed, 25 insertions(+), 21 deletions(-) diff --git a/bin/tests/system/qmin/clean.sh b/bin/tests/system/qmin/clean.sh index 172a423084..52c38e68ba 100644 --- a/bin/tests/system/qmin/clean.sh +++ b/bin/tests/system/qmin/clean.sh @@ -13,7 +13,7 @@ rm -f ns*/named.conf rm -f */named.memstats -rm -f */named.run +rm -f */named.run */named.run.prev rm -f dig.out.* rm -f ns*/named.lock rm -f ans*/query.log* diff --git a/bin/tests/system/qmin/tests.sh b/bin/tests/system/qmin/tests.sh index f2b2d4c3ad..26d3cb8946 100755 --- a/bin/tests/system/qmin/tests.sh +++ b/bin/tests/system/qmin/tests.sh @@ -272,7 +272,7 @@ $RNDCCMD 10.53.0.7 flush n=$((n+1)) echo_i "information that minimization was unsuccessful for .ugly is logged ($n)" ret=0 -grep "success resolving 'icky.icky.icky.ptang.zoop.boing.ugly/A' after disabling qname minimization due to 'FORMERR'" ns7/named.run > /dev/null || ret=1 +wait_for_log 5 "success resolving 'icky.icky.icky.ptang.zoop.boing.ugly/A' after disabling qname minimization" ns7/named.run > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status+ret)) diff --git a/bin/tests/system/resolver/ans2/ans.pl b/bin/tests/system/resolver/ans2/ans.pl index dd6775fc5c..94f645151f 100644 --- a/bin/tests/system/resolver/ans2/ans.pl +++ b/bin/tests/system/resolver/ans2/ans.pl @@ -122,6 +122,8 @@ for (;;) { # Delegation to broken TLD. $packet->push("authority", new Net::DNS::RR("broken 300 NS ns.broken")); $packet->push("additional", new Net::DNS::RR("ns.broken 300 A 10.53.0.4")); + } elsif ($qname =~ /\.partial-formerr/) { + $packet->header->rcode("FORMERR"); } else { # Data for the "bogus referrals" test $packet->push("authority", new Net::DNS::RR("below.www.example.com 300 NS ns.below.www.example.com")); diff --git a/bin/tests/system/resolver/ans3/ans.pl b/bin/tests/system/resolver/ans3/ans.pl index eb41f5fcb3..dbd10d944d 100644 --- a/bin/tests/system/resolver/ans3/ans.pl +++ b/bin/tests/system/resolver/ans3/ans.pl @@ -125,6 +125,9 @@ for (;;) { $packet->push("answer", new Net::DNS::RR($qname . " 300 A 10.53.0.3")); + } elsif ($qname =~ /\.partial-formerr/) { + $packet->push("answer", + new Net::DNS::RR($qname . " 1 A 10.53.0.3")); } else { $packet->push("answer", new Net::DNS::RR("www.example.com 300 A 1.2.3.4")); } diff --git a/bin/tests/system/resolver/ns4/root.db b/bin/tests/system/resolver/ns4/root.db index 71d90e32e5..df6c29b926 100644 --- a/bin/tests/system/resolver/ns4/root.db +++ b/bin/tests/system/resolver/ns4/root.db @@ -32,3 +32,6 @@ sourcens. NS ns.sourcens. ns.sourcens. A 10.53.0.4 targetns. NS ns.targetns. ns.targetns. A 10.53.0.6 +partial-formerr. NS ns.partial-formerr. +ns.partial-formerr. A 10.53.0.2 +ns.partial-formerr. A 10.53.0.3 diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 0d64978438..3ff9576756 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -862,6 +862,14 @@ grep "status: SERVFAIL" dig.ns5.out.${n} > /dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) +n=$((n+1)) +echo_i "checking SERVFAIL is not returned if only some authoritative servers return FORMERR ($n)" +ret=0 +dig_with_opts @10.53.0.5 ns.partial-formerr. a > dig.ns5.out.${n} || ret=1 +grep "status: SERVFAIL" dig.ns5.out.${n} > /dev/null && ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=$((status + ret)) + n=$((n+1)) echo_i "check logged command line ($n)" ret=0 diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 7392e2bf6d..ba258a82fe 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -9890,25 +9890,13 @@ rctx_badserver(respctx_t *rctx, isc_result_t result) { add_bad_edns(fctx, &query->addrinfo->sockaddr); inc_stats(fctx->res, dns_resstatscounter_edns0fail); } else if (rcode == dns_rcode_formerr) { - if (ISFORWARDER(query->addrinfo)) { - /* - * This forwarder doesn't understand us, - * but other forwarders might. Keep trying. - */ - rctx->broken_server = DNS_R_REMOTEFORMERR; - rctx->next_server = true; - } else { - /* - * The server doesn't understand us. Since - * all servers for a zone need similar - * capabilities, we assume that we will get - * FORMERR from all servers, and thus we - * cannot make any more progress with this - * fetch. - */ - log_formerr(fctx, "server sent FORMERR"); - result = DNS_R_FORMERR; - } + /* + * The server (or forwarder) doesn't understand us, + * but others might. + */ + rctx->next_server = true; + rctx->broken_server = DNS_R_REMOTEFORMERR; + log_formerr(fctx, "server sent FORMERR"); } else if (rcode == dns_rcode_badvers) { unsigned int version; #if DNS_EDNS_VERSION > 0 From 43e38a21ef70890e3b0512cc859af49b0259bb15 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 1 Jul 2022 14:28:26 -0700 Subject: [PATCH 2/2] CHANGES for [GL #3152] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 6608a674cd..e3912d5958 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5916. [bug] When resolving a name, don't give up immediately if an + authoritative server returns FORMERR; try the other + servers first. [GL #3152] + 5915. [bug] Detect missing closing brace (}) and computational overflows in $GENERATE directives. [GL #3429]