Fix datarace when UDP/TCP connect fails and we are in nmthread

When we were in nmthread, the isc__nm_async_<proto>connect() function
executes in the same thread as the isc__nm_<proto>connect() and on a
failure, it would block indefinitely because the failure branch was
setting sock->active to false before the condition around the wait had a
chance to skip the WAIT().

This also fixes the zero system test being stuck on FreeBSD 11, so we
re-enable the test in the commit.

diff --git a/bin/tests/system/zero/prereq.sh b/bin/tests/system/zero/prereq.sh
deleted file mode 100755 (executable)
index 55d18ac..0000000
--- a/bin/tests/system/zero/prereq.sh
+++ /dev/null
@@ -1,19 +0,0 @@
-#!/bin/sh
-#
-# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
-#
-# This Source Code Form is subject to the terms of the Mozilla Public
-# License, v. 2.0. If a copy of the MPL was not distributed with this
-# file, you can obtain one at https://mozilla.org/MPL/2.0/.
-#
-# See the COPYRIGHT file distributed with this work for additional
-# information regarding copyright ownership.
-
-. ../conf.sh
-
-if [ "$(uname -s)" != "Linux" ]; then
-    echo_i "This test is currently broken on non-Linux platforms"
-    exit 255
-fi
-
-exit 0

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index 02b276c87469805c126eb9ec4749d61be255c8b2..78bcb54521df12c10b77c5687b95a2919300e022 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -158,6 +158,7 @@ failed_connect_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req,
 static isc_result_t
 tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        isc__networker_t *worker = NULL;
+       isc_result_t result = ISC_R_DEFAULT;
        int r;
 
        REQUIRE(VALID_NMSOCK(sock));
@@ -182,7 +183,7 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        if (r != 0) {
                isc__nm_closesocket(sock->fd);
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -191,7 +192,7 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
                if (r != 0) {
                        isc__nm_incstats(sock->mgr,
                                         sock->statsindex[STATID_BINDFAIL]);
-                       goto failure;
+                       goto done;
                }
        }
 
