From: Matthijs Mekking Date: Wed, 10 Mar 2021 11:09:16 +0000 (+0100) Subject: Fix nonsensical stale TTL values in cache dump X-Git-Tag: v9.17.13~65^2~2 X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=2a5e0232ed56233ae047fe1c31fded0d1ea96d77;p=thirdparty%2Fbind9.git 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 a7acefc77da..51eb7c80e86 100644 --- 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 2205962c7d9..f16aaf08445 100755 --- 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 cd3af2cb7b5..4098e8ea0af 100644 --- 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 edd4fcaaa00..8a4eb4de473 100644 --- 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 a18fcceaa53..8af4e10e48b 100644 --- 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;