From 30534b125eb5981cb34e99b7eb9c58de1bbf7060 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. (cherry picked from commit f6abb80746fcb4809619879d3760a7292c5bbcc7) --- 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 172a4230840..52c38e68bae 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 f2b2d4c3adc..26d3cb8946c 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 dd6775fc5cd..94f645151f1 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 eb41f5fcb37..dbd10d944d8 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 71d90e32e56..df6c29b926d 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 0d649784383..3ff95767561 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 665c12bc20f..09392f11c93 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -9950,25 +9950,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 -- GitLab From 19be66772ce01eb7d14555c4064ac15e51067ddc 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] (cherry picked from commit 43e38a21ef70890e3b0512cc859af49b0259bb15) --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 38fe9f3ab55..9c9234abf1c 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] -- GitLab