From 5ff55e13e867b030e768d537c83ec84427b44403 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 29 Feb 2024 14:00:58 +1100 Subject: [PATCH 1/3] Restore the disassociate call to before the fetch [GL #3709] reordered the dns_rdataset_disassociate call to after the dns_resolver_createfetch call resulting in qctx->nsrrset still being associated when dns_resolver_createfetch is called in resume_dslookup (7e4e125e). Revert that part of the change and add comments as to why the multiple dns_rdataset_disassociate calls are where they are. --- lib/dns/resolver.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 635b9ea019..0570ff45a2 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -6964,7 +6964,7 @@ resume_dslookup(void *arg) { } /* Preserve data from resp before freeing it. */ - frdataset = resp->rdataset; + frdataset = resp->rdataset; /* a.k.a. fctx->nsrrset */ result = resp->result; isc_mem_putanddetach(&resp->mctx, resp, sizeof(*resp)); @@ -6988,6 +6988,13 @@ resume_dslookup(void *arg) { } dns_rdataset_clone(frdataset, &fctx->nameservers); + /* + * Disassociate now the NS's are saved. + */ + if (dns_rdataset_isassociated(frdataset)) { + dns_rdataset_disassociate(frdataset); + } + fctx->ns_ttl = fctx->nameservers.ttl; fctx->ns_ttl_ok = true; log_ns_ttl(fctx, "resume_dslookup"); @@ -7005,10 +7012,21 @@ resume_dslookup(void *arg) { case ISC_R_SHUTTINGDOWN: case ISC_R_CANCELED: - /* Don't try anymore */ + /* Don't try anymore. */ + /* Can't be done in cleanup. */ + if (dns_rdataset_isassociated(frdataset)) { + dns_rdataset_disassociate(frdataset); + } goto cleanup; default: + /* + * Disassociate for the next dns_resolver_createfetch call. + */ + if (dns_rdataset_isassociated(frdataset)) { + dns_rdataset_disassociate(frdataset); + } + /* * If the chain of resume_dslookup() invocations managed to * chop off enough labels from the original DS owner name to @@ -7058,10 +7076,6 @@ resume_dslookup(void *arg) { cleanup: dns_resolver_destroyfetch(&fetch); - if (dns_rdataset_isassociated(frdataset)) { - dns_rdataset_disassociate(frdataset); - } - if (result != ISC_R_SUCCESS) { /* An error occurred, tear down whole fctx */ fctx_done_unref(fctx, result); From 3fedbb1a66970fedcfcf084ffe333f469e2cd805 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Tue, 5 Mar 2024 15:51:05 +1100 Subject: [PATCH 2/3] test: DS query against broken NODATA responses This is a regresssion test for GL #4621 where the NODATA responses are SOA records that match the QNAME rather than the zone name. In particular for NS queries. --- bin/tests/system/resolver/ans2/ans.pl | 32 +++++++++++++++++++++++++++ bin/tests/system/resolver/ans3/ans.pl | 32 +++++++++++++++++++++++++++ bin/tests/system/resolver/ns6/root.db | 8 +++++++ bin/tests/system/resolver/tests.sh | 8 +++++++ 4 files changed, 80 insertions(+) diff --git a/bin/tests/system/resolver/ans2/ans.pl b/bin/tests/system/resolver/ans2/ans.pl index aa1d51b57a..b17fd6ec2e 100644 --- a/bin/tests/system/resolver/ans2/ans.pl +++ b/bin/tests/system/resolver/ans2/ans.pl @@ -133,6 +133,38 @@ for (;;) { $packet->push("additional", new Net::DNS::RR("ns.broken 300 A 10.53.0.4")); } elsif ($qname =~ /\.partial-formerr/) { $packet->header->rcode("FORMERR"); + } elsif ($qname eq "gl6412") { + if ($qtype eq "SOA") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } elsif ($qtype eq "NS") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 NS ns2" . $qname)); + $packet->push("answer", + new Net::DNS::RR($qname . " 300 NS ns3" . $qname)); + } else { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } + } elsif ($qname eq "a.gl6412" || $qname eq "a.a.gl6412") { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } elsif ($qname eq "ns2.gl6412") { + if ($qtype eq "A") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 A 10.53.0.2")); + } else { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } + } elsif ($qname eq "ns3.gl6412") { + if ($qtype eq "A") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 A 10.53.0.3")); + } else { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } } 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 893c9ed8d5..98f4ec04b5 100644 --- a/bin/tests/system/resolver/ans3/ans.pl +++ b/bin/tests/system/resolver/ans3/ans.pl @@ -142,6 +142,38 @@ sub handleQuery { } elsif ($qname =~ /\.partial-formerr/) { $packet->push("answer", new Net::DNS::RR($qname . " 1 A 10.53.0.3")); + } elsif ($qname eq "gl6412") { + if ($qtype eq "SOA") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } elsif ($qtype eq "NS") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 NS ns2" . $qname)); + $packet->push("answer", + new Net::DNS::RR($qname . " 300 NS ns3" . $qname)); + } else { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } + } elsif ($qname eq "a.gl6412" || $qname eq "a.a.gl6412") { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } elsif ($qname eq "ns2.gl6412") { + if ($qtype eq "A") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 A 10.53.0.2")); + } else { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } + } elsif ($qname eq "ns3.gl6412") { + if ($qtype eq "A") { + $packet->push("answer", + new Net::DNS::RR($qname . " 300 A 10.53.0.3")); + } else { + $packet->push("authority", + new Net::DNS::RR($qname . " 300 SOA . . 0 0 0 0 0")); + } } else { $packet->push("answer", new Net::DNS::RR("www.example.com 300 A 1.2.3.4")); } diff --git a/bin/tests/system/resolver/ns6/root.db b/bin/tests/system/resolver/ns6/root.db index 096381c3ce..35962fedf6 100644 --- a/bin/tests/system/resolver/ns6/root.db +++ b/bin/tests/system/resolver/ns6/root.db @@ -34,3 +34,11 @@ edns-version.tld. NS ns.edns-version.tld. ns.edns-version.tld. A 10.53.0.7 v4only.net. NS v4.nameserver. v4.nameserver. A 10.53.0.4 +; +; Servers for regression test for GL #6412 +; They return broken NODATA responses (incorrect SOA) for the test zone. +; +gl6412. NS ns2.gl6412. +gl6412. NS ns3.gl6412. +ns2.gl6412. A 10.53.0.2 +ns3.gl6412. A 10.53.0.3 diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 65ef9b928b..c446197126 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -829,6 +829,14 @@ grep "status: NOERROR" dig.out.${n} >/dev/null || ret=1 nextpart ns5/named.run | grep "$msg" >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi +n=$((n + 1)) +echo_i "GL#4612 regression test: DS query against broken NODATA responses (${n})" +# servers ns2 and ns3 return authority SOA which matches QNAME rather than the zone +ret=0 +dig_with_opts @10.53.0.7 a.a.gl6412 DS >dig.out.${n} || ret=1 +grep "status: SERVFAIL" dig.out.${n} >/dev/null || ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi + status=$((status + ret)) echo_i "exit status: $status" From f4f7f23d9de37cc24f65cb81c1b4d80152d3ddcb Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 29 Feb 2024 14:10:42 +1100 Subject: [PATCH 3/3] Add CHANGES note for [GL #4612] --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES b/CHANGES index b6657661e6..73699d4d22 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6354. [bug] Change 6035 introduced a regression when chasing DS + records resulting in an assertion failure. [GL #4612] + 6353. [bug] Improve the TTL-based cleaning by removing the expired headers from the heap, so they don't block the next cleaning round and clean more than a single item for