From bd7463914fe6375e3e9157f305c60d0172f2b312 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 31 Jan 2024 12:59:19 +0000 Subject: [PATCH 1/3] Disallow stale-answer-client-timeout non-zero values Remove all the code and tests which support non-zero stale-answer-client-timeout values, and adjust the documentation. --- bin/named/server.c | 36 +-- .../system/fetchlimit/ns5/named2.conf.in | 2 +- .../system/serve-stale/ns3/named.conf.in | 2 +- .../system/serve-stale/ns3/named2.conf.in | 3 +- .../system/serve-stale/ns3/named8.conf.in | 2 +- bin/tests/system/serve-stale/tests.sh | 279 +----------------- doc/arm/reference.rst | 10 +- lib/dns/include/dns/resolver.h | 11 +- lib/dns/resolver.c | 236 +-------------- lib/dns/validator.c | 4 - lib/dns/zone.c | 4 +- lib/ns/include/ns/query.h | 1 - lib/ns/query.c | 101 +------ 13 files changed, 56 insertions(+), 635 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 0366a855cc..498c11508e 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -4542,6 +4542,19 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, view->staleanswerclienttimeout = (uint32_t)-1; } else { view->staleanswerclienttimeout = cfg_obj_asuint32(obj); + + /* + * BIND 9 no longer supports non-zero values of + * stale-answer-client-timeout. + */ + if (view->staleanswerclienttimeout != 0) { + view->staleanswerclienttimeout = 0; + isc_log_write( + named_g_lctx, NAMED_LOGCATEGORY_GENERAL, + NAMED_LOGMODULE_SERVER, ISC_LOG_WARNING, + "BIND 9 no longer supports non-zero values of " + "stale-answer-client-timeout, adjusted to 0"); + } } obj = NULL; @@ -4806,27 +4819,6 @@ configure_view(dns_view_t *view, dns_viewlist_t *viewlist, cfg_obj_t *config, query_timeout = cfg_obj_asuint32(obj); dns_resolver_settimeout(view->resolver, query_timeout); - /* - * Adjust stale-answer-client-timeout upper bound - * to be resolver-query-timeout - 1s. - * This assignment is safe as dns_resolver_settimeout() - * ensures that resolver->querytimeout value will be in the - * [MINIMUM_QUERY_TIMEOUT, MAXIMUM_QUERY_TIMEOUT] range and - * MINIMUM_QUERY_TIMEOUT is > 1000 (in ms). - */ - if (view->staleanswerclienttimeout != (uint32_t)-1 && - view->staleanswerclienttimeout > - (dns_resolver_gettimeout(view->resolver) - 1000)) - { - view->staleanswerclienttimeout = - dns_resolver_gettimeout(view->resolver) - 1000; - isc_log_write( - named_g_lctx, NAMED_LOGCATEGORY_GENERAL, - NAMED_LOGMODULE_SERVER, ISC_LOG_WARNING, - "stale-answer-client-timeout adjusted to %" PRIu32, - view->staleanswerclienttimeout); - } - /* Specify whether to use 0-TTL for negative response for SOA query */ dns_resolver_setzeronosoattl(view->resolver, zero_no_soattl); @@ -7059,7 +7051,7 @@ tat_done(void *arg) { dns_fetchresponse_t *resp = (dns_fetchresponse_t *)arg; ns_tat_t *tat = NULL; - INSIST(resp != NULL && resp->type == FETCHDONE); + INSIST(resp != NULL); tat = resp->arg; diff --git a/bin/tests/system/fetchlimit/ns5/named2.conf.in b/bin/tests/system/fetchlimit/ns5/named2.conf.in index ba5ef64009..32b281d79a 100644 --- a/bin/tests/system/fetchlimit/ns5/named2.conf.in +++ b/bin/tests/system/fetchlimit/ns5/named2.conf.in @@ -25,7 +25,7 @@ options { notify yes; stale-answer-enable yes; stale-cache-enable yes; - stale-answer-client-timeout 1800; + stale-answer-client-timeout 0; clients-per-query 5; max-clients-per-query 10; }; diff --git a/bin/tests/system/serve-stale/ns3/named.conf.in b/bin/tests/system/serve-stale/ns3/named.conf.in index d5f475126d..16b3e04212 100644 --- a/bin/tests/system/serve-stale/ns3/named.conf.in +++ b/bin/tests/system/serve-stale/ns3/named.conf.in @@ -35,7 +35,7 @@ options { stale-answer-enable yes; stale-cache-enable yes; stale-refresh-time 30; - stale-answer-client-timeout 1800; + stale-answer-client-timeout 0; max-cache-ttl 24h; }; diff --git a/bin/tests/system/serve-stale/ns3/named2.conf.in b/bin/tests/system/serve-stale/ns3/named2.conf.in index 0a316d844c..51a70a89d6 100644 --- a/bin/tests/system/serve-stale/ns3/named2.conf.in +++ b/bin/tests/system/serve-stale/ns3/named2.conf.in @@ -38,8 +38,7 @@ options { stale-cache-enable yes; stale-answer-ttl 3; stale-refresh-time 0; - stale-answer-client-timeout 1800; # 1.8 seconds - recursive-clients 10; # CVE-2022-3924 + stale-answer-client-timeout 0; max-stale-ttl 3600; resolver-query-timeout 30000; # 30 seconds qname-minimization disabled; diff --git a/bin/tests/system/serve-stale/ns3/named8.conf.in b/bin/tests/system/serve-stale/ns3/named8.conf.in index 7b5792d12f..a46804158f 100644 --- a/bin/tests/system/serve-stale/ns3/named8.conf.in +++ b/bin/tests/system/serve-stale/ns3/named8.conf.in @@ -32,7 +32,7 @@ options { dnssec-validation no; stale-answer-enable yes; stale-cache-enable yes; - stale-answer-client-timeout 1800; + stale-answer-client-timeout 0; prefetch 2 8; dns64 2001:aaaa::/96 { clients { any; }; diff --git a/bin/tests/system/serve-stale/tests.sh b/bin/tests/system/serve-stale/tests.sh index c6f2dd47a9..507081b02e 100755 --- a/bin/tests/system/serve-stale/tests.sh +++ b/bin/tests/system/serve-stale/tests.sh @@ -1678,208 +1678,6 @@ grep -F "#!TXT" ns5/named.stats.$n.cachedb >/dev/null && ret=1 status=$((status + ret)) if [ $ret != 0 ]; then echo_i "failed"; fi -################################################ -# Test for stale-answer-client-timeout (1.8s). # -################################################ -echo_i "test stale-answer-client-timeout (1.8)" - -n=$((n + 1)) -echo_i "updating ns3/named.conf ($n)" -ret=0 -copy_setports ns3/named2.conf.in ns3/named.conf -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -echo_i "restart ns3" -stop_server --use-rndc --port ${CONTROLPORT} ns3 -start_server --noclean --restart --port ${PORT} ns3 - -n=$((n + 1)) -echo_i "check 'rndc serve-stale status' ($n)" -ret=0 -$RNDCCMD 10.53.0.3 serve-stale status >rndc.out.test$n 2>&1 || ret=1 -grep '_default: stale cache enabled; stale answers enabled (stale-answer-ttl=3 max-stale-ttl=3600 stale-refresh-time=0)' rndc.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -n=$((n + 1)) -echo_i "enable responses from authoritative server ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.2 txt enable >dig.out.test$n || ret=1 -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)) - -n=$((n + 1)) -echo_i "prime cache data.example TXT (stale-answer-client-timeout) ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.3 data.example TXT >dig.out.test$n || ret=1 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "ANSWER: 1," dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -n=$((n + 1)) -echo_i "prime cache nodata.example TXT (stale-answer-client-timeout) ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.3 nodata.example TXT >dig.out.test$n || ret=1 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "ANSWER: 0," dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -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 || ret=1 -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)) - -n=$((n + 1)) -echo_i "prime cache data.slow TXT (stale-answer-client-timeout) ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.3 data.slow TXT >dig.out.test$n || ret=1 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "ANSWER: 1," dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -n=$((n + 1)) -echo_i "disable responses from authoritative server ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.2 txt disable >dig.out.test$n || ret=1 -grep "ANSWER: 1," dig.out.test$n >/dev/null || ret=1 -grep "TXT.\"0\"" dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -# Allow RRset to become stale. -sleep 2 - -nextpart ns3/named.run >/dev/null - -echo_i "sending queries for tests $((n + 1))-$((n + 3))..." -t1=$($PERL -e 'print time()') -$DIG -p ${PORT} +tries=1 +timeout=11 @10.53.0.3 data.example TXT >dig.out.test$((n + 1)) & -$DIG -p ${PORT} +tries=1 +timeout=11 @10.53.0.3 nodata.example TXT >dig.out.test$((n + 2)) & -$DIG -p ${PORT} +tries=1 +timeout=11 @10.53.0.3 data.slow TXT >dig.out.test$((n + 3)) & -wait -t2=$($PERL -e 'print time()') - -# We configured a long value of 30 seconds for resolver-query-timeout. -# That should give us enough time to receive an stale answer from cache -# after stale-answer-client-timeout timer of 1.8 sec triggers. -n=$((n + 1)) -echo_i "check stale data.example TXT comes from cache (stale-answer-client-timeout 1.8) ($n)" -ret=0 -wait_for_log 5 "data.example TXT client timeout, stale answer used" ns3/named.run || ret=1 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "EDE: 3 (Stale Answer): (client timeout)" dig.out.test$n >/dev/null || ret=1 -grep "ANSWER: 1," dig.out.test$n >/dev/null || ret=1 -grep "data\.example\..*3.*IN.*TXT.*A text record with a 2 second ttl" dig.out.test$n >/dev/null || ret=1 -# Configured stale-answer-client-timeout is 1.8s, we allow some extra time -# just in case other tests are taking too much cpu. -[ $((t2 - t1)) -le 10 ] || { - echo_i "query took $((t2 - t1))s to resolve." - ret=1 -} -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -n=$((n + 1)) -echo_i "check stale nodata.example TXT comes from cache (stale-answer-client-timeout 1.8) ($n)" -ret=0 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "EDE: 3 (Stale Answer): (client timeout)" dig.out.test$n >/dev/null || ret=1 -grep "ANSWER: 0," dig.out.test$n >/dev/null || ret=1 -grep "example\..*3.*IN.*SOA" 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 data.slow TXT comes from cache (stale-answer-client-timeout 1.8) ($n)" -ret=0 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "EDE: 3 (Stale Answer): (client timeout)" dig.out.test$n >/dev/null || ret=1 -grep "ANSWER: 1," dig.out.test$n >/dev/null || ret=1 -grep "data\.slow\..*3.*IN.*TXT.*A slow text record with a 2 second ttl" dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -# Now query for RRset not in cache. The first query should time out, but once -# we enable the authoritative server, the second query should be able to get a -# response. - -nextpart ns3/named.run >/dev/null - -echo_i "sending queries for tests $((n + 2))-$((n + 4))..." -# first dig runs in background for 10 seconds, second in background for 3 -# seconds and the last for 3 seconds in the foreground. -# the second RRSIG lookup triggers the issue in [GL #3622] -$DIG -p ${PORT} +tries=1 +timeout=10 @10.53.0.3 longttl.example TXT >dig.out.test$((n + 3)) & -$DIG -p ${PORT} +tries=1 +timeout=3 @10.53.0.3 longttl.example RRSIG >dig.out.test$((n + 4)) & -$DIG -p ${PORT} +tries=1 +timeout=3 @10.53.0.3 longttl.example TXT >dig.out.test$((n + 2)) || true - -# Enable the authoritative name server after stale-answer-client-timeout. -n=$((n + 1)) -echo_i "enable responses from authoritative server ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.2 txt enable >dig.out.test$n || ret=1 -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)) - -n=$((n + 1)) -echo_i "check not in cache longttl.example TXT times out (stale-answer-client-timeout 1.8) ($n)" -ret=0 -wait_for_log 4 "longttl.example TXT client timeout, stale answer unavailable" ns3/named.run || ret=1 -grep "timed out" dig.out.test$n >/dev/null || ret=1 -grep ";; no servers could be reached" dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -wait - -n=$((n + 1)) -echo_i "check not in cache longttl.example TXT comes from authoritative (stale-answer-client-timeout 1.8) ($n)" -ret=0 -grep "status: NOERROR" dig.out.test$n >/dev/null || ret=1 -grep "EDE" dig.out.test$n >/dev/null && ret=1 -grep "ANSWER: 1," 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 not in cache longttl.example RRSIG times out (stale-answer-client-timeout 1.8) ($n)" -ret=0 -grep "timed out" dig.out.test$n >/dev/null || ret=1 -grep ";; no servers could be reached" dig.out.test$n >/dev/null || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - -# CVE-2022-3924, GL #3619 -n=$((n + 1)) -echo_i "check that named survives reaching recursive-clients quota (stale-answer-client-timeout 1.8) ($n)" -ret=0 -num=0 -# Make sure to exceed the configured value of 'recursive-clients 10;' by running -# 20 parallel queries with simulated network latency. -while [ $num -lt 20 ]; do - $DIG +tries=1 -p ${PORT} @10.53.0.3 "latency${num}.data.example" TXT >/dev/null 2>&1 & - num=$((num + 1)) -done -check_server_responds() { - $DIG -p ${PORT} @10.53.0.3 version.bind txt ch >dig.out.test$n || return 1 - grep "status: NOERROR" dig.out.test$n >/dev/null || return 1 -} -retry_quiet 5 check_server_responds || ret=1 -if [ $ret != 0 ]; then echo_i "failed"; fi -status=$((status + ret)) - ############################################# # Test for stale-answer-client-timeout off. # ############################################# @@ -1928,6 +1726,11 @@ grep "data\.example\..*[12].*IN.*TXT.*A text record with a 2 second ttl" dig.out if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) +check_server_responds() { + $DIG -p ${PORT} @10.53.0.3 version.bind txt ch >dig.out.test$n || return 1 + grep "status: NOERROR" dig.out.test$n >/dev/null || return 1 +} + ############################################################## # Test for stale-answer-client-timeout off and CNAME record. # ############################################################## @@ -2666,77 +2469,5 @@ 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 . -# 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 A (stale-answer-client-timeout 1.8) ($n)" -ret=0 -$DIG -p ${PORT} @10.53.0.3 cname.example A >dig.out.test$n || ret=1 -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 || ret=1 -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 diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 9814afa862..e0a821c7ab 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -2368,11 +2368,13 @@ Boolean Options ``off`` or ``disabled``. It also has no effect if :any:`stale-answer-enable` is disabled. - The maximum value for this option is :any:`resolver-query-timeout` minus - one second. The minimum value, ``0``, causes a cached (stale) RRset to be + The minimum value, ``0``, causes a cached (stale) RRset to be immediately returned if it is available while still attempting to - refresh the data in cache. :rfc:`8767` recommends a value of ``1800`` - (milliseconds). + refresh the data in cache. + + When this option is enabled, the only supported value in the current version + of BIND 9 is ``0``. Non-zero values generate a warning message, and are + treated as ``0``. .. namedconf:statement:: stale-cache-enable :tags: server, query diff --git a/lib/dns/include/dns/resolver.h b/lib/dns/include/dns/resolver.h index 7f0dde65a5..90770fd8f0 100644 --- a/lib/dns/include/dns/resolver.h +++ b/lib/dns/include/dns/resolver.h @@ -93,7 +93,6 @@ struct dns_fetchresponse { isc_loop_t *loop; isc_job_cb cb; void *arg; - enum { FETCHDONE, TRYSTALE } type; ISC_LINK(dns_fetchresponse_t) link; }; @@ -130,7 +129,6 @@ enum { * on ip6.arpa. */ DNS_FETCHOPT_NOFORWARD = 1 << 15, /*%< Do not use forwarders if * possible. */ - DNS_FETCHOPT_TRYSTALE_ONTIMEOUT = 1 << 16, /*% EDNS version bits: */ DNS_FETCHOPT_EDNSVERSIONSET = 1 << 23, @@ -291,11 +289,10 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, * we figure out how selective forwarding will work. * *\li When the fetch completes (successfully or otherwise), a - * dns_fetchresponse_t option is sent to callback 'cb' with - * 'type' set to FETCHDONE. + * dns_fetchresponse_t option is sent to callback 'cb'. * *\li The values of 'rdataset' and 'sigrdataset' will be returned in - * the FETCHDONE event. + * the fetch completion event. * *\li 'client' and 'id' are used for duplicate query detection. '*client' * must remain stable until after 'action' has been called or @@ -342,7 +339,7 @@ dns_resolver_cancelfetch(dns_fetch_t *fetch); * * Notes: * - *\li If 'fetch' has not completed, post its FETCHDONE event with a + *\li If 'fetch' has not completed, post its completion event with a * result code of #ISC_R_CANCELED. * * Requires: @@ -359,7 +356,7 @@ dns_resolver_destroyfetch(dns_fetch_t **fetchp); * *\li '*fetchp' is a valid fetch. * - *\li The caller has received the FETCHDONE event (either because the + *\li The caller has received the fetch completion event (either because the * fetch completed or because dns_resolver_cancelfetch() was called). * * Ensures: diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 442dfabce0..0395587ed1 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -300,7 +300,7 @@ struct tried { typedef enum { fetchstate_active, - fetchstate_done /*%< FETCHDONE events posted. */ + fetchstate_done /*%< Fetch completion events posted. */ } fetchstate_t; typedef enum { @@ -361,7 +361,6 @@ struct fetchctx { atomic_uint_fast32_t attributes; isc_timer_t *timer; isc_time_t expires; - isc_time_t expires_try_stale; isc_time_t next_timeout; isc_interval_t interval; dns_message_t *qmessage; @@ -1565,24 +1564,7 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result) { next = ISC_LIST_NEXT(resp, link); ISC_LIST_UNLINK(fctx->resps, resp, link); - /* - * Only the regular fetch events should be counted for the - * clients-per-query limit, in case if there are multiple events - * registered for a single client. - */ - if (resp->type == FETCHDONE) { - count++; - } - - if (resp->type == TRYSTALE) { - /* - * Not applicable to TRYSTALE resps; this function is - * called when the fetch has either completed or timed - * out due to resolver-query-timeout being reached. - */ - isc_mem_putanddetach(&resp->mctx, resp, sizeof(*resp)); - continue; - } + count++; resp->vresult = fctx->vresult; if (!HAVE_ANSWER(fctx)) { @@ -1863,17 +1845,9 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) { } /* - * But don't wait past the stale timeout (if any), the final - * expiration of the fetch, or for more than 10 seconds total. + * But don't wait past the the final expiration of the fetch, + * or for more than 10 seconds total. */ - if ((fctx->options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) != 0) { - uint64_t stale = isc_time_microdiff(&fctx->expires_try_stale, - &now); - if (stale >= US_PER_MS && us > stale) { - FCTXTRACE("setting stale timeout"); - us = stale; - } - } if (us > limit) { us = limit; } @@ -1887,63 +1861,6 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) { isc_time_nowplusinterval(&fctx->next_timeout, &fctx->interval); } -static isc_result_t -resquery_timeout(resquery_t *query) { - fetchctx_t *fctx = query->fctx; - dns_fetchresponse_t *resp = NULL, *next = NULL; - uint64_t timeleft; - isc_time_t now; - - FCTXTRACE("timeout"); - - /* - * If not configured for serve-stale, do nothing. - */ - if ((fctx->options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) == 0) { - return (ISC_R_SUCCESS); - } - - /* - * If we haven't reached the serve-stale timeout, do nothing. - * (Note that netmgr timeouts have millisecond accuracy, so - * anything less than 1000 microseconds is close enough to zero.) - */ - now = isc_time_now(); - timeleft = isc_time_microdiff(&fctx->expires_try_stale, &now); - if (timeleft >= US_PER_MS) { - return (ISC_R_SUCCESS); - } - - /* - * Send the TRYSTALE events. - */ - LOCK(&fctx->lock); - for (resp = ISC_LIST_HEAD(fctx->resps); resp != NULL; resp = next) { - next = ISC_LIST_NEXT(resp, link); - if (resp->type != TRYSTALE) { - continue; - } - - ISC_LIST_UNLINK(fctx->resps, resp, link); - resp->vresult = ISC_R_TIMEDOUT; - resp->result = ISC_R_TIMEDOUT; - isc_async_run(resp->loop, resp->cb, resp); - } - UNLOCK(&fctx->lock); - - /* - * If the next timeout is more than 1ms in the future, - * resume waiting. - */ - timeleft = isc_time_microdiff(&fctx->next_timeout, &now); - if (timeleft >= US_PER_MS) { - dns_dispatch_resume(query->dispentry, (timeleft / US_PER_MS)); - return (ISC_R_COMPLETE); - } - - return (ISC_R_SUCCESS); -} - static isc_result_t fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, unsigned int options) { @@ -4067,13 +3984,8 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) { */ if (fctx->minimized && !fctx->forwarding) { unsigned int options = fctx->options; - /* - * Also clear DNS_FETCHOPT_TRYSTALE_ONTIMEOUT here, - * otherwise every query minimization step will activate - * the try-stale timer again. - */ - options &= ~(DNS_FETCHOPT_QMINIMIZE | - DNS_FETCHOPT_TRYSTALE_ONTIMEOUT); + + options &= ~DNS_FETCHOPT_QMINIMIZE; /* * Is another QNAME minimization fetch still running? @@ -4161,7 +4073,6 @@ resume_qmin(void *arg) { dns_fixedname_t ffixed, dcfixed; REQUIRE(VALID_FCTX(fctx)); - REQUIRE(resp->type == FETCHDONE); res = fctx->res; @@ -4438,7 +4349,7 @@ static void fctx_add_event(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, dns_messageid_t id, isc_job_cb cb, void *arg, dns_rdataset_t *rdataset, dns_rdataset_t *sigrdataset, - dns_fetch_t *fetch, int type) { + dns_fetch_t *fetch) { dns_fetchresponse_t *resp = NULL; FCTXTRACE("addevent"); @@ -4452,7 +4363,6 @@ fctx_add_event(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, .fetch = fetch, .client = client, .id = id, - .type = type, .loop = loop, .cb = cb, .arg = arg, @@ -4483,7 +4393,7 @@ fctx_join(fetchctx_t *fctx, isc_loop_t *loop, const isc_sockaddr_t *client, REQUIRE(!SHUTTINGDOWN(fctx)); fctx_add_event(fctx, loop, client, id, cb, arg, rdataset, sigrdataset, - fetch, FETCHDONE); + fetch); fetch->magic = DNS_FETCH_MAGIC; fetchctx_attach(fctx, &fetch->private); @@ -4726,27 +4636,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, */ isc_interval_set(&fctx->interval, 2, 0); - /* - * If stale answers are enabled, compute an expiration time - * after which stale data will be served, if the target RRset is - * available in cache. - */ - if ((options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) != 0) { - INSIST(res->view->staleanswerclienttimeout <= - (res->query_timeout - 1000)); - isc_interval_set( - &interval, res->view->staleanswerclienttimeout / 1000, - res->view->staleanswerclienttimeout % 1000 * 1000000); - iresult = isc_time_nowplusinterval(&fctx->expires_try_stale, - &interval); - if (iresult != ISC_R_SUCCESS) { - UNEXPECTED_ERROR("isc_time_nowplusinterval: %s", - isc_result_totext(iresult)); - result = ISC_R_UNEXPECTED; - goto cleanup_timer; - } - } - /* * Attach to the view's cache and adb. */ @@ -4779,9 +4668,6 @@ fctx_create(dns_resolver_t *res, isc_loop_t *loop, const dns_name_t *name, return (ISC_R_SUCCESS); -cleanup_timer: - isc_timer_destroy(&fctx->timer); - cleanup_qmessage: dns_message_detach(&fctx->qmessage); @@ -4985,25 +4871,6 @@ clone_results(fetchctx_t *fctx) { continue; } - if (resp->type == TRYSTALE) { - /* - * We don't need to clone resulting data to this - * type of event, as its associated callback is - * only called when stale-answer-client-timeout - * triggers, and the logic in there doesn't - * expect any result as input, as it will itself - * lookup for stale data in cache to use as - * result, if any is available. - * - * Also, if we reached this point, then the - * whole fetch context is done, it will cancel - * timers, process associated callbacks of type - * FETCHDONE, and silently remove/free events of - * type TRYSTALE. - */ - continue; - } - resp->result = hresp->result; dns_name_copy(hresp->foundname, resp->foundname); dns_db_attach(hresp->db, &resp->db); @@ -7060,7 +6927,6 @@ resume_dslookup(void *arg) { unsigned int n; dns_fetch_t *fetch = NULL; - REQUIRE(resp->type == FETCHDONE); REQUIRE(VALID_FCTX(fctx)); res = fctx->res; @@ -7308,7 +7174,6 @@ betterreferral(respctx_t *rctx) { } /* - * resquery_response(): * Handles responses received in response to iterative queries sent by * resquery_send(). Sets up a response context (respctx_t). */ @@ -7330,13 +7195,6 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { QTRACE("response"); - if (eresult == ISC_R_TIMEDOUT) { - result = resquery_timeout(query); - if (result == ISC_R_COMPLETE) { - return; - } - } - if (isc_sockaddr_pf(&query->addrinfo->sockaddr) == PF_INET) { inc_stats(fctx->res, dns_resstatscounter_responsev4); } else { @@ -10051,7 +9909,6 @@ prime_done(void *arg) { dns_fetch_t *fetch = NULL; dns_db_t *db = NULL; - REQUIRE(resp->type == FETCHDONE); REQUIRE(VALID_RESOLVER(res)); isc_log_write(dns_lctx, DNS_LOGCATEGORY_RESOLVER, @@ -10474,15 +10331,7 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, goto unlock; } - /* - * Only the regular fetch events should be - * counted for the clients-per-query limit, in - * case if there are multiple events registered - * for a single client. - */ - if (resp->type == FETCHDONE) { - count++; - } + count++; } } if (count >= spillatmin && spillatmin != 0) { @@ -10511,11 +10360,6 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, fctx_join(fctx, loop, client, id, cb, arg, rdataset, sigrdataset, fetch); - if ((options & DNS_FETCHOPT_TRYSTALE_ONTIMEOUT) != 0) { - fctx_add_event(fctx, loop, client, id, cb, arg, NULL, NULL, - fetch, TRYSTALE); - } - if (new_fctx) { fetchctx_ref(fctx); isc_async_run(fctx->loop, fctx_start, fctx); @@ -10543,8 +10387,6 @@ fail: void dns_resolver_cancelfetch(dns_fetch_t *fetch) { fetchctx_t *fctx = NULL; - dns_fetchresponse_t *trystale = NULL; - dns_fetchresponse_t *fetchdone = NULL; REQUIRE(DNS_FETCH_VALID(fetch)); fctx = fetch->private; @@ -10555,11 +10397,9 @@ dns_resolver_cancelfetch(dns_fetch_t *fetch) { LOCK(&fctx->lock); /* - * Find completion events associated with this fetch (as opposed - * to those for other fetches that have joined the same fctx). - * The event(s) found are only sent and removed from the fctx->resps - * list after this loop is finished (i.e. the fctx->resps list is not - * modified during iteration). + * Find the completion event associated with this fetch (as opposed + * to those for other fetches that have joined the same fctx) and run + * the callback asynchronously with a ISC_R_CANCELED result. */ if (fctx->state != fetchstate_done) { dns_fetchresponse_t *next = NULL; @@ -10568,59 +10408,15 @@ dns_resolver_cancelfetch(dns_fetch_t *fetch) { { next = ISC_LIST_NEXT(resp, link); - /* - * Only process events associated with the provided - * 'fetch'. - */ - if (resp->fetch != fetch) { - continue; - } - - /* - * Track various events associated with the - * provided 'fetch' in separate variables as they - * will need to be sent in a specific order. - */ - switch (resp->type) { - case TRYSTALE: - INSIST(trystale == NULL); - trystale = resp; - break; - case FETCHDONE: - INSIST(fetchdone == NULL); - fetchdone = resp; - break; - default: - UNREACHABLE(); - } - - /* - * Break out of the loop once all possible types of - * events associated with the provided 'fetch' are - * found. - */ - if (trystale != NULL && fetchdone != NULL) { + if (resp->fetch == fetch) { + resp->result = ISC_R_CANCELED; + ISC_LIST_UNLINK(fctx->resps, resp, link); + isc_async_run(resp->loop, resp->cb, resp); break; } } } - /* - * The "trystale" event must be sent before the "fetchdone" event, - * because the latter clears the "recursing" query attribute, which is - * required by both events (handled by the same callback function). - */ - if (trystale != NULL) { - trystale->result = ISC_R_CANCELED; - ISC_LIST_UNLINK(fctx->resps, trystale, link); - isc_async_run(trystale->loop, trystale->cb, trystale); - } - if (fetchdone != NULL) { - fetchdone->result = ISC_R_CANCELED; - ISC_LIST_UNLINK(fctx->resps, fetchdone, link); - isc_async_run(fetchdone->loop, fetchdone->cb, fetchdone); - } - /* * The fctx continues running even if no fetches remain; * the answer is still cached. diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 82b22b8d96..38d0b62e5a 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -384,8 +384,6 @@ fetch_callback_dnskey(void *arg) { isc_result_t eresult = resp->result; isc_result_t result; - INSIST(resp->type == FETCHDONE); - /* Free resources which are not of interest. */ if (resp->node != NULL) { dns_db_detachnode(resp->db, &resp->node); @@ -455,8 +453,6 @@ fetch_callback_ds(void *arg) { isc_result_t result; bool trustchain; - INSIST(resp->type == FETCHDONE); - /* * Set 'trustchain' to true if we're walking a chain of * trust; false if we're attempting to prove insecurity. diff --git a/lib/dns/zone.c b/lib/dns/zone.c index 918c481e54..3dbf5ff06d 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -10118,7 +10118,7 @@ keyfetch_done(void *arg) { dns_rdataset_t *dnskeys = NULL, *dnskeysigs = NULL; dns_rdataset_t *keydataset = NULL, dsset; - INSIST(resp != NULL && resp->type == FETCHDONE); + INSIST(resp != NULL); kfetch = resp->arg; @@ -21409,7 +21409,7 @@ nsfetch_done(void *arg) { dns_rdataset_t *nsrrset = NULL; dns_rdataset_t *nssigset = NULL; - INSIST(resp != NULL && resp->type == FETCHDONE); + INSIST(resp != NULL); nsfetch = resp->arg; diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index 96bcbf9b39..f6d0712c93 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -165,7 +165,6 @@ struct ns_query { #define NS_QUERYATTR_REDIRECT 0x020000 #define NS_QUERYATTR_ANSWERED 0x040000 #define NS_QUERYATTR_STALEOK 0x080000 -#define NS_QUERYATTR_STALEPENDING 0x100000 typedef struct query_ctx query_ctx_t; diff --git a/lib/ns/query.c b/lib/ns/query.c index 0404a78847..4bc57e04c0 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -144,10 +144,6 @@ /*% Was the client already sent a response? */ #define QUERY_ANSWERED(q) (((q)->attributes & NS_QUERYATTR_ANSWERED) != 0) -/*% Have we already processed an answer via stale-answer-client-timeout? */ -#define QUERY_STALEPENDING(q) \ - (((q)->attributes & NS_QUERYATTR_STALEPENDING) != 0) - /*% Does the query allow stale data in the response? */ #define QUERY_STALEOK(q) (((q)->attributes & NS_QUERYATTR_STALEOK) != 0) @@ -2622,7 +2618,6 @@ cleanup_after_fetch(dns_fetchresponse_t *resp, const char *ctracestr, dns_fetch_t **fetchp = NULL; isc_result_t result; - REQUIRE(resp->type == FETCHDONE); REQUIRE(NS_CLIENT_VALID(client)); CTRACE(ISC_LOG_DEBUG(3), ctracestr); @@ -5808,7 +5803,7 @@ ns__query_start(query_ctx_t *qctx) { } } - if (!qctx->is_zone && (qctx->view->staleanswerclienttimeout == 0) && + if (!qctx->is_zone && qctx->view->staleanswerclienttimeout == 0 && dns_view_staleanswerenabled(qctx->view)) { /* @@ -6012,13 +6007,8 @@ query_lookup(query_ctx_t *qctx) { * If DNS_DBFIND_STALETIMEOUT is set, a stale answer is requested. * This can happen if 'stale-answer-client-timeout' is enabled. * - * If 'stale-answer-client-timeout' is set to 0, and a stale - * answer is found, send it to the client, and try to refresh the - * RRset. - * - * If 'stale-answer-client-timeout' is non-zero, and a stale - * answer is found, send it to the client. Don't try to refresh the - * RRset because a fetch is already in progress. + * If a stale answer is found, send it to the client, and try to refresh + * the RRset. */ stale_timeout = ((dboptions & DNS_DBFIND_STALETIMEOUT) != 0); @@ -6140,35 +6130,7 @@ query_lookup(query_ctx_t *qctx) { } } } else { - /* - * The 'stale-answer-client-timeout' triggered, return - * the stale answer if available, otherwise wait until - * the resolver finishes. - */ - isc_log_write( - ns_lctx, NS_LOGCATEGORY_SERVE_STALE, - NS_LOGMODULE_QUERY, ISC_LOG_INFO, - "%s %s client timeout, stale answer %s (%s)", - namebuf, typebuf, - stale_found ? "used" : "unavailable", - isc_result_totext(result)); - if (stale_found) { - ns_client_extendederror(qctx->client, ede, - "client timeout"); - } else if (!answer_found) { - return (result); - } - - if (!stale_client_answer(result)) { - return (result); - } - - /* - * There still might be real answer later. Mark the - * query so we'll know we can skip answering. - */ - qctx->client->query.attributes |= - NS_QUERYATTR_STALEPENDING; + UNREACHABLE(); } } @@ -6242,36 +6204,6 @@ query_clear_stale(ns_client_t *client) { message_clearrdataset(client->message, DNS_RDATASETATTR_STALE_ADDED); } -/* - * Create a new query context with the sole intent of looking up for a stale - * RRset in cache. If an entry is found, we mark the original query as - * answered, in order to avoid answering the query twice, when the original - * fetch finishes. - */ -static void -query_lookup_stale(ns_client_t *client) { - query_ctx_t qctx; - - qctx_init(client, NULL, client->query.qtype, &qctx); - if (DNS64(client)) { - qctx.qtype = qctx.type = dns_rdatatype_a; - qctx.dns64 = true; - } - if (DNS64EXCLUDE(client)) { - qctx.dns64_exclude = true; - } - dns_db_attach(client->view->cachedb, &qctx.db); - client->query.attributes &= ~NS_QUERYATTR_RECURSIONOK; - client->query.dboptions |= DNS_DBFIND_STALETIMEOUT; - client->nodetach = true; - (void)query_lookup(&qctx); - if (qctx.node != NULL) { - dns_db_detachnode(qctx.db, &qctx.node); - } - qctx_freedata(&qctx); - qctx_destroy(&qctx); -} - /* * Event handler to resume processing a query after recursion, or when a * client timeout is triggered. If the query has timed out or been cancelled @@ -6296,13 +6228,6 @@ fetch_callback(void *arg) { CTRACE(ISC_LOG_DEBUG(3), "fetch_callback"); - if (resp->type == TRYSTALE) { - if (resp->result != ISC_R_CANCELED) { - query_lookup_stale(client); - } - isc_mem_putanddetach(&resp->mctx, resp, sizeof(*resp)); - return; - } /* * We are resuming from recursion. Reset any attributes, options * that a lookup due to stale-answer-client-timeout may have set. @@ -6310,22 +6235,13 @@ fetch_callback(void *arg) { 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; LOCK(&client->query.fetchlock); INSIST(FETCH_RECTYPE_NORMAL(client) == resp->fetch || FETCH_RECTYPE_NORMAL(client) == NULL); - if (QUERY_STALEPENDING(&client->query)) { - /* - * We've gotten an authoritative answer to a query that - * was left pending after a stale timeout. We don't need - * to do anything with it; free all the data and go home. - */ - FETCH_RECTYPE_NORMAL(client) = NULL; - fetch_answered = true; - } else if (FETCH_RECTYPE_NORMAL(client) != NULL) { + if (FETCH_RECTYPE_NORMAL(client) != NULL) { /* * This is the fetch we've been waiting for. */ @@ -6567,13 +6483,6 @@ ns_query_recurse(ns_client_t *client, dns_rdatatype_t qtype, dns_name_t *qname, peeraddr = &client->peeraddr; } - if (client->view->staleanswerclienttimeout > 0 && - client->view->staleanswerclienttimeout != (uint32_t)-1 && - dns_view_staleanswerenabled(client->view)) - { - client->query.fetchoptions |= DNS_FETCHOPT_TRYSTALE_ONTIMEOUT; - } - isc_nmhandle_attach(client->handle, &HANDLE_RECTYPE_NORMAL(client)); result = dns_resolver_createfetch( client->view->resolver, qname, qtype, qdomain, nameservers, From 152c6e22744165591128baeba5f510993b3a6dfe Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 6 Feb 2024 16:14:22 +0000 Subject: [PATCH 2/3] Add CHANGES and release notes for [GL #4447] --- CHANGES | 3 +++ doc/notes/notes-current.rst | 4 +++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGES b/CHANGES index 0bd113d400..eeceab7e30 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6347. [func] Disallow stale-answer-client-timeout non-zero values. + [GL #4447] + 6346. [bug] Cleaned up several minor bugs in the RBTDB dbiterator implementation. [GL !8741] diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index 350decbdf8..246466be7c 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -41,7 +41,9 @@ New Features Removed Features ~~~~~~~~~~~~~~~~ -- None. +- BIND 9 no longer supports non-zero :any:`stale-answer-client-timeout` values, + when the feature is turned on. When using a non-zero value, ``named`` now + generates a warning log message, and treats the value as ``0``. :gl:`#4447` Feature Changes ~~~~~~~~~~~~~~~ From 03b68b8c383bead09e3208e17aa803b692467d41 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Wed, 7 Feb 2024 09:22:55 +0000 Subject: [PATCH 3/3] Address scan-build warnings The warnings (see below) seem to be false-positives. Address them by adding runtime checks. resolver.c:1627:10: warning: Access to field 'tid' results in a dereference of a null pointer (loaded from variable 'fctx') [core.NullDereference] 1627 | REQUIRE(fctx->tid == isc_tid()); | ^~~~~~~~~ ../../lib/isc/include/isc/util.h:332:34: note: expanded from macro 'REQUIRE' 332 | #define REQUIRE(e) ISC_REQUIRE(e) | ^ ../../lib/isc/include/isc/assertions.h:45:11: note: expanded from macro 'ISC_REQUIRE' 45 | ((void)((cond) || \ | ^~~~ resolver.c:10335:6: warning: Access to field 'depth' results in a dereference of a null pointer (loaded from variable 'fctx') [core.NullDereference] 10335 | if (fctx->depth > depth) { | ^~~~~~~~~~~ 2 warnings generated. --- lib/dns/resolver.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 0395587ed1..5f30be8645 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -1641,6 +1641,7 @@ fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func, bool no_response = false; bool age_untried = false; + REQUIRE(fctx != NULL); REQUIRE(fctx->tid == isc_tid()); FCTXTRACE("done"); @@ -10353,6 +10354,8 @@ dns_resolver_createfetch(dns_resolver_t *res, const dns_name_t *name, new_fctx = true; } + RUNTIME_CHECK(fctx != NULL); + if (fctx->depth > depth) { fctx->depth = depth; }