diff --git a/CHANGES b/CHANGES index 8d26051d1f..b786f8a39d 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +5777. [bug] TCP connections could hang after receiving + non-matching responses. [GL #3042] + 5776. [bug] Add a missing isc_condition_destroy() for nmsocket condition variable and add missing isc_mutex_destroy() for nmworker lock. [GL #3051] diff --git a/bin/tests/system/resolver/ans8/ans.pl b/bin/tests/system/resolver/ans8/ans.pl index 753fc65f65..0364e13870 100644 --- a/bin/tests/system/resolver/ans8/ans.pl +++ b/bin/tests/system/resolver/ans8/ans.pl @@ -86,8 +86,11 @@ sub handleUDP { # versions just get it completely wrong. if ($qname eq "truncated.no-questions") { - # QR, AA, TC + # QR, AA, TC: forces TCP retry return (pack("nnnnnn", $id, 0x8600, 0, 0, 0, 0)); + } elsif ($qname eq "tcpalso.no-questions") { + # QR, REFUSED: forces TCP retry + return (pack("nnnnnn", $id, 0x8205, 0, 0, 0, 0)); } # QR, AA return (pack("nnnnnn", $id, 0x8400, 0, 0, 0, 0)); @@ -118,9 +121,15 @@ sub handleTCP { $response->header->qr(1); $response->header->aa(1); $response->header->id($id); - $response->push("answer", new Net::DNS::RR("$qname 300 A 1.2.3.4")); - push(@results, $response->data); + + if ($qname eq "tcpalso.no-questions") { + # for this qname we also return a bad reply over TCP + # QR, REFUSED, no question section + push (@results, pack("nnnnnn", $id, 0x8005, 0, 0, 0, 0)); + } else { + push(@results, $response->data); + } return \@results; } diff --git a/bin/tests/system/resolver/clean.sh b/bin/tests/system/resolver/clean.sh index 1d32bc122e..2a3b2e555c 100644 --- a/bin/tests/system/resolver/clean.sh +++ b/bin/tests/system/resolver/clean.sh @@ -21,6 +21,7 @@ rm -f dig.out dig.out.* dig.*.out.* rm -f dig.*.foo.* rm -f dig.*.bar.* rm -f dig.*.prime.* +rm -f nextpart.out.* rm -f ns4/tld.db rm -f ns6/K* rm -f ns6/example.net.db.signed ns6/example.net.db diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index 4f2cdc4907..e2a43bdf48 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -822,6 +822,27 @@ grep "1\.2\.3\.4" dig.ns5.out.${n} > /dev/null && ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=`expr $status + $ret` +n=`expr $n + 1` +echo_i "check that SERVFAIL is returned for an empty question section via TCP ($n)" +ret=0 +nextpart ns5/named.run > /dev/null +# bind to local address so that addresses in log messages are consistent +# between platforms; use tcp to get SERVFAIL rather than timeout on slow +# machines +$DIG $DIGOPTS @10.53.0.5 -b 10.53.0.5 +tcp tcpalso.no-questions. a +tries=3 +time=4 > dig.ns5.out.${n} || ret=1 +grep "status: SERVFAIL" dig.ns5.out.${n} > /dev/null || ret=1 +check_namedrun() { +nextpartpeek ns5/named.run > nextpart.out.${n} +grep 'resolving tcpalso.no-questions/A for [^:]*: empty question section, accepting it anyway as TC=1' nextpart.out.${n} > /dev/null || return 1 +grep '(tcpalso.no-questions/A): connecting via TCP' nextpart.out.${n} > /dev/null || return 1 +grep 'resolving tcpalso.no-questions/A for [^:]*: empty question section$' nextpart.out.${n} > /dev/null || return 1 +grep '(tcpalso.no-questions/A): nextitem' nextpart.out.${n} > /dev/null || return 1 +return 0 +} +retry_quiet 12 check_namedrun || ret=1 +if [ $ret != 0 ]; then echo_i "failed"; fi +status=`expr $status + $ret` + n=`expr $n + 1` echo_i "checking SERVFAIL is returned when all authoritative servers return FORMERR ($n)" ret=0 diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index d5cbd07dcf..27f221c9f7 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -58,7 +58,6 @@ struct dns_dispatchmgr { /* Locked by "lock". */ isc_mutex_t lock; - unsigned int state; ISC_LIST(dns_dispatch_t) list; dns_qid_t *qid; @@ -69,9 +68,6 @@ struct dns_dispatchmgr { unsigned int nv6ports; /*%< # of available ports for IPv4 */ }; -#define MGR_SHUTTINGDOWN 0x00000001U -#define MGR_IS_SHUTTINGDOWN(l) (((l)->state & MGR_SHUTTINGDOWN) != 0) - struct dns_dispentry { unsigned int magic; isc_refcount_t references; @@ -124,7 +120,8 @@ struct dns_dispatch { /* Locked by "lock". */ isc_mutex_t lock; /*%< locks all below */ isc_socktype_t socktype; - atomic_uint_fast32_t state; + atomic_uint_fast32_t tcpstate; + atomic_bool tcpreading; isc_refcount_t references; unsigned int shutdown_out : 1; @@ -752,6 +749,8 @@ tcp_recv(isc_nmhandle_t *handle, isc_result_t result, isc_region_t *region, REQUIRE(VALID_DISPATCH(disp)); + atomic_store(&disp->tcpreading, false); + qid = disp->mgr->qid; ISC_LIST_INIT(resps); @@ -996,7 +995,6 @@ dispatchmgr_destroy(dns_dispatchmgr_t *mgr) { mgr->magic = 0; isc_mutex_destroy(&mgr->lock); - mgr->state = 0; qid_destroy(mgr->mctx, &mgr->qid); @@ -1210,7 +1208,7 @@ dns_dispatch_gettcp(dns_dispatchmgr_t *mgr, const isc_sockaddr_t *destaddr, (localaddr == NULL || isc_sockaddr_eqaddr(localaddr, &sockname))) { - if (atomic_load(&disp->state) == + if (atomic_load(&disp->tcpstate) == DNS_DISPATCHSTATE_CONNECTED) { /* We found connected dispatch */ disp_connected = disp; @@ -1531,11 +1529,14 @@ dispatch_getnext(dns_dispatch_t *disp, dns_dispentry_t *resp, int32_t timeout) { break; case isc_socktype_tcp: - dns_dispatch_attach(disp, &(dns_dispatch_t *){ NULL }); - if (timeout > 0) { - isc_nmhandle_settimeout(disp->handle, timeout); + if (atomic_compare_exchange_strong(&disp->tcpreading, + &(bool){ false }, true)) { + dns_dispatch_attach(disp, &(dns_dispatch_t *){ NULL }); + if (timeout > 0) { + isc_nmhandle_settimeout(disp->handle, timeout); + } + isc_nm_read(disp->handle, tcp_recv, disp); } - isc_nm_read(disp->handle, tcp_recv, disp); break; default: @@ -1694,7 +1695,7 @@ startrecv(isc_nmhandle_t *handle, dns_dispatch_t *disp, dns_dispentry_t *resp) { LOCK(&disp->lock); REQUIRE(disp->handle == NULL); REQUIRE(atomic_compare_exchange_strong( - &disp->state, + &disp->tcpstate, &(uint_fast32_t){ DNS_DISPATCHSTATE_CONNECTING }, DNS_DISPATCHSTATE_CONNECTED)); @@ -1722,10 +1723,6 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { ISC_LIST_INIT(resps); - if (MGR_IS_SHUTTINGDOWN(disp->mgr)) { - eresult = ISC_R_SHUTTINGDOWN; - } - if (eresult == ISC_R_SUCCESS) { startrecv(handle, disp, NULL); } @@ -1763,10 +1760,6 @@ udp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { dispatch_log(disp, LVL(90), "UDP connected (%p): %s", resp, isc_result_totext(eresult)); - if (MGR_IS_SHUTTINGDOWN(disp->mgr)) { - eresult = ISC_R_SHUTTINGDOWN; - } - if (eresult == ISC_R_SUCCESS && resp->canceled) { eresult = ISC_R_CANCELED; } else if (eresult == ISC_R_SUCCESS) { @@ -1795,7 +1788,7 @@ detach: isc_result_t dns_dispatch_connect(dns_dispentry_t *resp) { dns_dispatch_t *disp = NULL; - uint_fast32_t state = DNS_DISPATCHSTATE_NONE; + uint_fast32_t tcpstate = DNS_DISPATCHSTATE_NONE; REQUIRE(VALID_RESPONSE(resp)); @@ -1810,10 +1803,10 @@ dns_dispatch_connect(dns_dispentry_t *resp) { * Check whether the dispatch is already connecting * or connected. */ - atomic_compare_exchange_strong(&disp->state, - (uint_fast32_t *)&state, + atomic_compare_exchange_strong(&disp->tcpstate, + (uint_fast32_t *)&tcpstate, DNS_DISPATCHSTATE_CONNECTING); - switch (state) { + switch (tcpstate) { case DNS_DISPATCHSTATE_NONE: /* First connection, continue with connecting */ LOCK(&disp->lock); diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 7772d1cbac..5c78c410e2 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -7579,7 +7579,7 @@ resquery_response(isc_result_t eresult, isc_region_t *region, void *arg) { default: result = same_question(fctx, query->rmessage); if (result != ISC_R_SUCCESS) { - FCTXTRACE3("response did not match question", result); + FCTXTRACE3("question section invalid", result); rctx.nextitem = true; rctx_done(&rctx, result); return;