Fix nonsensical stale TTL values in cache dump

When introducing change 5149, "rndc dumpdb" started to print a line
above a stale RRset, indicating how long the data will be retained.

At that time, I thought it should also be possible to load
a cache from file. But if a TTL has a value of 0 (because it is stale),
stale entries wouldn't be loaded from file. So, I added the
'max-stale-ttl' to TTL values, and adjusted the $DATE accordingly.

Since we actually don't have a "load cache from file" feature, this
is premature and is causing confusion at operators. This commit
changes the 'max-stale-ttl' adjustments.

A check in the serve-stale system test is added for a non-stale
RRset (longttl.example) to make sure the TTL in cache is sensible.

Also, the comment above stale RRsets could have nonsensical
values. A possible reason why this may happen is when the RRset was
marked a stale but the 'max-stale-ttl' has passed (and is actually an
RRset awaiting cleanup). This would lead to the "will be retained"
value to be negative (but since it is stored in an uint32_t, you would
get a nonsensical value (e.g. 4294362497).

To mitigate against this, we now also check if the header is not
ancient. In addition we check if the stale_ttl would be negative, and
if so we set it to 0. Most likely this will not happen because the
header would already have been marked ancient, but there is a possible
race condition where the 'rdh_ttl + serve_stale_ttl' has passed,
but the header has not been checked for staleness.

diff --git a/CHANGES b/CHANGES
index a7acefc77da9cb5ecb2eb01b7c0d140ece952bd5..51eb7c80e86e3f5172972b039adf6545bb0c32bc 100644 (file)
--- a/CHANGES
+++ b/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]

diff --git a/bin/tests/system/serve-stale/tests.sh b/bin/tests/system/serve-stale/tests.sh
index 2205962c7d918f57f12ed1eeffe2f8a8d6249d7e..f16aaf08445306c3f22f8035e204136272c50e4d 100755 (executable)
--- a/bin/tests/system/serve-stale/tests.sh
+++ b/bin/tests/system/serve-stale/tests.sh
@@ -107,9 +107,10 @@ sleep 2
 
 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
 
@@ -135,6 +136,15 @@ awk '/; answer/ { x=$0; getline; print x, $0}' ns1/named_dump.db.test$n |
 if [ $ret != 0 ]; then echo_i "failed"; fi
 status=$((status+ret))
 
+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))
+
 n=$((n+1))
 echo_i "check stale othertype.example ($n)"
 ret=0

diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst
index cd3af2cb7b5fc72612c96b2ca0cd0ef75550f5f4..4098e8ea0afa2843aa890476fa210b046c56066d 100644 (file)
--- a/doc/notes/notes-current.rst
+++ b/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]

diff --git a/lib/dns/masterdump.c b/lib/dns/masterdump.c
index edd4fcaaa00a817131dc7d0e0bae91d79bb7e5ff..8a4eb4de473b997cf8d21a192e31f89e5a39d5f8 100644 (file)
--- a/lib/dns/masterdump.c
+++ b/lib/dns/masterdump.c
@@ -1111,8 +1111,7 @@ again:
                                fprintf(f,
                                        "; stale (will be retained for %u more "
                                        "seconds)\n",
-                                       (rds->stale_ttl -
-                                        ctx->serve_stale_ttl));
+                                       rds->stale_ttl);
                        } else if (ANCIENT(rds)) {
                                isc_buffer_t b;
                                char buf[sizeof("YYYYMMDDHHMMSS")];
@@ -1591,14 +1590,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 &&

diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c
index a18fcceaa53a14f60091cd0f96ef8fd663375bd4..8af4e10e48bceae3d9689f14d84f23a124a0636c 100644 (file)
--- a/lib/dns/rbtdb.c
+++ b/lib/dns/rbtdb.c
@@ -3123,13 +3123,18 @@ 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(header) && !ANCIENT(header)) {
+               dns_ttl_t stale = header->rdh_ttl + rbtdb->serve_stale_ttl;
+               if (stale > now) {
+                       stale = stale - now;
+               } else {
+                       stale = 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->stale_ttl = stale;
                rdataset->ttl = 0;
        } else if (IS_CACHE(rbtdb) && !ACTIVE(header, now)) {
                rdataset->attributes |= DNS_RDATASETATTR_ANCIENT;