@@ -201,20 +202,25 @@ tcp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        if (r != 0) {
                isc__nm_incstats(sock->mgr,
                                 sock->statsindex[STATID_CONNECTFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
 
        atomic_store(&sock->connected, true);
 
-       return (ISC_R_SUCCESS);
-
-failure:
-       atomic_store(&sock->active, false);
+done:
+       result = isc__nm_uverr2result(r);
 
-       isc__nm_tcp_close(sock);
+       LOCK(&sock->lock);
+       sock->result = result;
+       SIGNAL(&sock->cond);
+       if (!atomic_load(&sock->active)) {
+               WAIT(&sock->scond, &sock->lock);
+       }
+       INSIST(atomic_load(&sock->active));
+       UNLOCK(&sock->lock);
 
-       return (isc__nm_uverr2result(r));
+       return (result);
 }
 
 void
@@ -234,22 +240,12 @@ isc__nm_async_tcpconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
        REQUIRE(sock->tid == isc_nm_tid());
 
        result = tcp_connect_direct(sock, req);
-       if (result == ISC_R_SUCCESS) {
-               atomic_store(&sock->connected, true);
-               /* The connect cb will be executed in tcp_connect_cb() */
-       } else {
+       if (result != ISC_R_SUCCESS) {
+               atomic_store(&sock->active, false);
+               isc__nm_tcp_close(sock);
                isc__nm_uvreq_put(&req, sock);
        }
 
-       LOCK(&sock->lock);
-       sock->result = result;
-       SIGNAL(&sock->cond);
-       if (!atomic_load(&sock->active)) {
-               WAIT(&sock->scond, &sock->lock);
-       }
-       INSIST(atomic_load(&sock->active));
-       UNLOCK(&sock->lock);
-
        /*
         * The sock is now attached to the handle.
         */
@@ -334,7 +330,6 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
        sock = isc_mem_get(mgr->mctx, sizeof(*sock));
        isc__nmsocket_init(sock, mgr, isc_nm_tcpsocket, local);
 
-       atomic_init(&sock->active, false);
        sock->extrahandlesize = extrahandlesize;
        sock->connect_timeout = timeout;
        sock->result = ISC_R_DEFAULT;
@@ -360,6 +355,7 @@ isc_nm_tcpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
                                         (isc__netievent_t *)ievent);
                isc__nm_put_netievent_tcpconnect(mgr, ievent);
        } else {
+               atomic_init(&sock->active, false);
                sock->tid = isc_random_uniform(mgr->nworkers);
                isc__nm_enqueue_ievent(&mgr->workers[sock->tid],
                                       (isc__netievent_t *)ievent);
@@ -534,7 +530,7 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
        if (r < 0) {
                isc__nm_closesocket(sock->fd);
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -547,7 +543,7 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                                &sock->iface->addr.type.sa, flags);
        if (r < 0) {
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
+               goto done;
        }
 #else
        if (sock->parent->fd == -1) {
@@ -556,7 +552,7 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                if (r < 0) {
                        isc__nm_incstats(sock->mgr,
                                         sock->statsindex[STATID_BINDFAIL]);
-                       goto failure;
+                       goto done;
                }
                sock->parent->uv_handle.tcp.flags = sock->uv_handle.tcp.flags;
                sock->parent->fd = sock->fd;
@@ -578,12 +574,12 @@ isc__nm_async_tcplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                              "uv_listen failed: %s",
                              isc_result_totext(isc__nm_uverr2result(r)));
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
+               goto done;
        }
 
        atomic_store(&sock->listening, true);
 
-failure:
+done:
        result = isc__nm_uverr2result(r);
        if (result != ISC_R_SUCCESS) {
                sock->pquota = NULL;

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index dcd24316ca0dce4faddf5a2380ed1493d3bb8df9..edcbff6539c6fb82b7253b241faa18ff9edafe78 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -197,6 +197,7 @@ failed_connect_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req,
 static isc_result_t
 tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        isc__networker_t *worker = NULL;
+       isc_result_t result = ISC_R_DEFAULT;
        int r;
 
        REQUIRE(VALID_NMSOCK(sock));
@@ -221,7 +222,7 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        if (r != 0) {
                isc__nm_closesocket(sock->fd);
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -234,7 +235,7 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
                if (r != 0 && r != UV_EINVAL) {
                        isc__nm_incstats(sock->mgr,
                                         sock->statsindex[STATID_BINDFAIL]);
-                       goto failure;
+                       goto done;
                }
        }
 
@@ -244,20 +245,25 @@ tcpdns_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        if (r != 0) {
                isc__nm_incstats(sock->mgr,
                                 sock->statsindex[STATID_CONNECTFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
 
        atomic_store(&sock->connected, true);
 
-       return (ISC_R_SUCCESS);
-
-failure:
-       atomic_store(&sock->active, false);
+done:
+       result = isc__nm_uverr2result(r);
 
-       isc__nm_tcpdns_close(sock);
+       LOCK(&sock->lock);
+       sock->result = result;
+       SIGNAL(&sock->cond);
+       if (!atomic_load(&sock->active)) {
+               WAIT(&sock->scond, &sock->lock);
+       }
+       INSIST(atomic_load(&sock->active));
+       UNLOCK(&sock->lock);
 
-       return (isc__nm_uverr2result(r));
+       return (result);
 }
 
 void
@@ -277,22 +283,12 @@ isc__nm_async_tcpdnsconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
        REQUIRE(sock->tid == isc_nm_tid());
 
        result = tcpdns_connect_direct(sock, req);
-       if (result == ISC_R_SUCCESS) {
-               atomic_store(&sock->connected, true);
-               /* The connect cb will be executed in tcpdns_connect_cb() */
-       } else {
+       if (result != ISC_R_SUCCESS) {
+               atomic_store(&sock->active, false);
+               isc__nm_tcpdns_close(sock);
                isc__nm_uvreq_put(&req, sock);
        }
 
-       LOCK(&sock->lock);
-       sock->result = result;
-       SIGNAL(&sock->cond);
-       if (!atomic_load(&sock->active)) {
-               WAIT(&sock->scond, &sock->lock);
-       }
-       INSIST(atomic_load(&sock->active));
-       UNLOCK(&sock->lock);
-
        /*
         * The sock is now attached to the handle.
         */
@@ -377,7 +373,6 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
        sock = isc_mem_get(mgr->mctx, sizeof(*sock));
        isc__nmsocket_init(sock, mgr, isc_nm_tcpdnssocket, local);
 
-       atomic_init(&sock->active, false);
        sock->extrahandlesize = extrahandlesize;
        sock->connect_timeout = timeout;
        sock->result = ISC_R_DEFAULT;
@@ -403,6 +398,7 @@ isc_nm_tcpdnsconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
                                            (isc__netievent_t *)ievent);
                isc__nm_put_netievent_tcpdnsconnect(mgr, ievent);
        } else {
+               atomic_init(&sock->active, false);
                sock->tid = isc_random_uniform(mgr->nworkers);
                isc__nm_enqueue_ievent(&mgr->workers[sock->tid],
                                       (isc__netievent_t *)ievent);
@@ -579,7 +575,7 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
        if (r < 0) {
                isc__nm_closesocket(sock->fd);
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -592,7 +588,7 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                                &sock->iface->addr.type.sa, flags);
        if (r < 0) {
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
+               goto done;
        }
 #else
        if (sock->parent->fd == -1) {
@@ -601,7 +597,7 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                if (r < 0) {
                        isc__nm_incstats(sock->mgr,
                                         sock->statsindex[STATID_BINDFAIL]);
-                       goto failure;
+                       goto done;
                }
                sock->parent->uv_handle.tcp.flags = sock->uv_handle.tcp.flags;
                sock->parent->fd = sock->fd;
@@ -623,12 +619,12 @@ isc__nm_async_tcpdnslisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                              "uv_listen failed: %s",
                              isc_result_totext(isc__nm_uverr2result(r)));
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
+               goto done;
        }
 
        atomic_store(&sock->listening, true);
 
-failure:
+done:
        result = isc__nm_uverr2result(r);
        if (result != ISC_R_SUCCESS) {
                sock->pquota = NULL;

diff --git a/lib/isc/netmgr/udp.c b/lib/isc/netmgr/udp.c
index 22f96d135d51551200916cda906bf47b327f0b3d..85469a4a1cbcf2033c8f8ec708fd60431962ea8f 100644 (file)
--- a/lib/isc/netmgr/udp.c
+++ b/lib/isc/netmgr/udp.c
@@ -261,7 +261,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
        if (r < 0) {
                isc__nm_closesocket(sock->fd);
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -275,7 +275,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                                uv_bind_flags);
        if (r < 0) {
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
+               goto done;
        }
 #else
        if (sock->parent->fd == -1) {
@@ -286,7 +286,7 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
                if (r < 0) {
                        isc__nm_incstats(sock->mgr,
                                         sock->statsindex[STATID_BINDFAIL]);
-                       goto failure;
+                       goto done;
                }
                sock->parent->uv_handle.udp.flags = sock->uv_handle.udp.flags;
                sock->parent->fd = sock->fd;
@@ -307,12 +307,12 @@ isc__nm_async_udplisten(isc__networker_t *worker, isc__netievent_t *ev0) {
        r = uv_udp_recv_start(&sock->uv_handle.udp, udp_alloc_cb, udp_recv_cb);
        if (r != 0) {
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
+               goto done;
        }
 
        atomic_store(&sock->listening, true);
 
-failure:
+done:
        result = isc__nm_uverr2result(r);
        sock->parent->rchildren += 1;
        if (sock->parent->result == ISC_R_DEFAULT) {
@@ -641,6 +641,7 @@ static isc_result_t
 udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        isc__networker_t *worker = NULL;
        int uv_bind_flags = UV_UDP_REUSEADDR;
+       isc_result_t result = ISC_R_DEFAULT;
        int r;
 
        REQUIRE(isc__nm_in_netthread());
@@ -662,7 +663,7 @@ udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
        if (r != 0) {
                isc__nm_closesocket(sock->fd);
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPENFAIL]);
-               goto failure;
+               goto done;
        }
        isc__nm_incstats(sock->mgr, sock->statsindex[STATID_OPEN]);
 
@@ -674,17 +675,8 @@ udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
                        uv_bind_flags);
        if (r != 0) {
                isc__nm_incstats(sock->mgr, sock->statsindex[STATID_BINDFAIL]);
-               goto failure;
-       }
-
-       r = isc_uv_udp_connect(&sock->uv_handle.udp, &req->peer.type.sa);
-       if (r != 0) {
-               isc__nm_incstats(sock->mgr,
-                                sock->statsindex[STATID_CONNECTFAIL]);
-               goto failure;
+               goto done;
        }
-       isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
-       atomic_store(&sock->connecting, false);
 
 #ifdef ISC_RECV_BUFFER_SIZE
        uv_recv_buffer_size(&sock->uv_handle.handle,
@@ -695,16 +687,30 @@ udp_connect_direct(isc_nmsocket_t *sock, isc__nm_uvreq_t *req) {
                            &(int){ ISC_SEND_BUFFER_SIZE });
 #endif
 
-       atomic_store(&sock->connected, true);
+       r = isc_uv_udp_connect(&sock->uv_handle.udp, &req->peer.type.sa);
+       if (r != 0) {
+               isc__nm_incstats(sock->mgr,
+                                sock->statsindex[STATID_CONNECTFAIL]);
+               goto done;
+       }
+       isc__nm_incstats(sock->mgr, sock->statsindex[STATID_CONNECT]);
 
-       return (ISC_R_SUCCESS);
+       atomic_store(&sock->connecting, false);
+       atomic_store(&sock->connected, true);
 
-failure:
-       atomic_store(&sock->active, false);
+done:
+       result = isc__nm_uverr2result(r);
 
-       isc__nm_udp_close(sock);
+       LOCK(&sock->lock);
+       sock->result = result;
+       SIGNAL(&sock->cond);
+       if (!atomic_load(&sock->active)) {
+               WAIT(&sock->scond, &sock->lock);
+       }
+       INSIST(atomic_load(&sock->active));
+       UNLOCK(&sock->lock);
 
-       return (isc__nm_uverr2result(r));
+       return (result);
 }
 
 /*
@@ -730,23 +736,14 @@ isc__nm_async_udpconnect(isc__networker_t *worker, isc__netievent_t *ev0) {
        req->handle = isc__nmhandle_get(sock, &req->peer, &sock->iface->addr);
        result = udp_connect_direct(sock, req);
        if (result != ISC_R_SUCCESS) {
+               atomic_store(&sock->active, false);
+               isc__nm_udp_close(sock);
                isc__nm_uvreq_put(&req, sock);
-       }
-
-       LOCK(&sock->lock);
-       sock->result = result;
-       SIGNAL(&sock->cond);
-       if (!atomic_load(&sock->active)) {
-               WAIT(&sock->scond, &sock->lock);
-       }
-       INSIST(atomic_load(&sock->active));
-       UNLOCK(&sock->lock);
-
-       /*
-        * The callback has to be called after the socket has been
-        * initialized
-        */
-       if (result == ISC_R_SUCCESS) {
+       } else {
+               /*
+                * The callback has to be called after the socket has been
+                * initialized
+                */
                isc__nm_connectcb(sock, req, ISC_R_SUCCESS);
        }
 
@@ -785,7 +782,6 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
        sock = isc_mem_get(mgr->mctx, sizeof(isc_nmsocket_t));
        isc__nmsocket_init(sock, mgr, isc_nm_udpsocket, local);
 
-       atomic_init(&sock->active, false);
        sock->connect_cb = cb;
        sock->connect_cbarg = cbarg;
        sock->read_timeout = timeout;
@@ -822,6 +818,7 @@ isc_nm_udpconnect(isc_nm_t *mgr, isc_nmiface_t *local, isc_nmiface_t *peer,
                                         (isc__netievent_t *)event);
                isc__nm_put_netievent_udpconnect(mgr, event);
        } else {
+               atomic_init(&sock->active, false);
                sock->tid = isc_random_uniform(mgr->nworkers);
                isc__nm_enqueue_ievent(&mgr->workers[sock->tid],
                                       (isc__netievent_t *)event);

diff --git a/util/copyrights b/util/copyrights
index bebc950682e11b12c1b1bb87dfc9a094c1dbbd86..71420be2765e416a2a1d486a7217e1fcf578b100 100644 (file)
--- a/util/copyrights
+++ b/util/copyrights
@@ -969,7 +969,6 @@
 ./bin/tests/system/xferquota/tests.sh          SH      2000,2001,2004,2007,2012,2016,2018,2019,2020
 ./bin/tests/system/zero/ans5/ans.pl            PERL    2016,2018,2019,2020
 ./bin/tests/system/zero/clean.sh               SH      2013,2014,2015,2016,2018,2019,2020
-./bin/tests/system/zero/prereq.sh              SH      2020
 ./bin/tests/system/zero/setup.sh               SH      2013,2014,2016,2018,2019,2020
 ./bin/tests/system/zero/tests.sh               SH      2013,2016,2017,2018,2019,2020
 ./bin/tests/system/zonechecks/clean.sh         SH      2004,2007,2012,2014,2015,2016,2018,2019,2020