From c449cab63aa3d2ed2422778d2dfdb5a0d8910c30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Wed, 10 Jun 2020 17:51:27 +0200 Subject: [PATCH 1/2] Don't clean quota cb cb_func/data, we don't own it --- lib/isc/quota.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/isc/quota.c b/lib/isc/quota.c index 2f69e86feb..137838828f 100644 --- a/lib/isc/quota.c +++ b/lib/isc/quota.c @@ -119,8 +119,6 @@ quota_release(isc_quota_t *quota) { UNLOCK("a->cblock); if (cb != NULL) { cb->cb_func(quota, cb->data); - cb->cb_func = NULL; - cb->data = NULL; return; } } From 85d8e4bf766f0869a7ec80974fba0f1185b648d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Witold=20Kr=C4=99cicki?= Date: Wed, 10 Jun 2020 16:19:16 +0200 Subject: [PATCH 2/2] Fix a race in TCP accepting. There's a possibility of a race in TCP accepting code: T1 accepts a connection C1 T2 accepts a connection C2 T1 tries to accept a connection C3, but we hit a quota, isc_quota_cb_init() sets quota_accept_cb for the socket, we return from accept_connection T2 drops C2, but we race in quota_release with accepting C3 so we don't see quota->waiting is > 0, we don't launch the callback T1 accepts a connection C4, we are able to get the quota we clear the quota_accept_cb from sock->quotacb T1 drops C1, tries to call the callback which is zeroed, sigsegv. --- CHANGES | 2 ++ lib/isc/netmgr/tcp.c | 12 +++++++----- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGES b/CHANGES index b56d003225..4c525c958a 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +5438. [bug] Fix a race in TCP accepting code. [GL #1930] + 5437. [bug] Fix a data race in resolver log_formerr. [GL #1808] 5436. [placeholder] diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c index 7bd91db8ce..9710f8464f 100644 --- a/lib/isc/netmgr/tcp.c +++ b/lib/isc/netmgr/tcp.c @@ -73,6 +73,9 @@ tcp_listenclose_cb(uv_handle_t *handle); static isc_result_t accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota); +static void +quota_accept_cb(isc_quota_t *quota, void *sock0); + static int tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) { isc__networker_t *worker = NULL; @@ -179,6 +182,7 @@ isc_nm_listentcp(isc_nm_t *mgr, isc_nmiface_t *iface, isc_nm_cb_t cb, */ nsock->pquota = quota; } + isc_quota_cb_init(&nsock->quotacb, quota_accept_cb, nsock); ievent = isc__nm_get_ievent(mgr, netievent_tcplisten); ievent->sock = nsock; @@ -733,12 +737,11 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { * We need to attach to ssock, because it might be queued * waiting for a TCP quota slot. If so, then we'll detach it * later when the connection is accepted. (XXX: This may be - * suboptimal, it might be better to attach unless - * we need to.) + * suboptimal, it might be better not to attach unless + * we need to - but we risk a race then.) */ isc_nmsocket_t *tsock = NULL; isc_nmsocket_attach(ssock, &tsock); - isc_quota_cb_init(&ssock->quotacb, quota_accept_cb, tsock); result = isc_quota_attach_cb(ssock->pquota, "a, &ssock->quotacb); if (result == ISC_R_QUOTA) { @@ -749,9 +752,8 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) { /* * We're under quota, so there's no need to wait; - * clear the quota callback and and detach the socket. + * Detach the socket. */ - isc_quota_cb_init(&ssock->quotacb, NULL, NULL); isc_nmsocket_detach(&tsock); }