Merge branch '2289-cache-dump-stale-ttl-weird-values' into 'main'

Fix nonsensical stale TTL values in cache dump

Closes #2289

See merge request isc-projects/bind9!4799

CHANGES

@@ -1,3 +1,10 @@
+5618.	[bug]		When introducing change 5149, "rndc dumpdb" started
+			to print a line above a stale RRset, indicating how
+			long the data will be retained. Also, TTLs were
+			increased with 'max-stale-ttl'. This could lead to
+			nonsensical values and both issues have been fixed.
+			[GL #389] [GL #2289]
+
 5617.	[placeholder]
 
 5616.	[placeholder]

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

@@ -105,11 +105,25 @@ status=$((status+ret))
 
 sleep 2
 
+# Run rndc dumpdb, test whether the stale data has correct comment printed.
+# The max-stale-ttl is 3600 seconds, so the comment should say the data is
+# stale for somewhere between 3500-3599 seconds.
+echo_i "check rndc dump stale data.example ($n)"
+rndc_dumpdb ns1 || ret=1
+awk '/; stale/ { x=$0; getline; print x, $0}' ns1/named_dump.db.test$n |
+    grep "; stale data\.example.*3[56]...*TXT.*A text record with a 2 second ttl" > /dev/null 2>&1 || ret=1
+# Also make sure the not expired data does not have a stale comment.
+awk '/; answer/ { x=$0; getline; print x, $0}' ns1/named_dump.db.test$n |
+    grep "; answer longttl\.example.*[56]...*TXT.*A text record with a 600 second ttl" > /dev/null 2>&1 || ret=1
+if [ $ret != 0 ]; then echo_i "failed"; fi
+status=$((status+ret))
+
 echo_i "sending queries for tests $((n+1))-$((n+4))..."
 $DIG -p ${PORT} @10.53.0.1 data.example TXT > dig.out.test$((n+1)) &
-$DIG -p ${PORT} @10.53.0.1 othertype.example CAA > dig.out.test$((n+2)) &
-$DIG -p ${PORT} @10.53.0.1 nodata.example TXT > dig.out.test$((n+3)) &
-$DIG -p ${PORT} @10.53.0.1 nxdomain.example TXT > dig.out.test$((n+4))
+$DIG -p ${PORT} @10.53.0.1 longttl.example TXT > dig.out.test$((n+2)) &
+$DIG -p ${PORT} @10.53.0.1 othertype.example CAA > dig.out.test$((n+3)) &
+$DIG -p ${PORT} @10.53.0.1 nodata.example TXT > dig.out.test$((n+4)) &
+$DIG -p ${PORT} @10.53.0.1 nxdomain.example TXT > dig.out.test$((n+5))
 
 wait
 
@@ -122,16 +136,12 @@ grep "data\.example\..*4.*IN.*TXT.*A text record with a 2 second ttl" dig.out.te
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
-# Run rndc dumpdb, test whether the stale data has correct comment printed.
-# The max-stale-ttl is 3600 seconds, so the comment should say the data is
-# stale for somewhere between 3500-3599 seconds.
-echo_i "check rndc dump stale data.example ($n)"
-rndc_dumpdb ns1 || ret=1
-awk '/; stale/ { x=$0; getline; print x, $0}' ns1/named_dump.db.test$n |
-    grep "; stale (will be retained for 35.. more seconds) data\.example.*A text record with a 2 second ttl" > /dev/null 2>&1 || ret=1
-# Also make sure the not expired data does not have a stale comment.
-awk '/; answer/ { x=$0; getline; print x, $0}' ns1/named_dump.db.test$n |
-    grep "; answer longttl\.example.*A text record with a 600 second ttl" > /dev/null 2>&1 || ret=1
+n=$((n+1))
+echo_i "check non-stale longttl.example ($n)"
+ret=0
+grep "status: NOERROR" dig.out.test$n > /dev/null || ret=1
+grep "ANSWER: 1," dig.out.test$n > /dev/null || ret=1
+grep "longttl\.example\..*59[0-9].*IN.*TXT.*A text record with a 600 second ttl" dig.out.test$n > /dev/null || ret=1
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 

doc/notes/notes-current.rst

@@ -80,3 +80,9 @@ Bug Fixes
   state. (Other zone journal files were not affected.) This has been
   fixed. If a corrupt journal file is detected, ``named`` can now recover
   from it. [GL #2600]
+
+- When dumping the cache to file, TTLs were being increased with
+  ``max-stale-ttl``. Also the comment above stale RRsets could have nonsensical
+  values if the RRset was still marked a stale but the ``max-stale-ttl`` has
+  passed (and is actually an RRset awaiting cleanup). Both issues have now
+  been fixed. [GL #389] [GL #2289]

lib/dns/masterdump.c

@@ -1108,11 +1108,7 @@ again:
 		} else {
 			isc_result_t result;
 			if (STALE(rds)) {
-				fprintf(f,
-					"; stale (will be retained for %u more "
-					"seconds)\n",
-					(rds->stale_ttl -
-					 ctx->serve_stale_ttl));
+				fprintf(f, "; stale\n");
 			} else if (ANCIENT(rds)) {
 				isc_buffer_t b;
 				char buf[sizeof("YYYYMMDDHHMMSS")];
@@ -1591,14 +1587,8 @@ dumpctx_create(isc_mem_t *mctx, dns_db_t *db, dns_dbversion_t *version,
 
 	dctx->do_date = dns_db_iscache(dctx->db);
 	if (dctx->do_date) {
-		/*
-		 * Adjust the date backwards by the serve-stale TTL, if any.
-		 * This is so the TTL will be loaded correctly when next
-		 * started.
-		 */
 		(void)dns_db_getservestalettl(dctx->db,
 					      &dctx->tctx.serve_stale_ttl);
-		dctx->now -= dctx->tctx.serve_stale_ttl;
 	}
 
 	if (dctx->format == dns_masterformat_text &&

lib/dns/rbtdb.c

@@ -3088,6 +3088,8 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header,
 	      isc_stdtime_t now, isc_rwlocktype_t locktype,
 	      dns_rdataset_t *rdataset) {
 	unsigned char *raw; /* RDATASLAB */
+	bool stale = STALE(header);
+	bool ancient = ANCIENT(header);
 
 	/*
 	 * Caller must be holding the node reader lock.
@@ -3105,6 +3107,29 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header,
 
 	INSIST(rdataset->methods == NULL); /* We must be disassociated. */
 
+	/*
+	 * Mark header stale or ancient if the RRset is no longer active.
+	 */
+	if (!ACTIVE(header, now)) {
+		dns_ttl_t stale_ttl = header->rdh_ttl + rbtdb->serve_stale_ttl;
+		/*
+		 * If this data is in the stale window keep it and if
+		 * DNS_DBFIND_STALEOK is not set we tell the caller to
+		 * skip this record.  We skip the records with ZEROTTL
+		 * (these records should not be cached anyway).
+		 */
+
+		if (KEEPSTALE(rbtdb) && stale_ttl > now) {
+			stale = true;
+		} else {
+			/*
+			 * We are not keeping stale, or it is outside the
+			 * stale window. Mark ancient, i.e. ready for cleanup.
+			 */
+			ancient = true;
+		}
+	}
+
 	rdataset->methods = &rdataset_methods;
 	rdataset->rdclass = rbtdb->common.rdclass;
 	rdataset->type = RBTDB_RDATATYPE_BASE(header->type);
@@ -3123,14 +3148,19 @@ bind_rdataset(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node, rdatasetheader_t *header,
 	if (PREFETCH(header)) {
 		rdataset->attributes |= DNS_RDATASETATTR_PREFETCH;
 	}
-	if (STALE(header)) {
+	if (stale && !ancient) {
+		dns_ttl_t stale_ttl = header->rdh_ttl + rbtdb->serve_stale_ttl;
+		if (stale_ttl > now) {
+			stale_ttl = stale_ttl - now;
+		} else {
+			stale_ttl = 0;
+		}
 		if (STALE_WINDOW(header)) {
 			rdataset->attributes |= DNS_RDATASETATTR_STALE_WINDOW;
 		}
 		rdataset->attributes |= DNS_RDATASETATTR_STALE;
-		rdataset->stale_ttl =
-			(rbtdb->serve_stale_ttl + header->rdh_ttl) - now;
-		rdataset->ttl = 0;
+		rdataset->stale_ttl = stale_ttl;
+		rdataset->ttl = stale_ttl;
 	} else if (IS_CACHE(rbtdb) && !ACTIVE(header, now)) {
 		rdataset->attributes |= DNS_RDATASETATTR_ANCIENT;
 		rdataset->stale_ttl = header->rdh_ttl;

lib/dns/tests/db_test.c

@@ -248,7 +248,6 @@ dns_dbfind_staleok_test(void **state) {
 				count++;
 				assert_in_range(count, 0, 49); /* loop sanity */
 				assert_int_equal(result, ISC_R_SUCCESS);
-				assert_int_equal(rdataset.ttl, 0);
 				assert_int_equal(rdataset.attributes &
 							 DNS_RDATASETATTR_STALE,
 						 DNS_RDATASETATTR_STALE);