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.

diff --git a/CHANGES b/CHANGES
index b56d003225399d129ba04fb4b4847ca2c0c90293..4c525c958ad89627e9213cd74f805014a0ba27fa 100644 (file)
--- 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 7bd91db8ce232cf048a1a59ba9982a9c6c55fb1d..9710f8464f2b51e7553d34a94f9617a02c4bce9c 100644 (file)
--- 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, &quota,
                                             &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);
        }