Merge branch '2733-serve-stale-prefetch-crash' into 'main'

Fix crash with serve-stale in combination with prefetch

Closes #2733

See merge request isc-projects/bind9!5111

CHANGES

@@ -1,3 +1,7 @@
+5650.	[bug]		Prevent a crash that could occur if serve-stale
+			was enabled and a prefetch was triggered during a
+			query restart. [GL #2733]
+
 5649.	[bug]		If a query was answered with stale data on a server
 			with DNS64 enabled, an assertion could occur if a
 			non-stale answer arrived afterward. [GL #2731]

bin/tests/system/serve-stale/ans2/ans.pl

@@ -26,7 +26,12 @@ sub rmpid { unlink "ans.pid"; exit 1; };
 $SIG{INT} = \&rmpid;
 $SIG{TERM} = \&rmpid;
 
+# If send_response is set, the server will respond, otherwise the query will
+# be dropped.
 my $send_response = 1;
+# If slow_response is set, a lookup for the CNAME target (target.example) is
+# delayed. Other lookups will not be delayed.
+my $slow_response = 0;
 
 my $localaddr = "10.53.0.2";
 
@@ -49,6 +54,8 @@ my $TXT = "data.example 2 IN TXT \"A text record with a 2 second ttl\"";
 my $LONGTXT = "longttl.example 600 IN TXT \"A text record with a 600 second ttl\"";
 my $CAA = "othertype.example 2 IN CAA 0 issue \"ca1.example.net\"";
 my $negSOA = "example 2 IN SOA . . 0 0 0 0 300";
+my $CNAME = "cname.example 7 IN CNAME target.example";
+my $TARGET = "target.example 9 IN A $localaddr";
 
 sub reply_handler {
     my ($qname, $qclass, $qtype) = @_;
@@ -75,6 +82,15 @@ sub reply_handler {
 	}
 	$rcode = "NOERROR";
         return ($rcode, \@ans, \@auth, \@add, { aa => 1 });
+    } elsif ($qname eq "slowdown" ) {
+	if ($qtype eq "TXT") {
+	    $send_response = 1;
+	    $slow_response = 1;
+            my $rr = new Net::DNS::RR("$qname 0 $qclass TXT \"$send_response\"");
+            push @ans, $rr;
+	}
+	$rcode = "NOERROR";
+        return ($rcode, \@ans, \@auth, \@add, { aa => 1 });
     }
 
     # If we are not responding to queries we are done.
@@ -118,7 +134,7 @@ sub reply_handler {
 	}
 	$rcode = "NOERROR";
     } elsif ($qname eq "a-only.example") {
-        if ($qtype eq "A") {
+	if ($qtype eq "A") {
 	    my $rr = new Net::DNS::RR("a-only.example 2 IN A $localaddr");
 	    push @ans, $rr;
 	} else {
@@ -126,6 +142,28 @@ sub reply_handler {
 	    push @auth, $rr;
 	}
 	$rcode = "NOERROR";
+    } elsif ($qname eq "cname.example") {
+	if ($qtype eq "A") {
+	    my $rr = new Net::DNS::RR($CNAME);
+	    push @ans, $rr;
+	} else {
+	    my $rr = new Net::DNS::RR($negSOA);
+	    push @auth, $rr;
+	}
+	$rcode = "NOERROR";
+    } elsif ($qname eq "target.example") {
+	if ($slow_response) {
+                print "  Sleeping 3 seconds\n";
+		sleep(3);
+	}
+	if ($qtype eq "A") {
+	    my $rr = new Net::DNS::RR($TARGET);
+	    push @ans, $rr;
+	} else {
+	    my $rr = new Net::DNS::RR($negSOA);
+	    push @auth, $rr;
+	}
+	$rcode = "NOERROR";
     } elsif ($qname eq "longttl.example") {
 	if ($qtype eq "TXT") {
 	    my $rr = new Net::DNS::RR($LONGTXT);

bin/tests/system/serve-stale/clean.sh

@@ -7,7 +7,7 @@
 # See the COPYRIGHT file distributed with this work for additional
 # information regarding copyright ownership.
 
-rm -f dig.out.test*
+rm -f dig.out*
 rm -f ns*/named.conf
 rm -f ns*/root.bk
 rm -f rndc.out.test*

bin/tests/system/serve-stale/ns3/named8.conf.in

@@ -29,11 +29,12 @@ options {
 	recursion yes;
 	stale-answer-enable yes;
 	stale-cache-enable yes;
-        stale-answer-client-timeout 1800;
+	stale-answer-client-timeout 1800;
+	prefetch 2 8;
 	dns64 2001:aaaa::/96 {
 		clients { any; };
-                mapped { any; };
-        };
+		mapped { any; };
+	};
 };
 
 zone "." {

bin/tests/system/serve-stale/tests.sh

@@ -2244,12 +2244,14 @@ status=$((status+ret))
 
 n=$((n+1))
 echo_i "check DNS64 processing of a stale negative answer ($n)"
+ret=0
 # configure ns3 with dns64
 copy_setports ns3/named8.conf.in ns3/named.conf
 rndc_reload ns3 10.53.0.3
-# flush cache, enable ans2 responses
+# flush cache, enable ans2 responses, make sure serve-stale is on
 $RNDCCMD 10.53.0.3 flush > rndc.out.test$n.1 2>&1 || ret=1
 $DIG -p ${PORT} @10.53.0.2 txt enable > /dev/null
+$RNDCCMD 10.53.0.3 serve-stale on > rndc.out.test$n.2 2>&1 || ret=1
 # prime the cache with an AAAA NXRRSET response
 $DIG -p ${PORT} @10.53.0.3 a-only.example AAAA > dig.out.1.test$n
 grep "status: NOERROR" dig.out.1.test$n > /dev/null || ret=1
@@ -2269,5 +2271,77 @@ grep "2001:aaaa" dig.out.2.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
+###########################################################
+# Test serve-stale's interaction with prefetch processing #
+###########################################################
+echo_i "test serve-stale's interaction with prefetch processing"
+
+# Test case for #2733, ensuring that prefetch queries do not trigger
+# a lookup due to stale-answer-client-timeout.
+#
+# 1. Cache the following records:
+#    cname.example 7 IN CNAME target.example.
+#    target.example 9 IN A <addr>.
+# 2. Let the CNAME RRset expire.
+# 3. Query for 'cname.example/A'.
+#
+# This starts recursion because cname.example/CNAME is expired.
+# The authoritative server is up so likely it will respond before
+#  stale-answer-client-timeout is triggered.
+# The 'target.example/A' RRset is found in cache with a positive value
+#  and is eligble for prefetching.
+# A prefetch is done for 'target.example/A', our ans2 server will
+#  delay the request.
+# The 'prefetch_done()' callback should have the right event type
+#  (DNS_EVENT_FETCHDONE).
+
+# flush cache
+n=$((n+1))
+echo_i "flush cache ($n)"
+ret=0
+$RNDCCMD 10.53.0.3 flushtree example > rndc.out.test$n.1 2>&1 || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# prime the cache with CNAME and A; CNAME expires sooner
+n=$((n+1))
+echo_i "prime cache cname.example (stale-answer-client-timeout 1.8) ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 cname.example A > dig.out.test$n
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 2," dig.out.test$n > /dev/null || ret=1
+grep "cname\.example\..*7.*IN.*CNAME.*target\.example\." dig.out.test$n > /dev/null || ret=1
+grep "target\.example\..*9.*IN.*A" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# wait for the CNAME to be stale; A will still be valid and in prefetch window.
+# (the longer TTL is needed, otherwise data won't be prefetch-eligible.)
+sleep 7
+
+# re-enable auth responses, but with a delay answering the A
+n=$((n+1))
+echo_i "delay responses from authoritative server ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.2 txt slowdown  > dig.out.test$n
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "TXT.\"1\"" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
+# resend the query and wait in the background; we should get a stale answer
+n=$((n+1))
+echo_i "check prefetch processing of a stale CNAME target ($n)"
+ret=0
+$DIG -p ${PORT} @10.53.0.3 cname.example A > dig.out.test$n &
+sleep 2
+wait
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 2," dig.out.test$n > /dev/null || ret=1
+grep "cname\.example\..*7.*IN.*CNAME.*target\.example\." dig.out.test$n > /dev/null || ret=1
+grep "target\.example\..*[1-2].*IN.*A" dig.out.test$n > /dev/null || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
 echo_i "exit status: $status"
 [ $status -eq 0 ] || exit 1

doc/notes/notes-current.rst

@@ -65,3 +65,7 @@ Bug Fixes
 - If a query was answered with stale data on a server with DNS64 enabled,
   an assertion could occur if a non-stale answer arrived afterward. This
   has been fixed. :gl:`#2731`
+
+- Fixed an assertion failure that could occur if stale data was used
+  to answer a query, and then a prefetch was triggered after the query
+  was restarted (for example, to follow a CNAME). :gl:`#2733`

lib/ns/query.c

@@ -6118,6 +6118,7 @@ fetch_callback(isc_task_t *task, isc_event_t *event) {
 	if (client->view->cachedb != NULL && client->view->recursion) {
 		client->query.attributes |= NS_QUERYATTR_RECURSIONOK;
 	}
+	client->query.fetchoptions &= ~DNS_FETCHOPT_TRYSTALE_ONTIMEOUT;
 	client->query.dboptions &= ~DNS_DBFIND_STALETIMEOUT;
 	client->nodetach = false;