Merge branch '4612-resolver-crashes-on-10-0-0-38-abcdefghijklmnopqrstuvwxyz012345-plex-direct-ds-query' into 'main'

Resolve "resolver crashes on 10-0-0-38.abcdefghijklmnopqrstuvwxyz012345.plex.direct DS query"

Closes #4612

See merge request isc-projects/bind9!8794

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

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"));

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"));
 	}

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

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"

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);