mir/bind
2
0
Fork 0
mirror of https://gitlab.isc.org/isc-projects/bind9 synced 2025-08-30 05:57:52 +00:00
Code Issues Releases Wiki Activity

Merge branch '3042-tcp-hang' into 'main'

Browse Source 
prevent a shutdown hang on non-matching TCP responses

Closes #3042

See merge request isc-projects/bind9!5616
This commit is contained in:
Evan Hunt 2021-12-08 18:48:34 +00:00
commit ea0b5dbd5d
6 changed files with 55 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
CHANGES
View File

@ -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]

15
bin/tests/system/resolver/ans8/ans.pl
View File

@ -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;
}

1
bin/tests/system/resolver/clean.sh
View File

@ -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

21
bin/tests/system/resolver/tests.sh
View File

@ -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

41
lib/dns/dispatch.c
View File

@ -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);

2
lib/dns/resolver.c
View File

@ -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;