Fix the data race in accessing the isc_nm_t timers

The following TSAN report about accessing the mgr timers (mgr->init,
mgr->idle, mgr->keepalive and mgr->advertised) has been fixed in this
commit:

    ==================
    WARNING: ThreadSanitizer: data race (pid=2746)
    Read of size 4 at 0x7b440008a948 by thread T18:
    #0 isc__nm_tcpdns_read /home/ondrej/Projects/bind9/lib/isc/netmgr/tcpdns.c:849:25 (libisc.so.1706+0x2ba0f)
    #1 isc_nm_read /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:1679:3 (libisc.so.1706+0x22258)
    #2 tcpdns_connect_connect_cb /home/ondrej/Projects/bind9/lib/isc/tests/tcpdns_test.c:363:2 (tcpdns_test+0x4bc5fb)
    #3 isc__nm_async_connectcb /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:1816:2 (libisc.so.1706+0x228c9)
    #4 isc__nm_connectcb /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:1791:3 (libisc.so.1706+0x22713)
    #5 tcpdns_connect_cb /home/ondrej/Projects/bind9/lib/isc/netmgr/tcpdns.c:343:2 (libisc.so.1706+0x2d89d)
    #6 uv__stream_connect /home/ondrej/Projects/tsan/libuv/src/unix/stream.c:1381:5 (libuv.so.1+0x27c18)
    #7 uv__stream_io /home/ondrej/Projects/tsan/libuv/src/unix/stream.c:1298:5 (libuv.so.1+0x25977)
    #8 uv__io_poll /home/ondrej/Projects/tsan/libuv/src/unix/linux-core.c:462:11 (libuv.so.1+0x2e795)
    #9 uv_run /home/ondrej/Projects/tsan/libuv/src/unix/core.c:385:5 (libuv.so.1+0x158ec)
    #10 nm_thread /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:530:11 (libisc.so.1706+0x1c94a)

    Previous write of size 4 at 0x7b440008a948 by main thread:
    #0 isc_nm_settimeouts /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:490:12 (libisc.so.1706+0x1dda5)
    #1 tcpdns_recv_two /home/ondrej/Projects/bind9/lib/isc/tests/tcpdns_test.c:601:2 (tcpdns_test+0x4bad0e)
    #2 cmocka_run_one_test_or_fixture <null> (libcmocka.so.0+0x70be)
    #3 __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16 (libc.so.6+0x2409a)

    Location is heap block of size 281 at 0x7b440008a840 allocated by main thread:
    #0 malloc <null> (tcpdns_test+0x42864b)
    #1 default_memalloc /home/ondrej/Projects/bind9/lib/isc/mem.c:713:8 (libisc.so.1706+0x6d261)
    #2 mem_get /home/ondrej/Projects/bind9/lib/isc/mem.c:622:8 (libisc.so.1706+0x69b9c)
    #3 isc___mem_get /home/ondrej/Projects/bind9/lib/isc/mem.c:1044:9 (libisc.so.1706+0x6d379)
    #4 isc__mem_get /home/ondrej/Projects/bind9/lib/isc/mem.c:2432:10 (libisc.so.1706+0x6889e)
    #5 isc_nm_start /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:203:8 (libisc.so.1706+0x1c219)
    #6 nm_setup /home/ondrej/Projects/bind9/lib/isc/tests/tcpdns_test.c:244:11 (tcpdns_test+0x4baaa4)
    #7 cmocka_run_one_test_or_fixture <null> (libcmocka.so.0+0x70fd)
    #8 __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16 (libc.so.6+0x2409a)

    Thread T18 'isc-net-0000' (tid=3513, running) created by main thread at:
    #0 pthread_create <null> (tcpdns_test+0x429e7b)
    #1 isc_thread_create /home/ondrej/Projects/bind9/lib/isc/pthreads/thread.c:73:8 (libisc.so.1706+0x8476a)
    #2 isc_nm_start /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:271:3 (libisc.so.1706+0x1c66a)
    #3 nm_setup /home/ondrej/Projects/bind9/lib/isc/tests/tcpdns_test.c:244:11 (tcpdns_test+0x4baaa4)
    #4 cmocka_run_one_test_or_fixture <null> (libcmocka.so.0+0x70fd)
    #5 __libc_start_main /build/glibc-vjB4T1/glibc-2.28/csu/../csu/libc-start.c:308:16 (libc.so.6+0x2409a)

    SUMMARY: ThreadSanitizer: data race /home/ondrej/Projects/bind9/lib/isc/netmgr/tcpdns.c:849:25 in isc__nm_tcpdns_read
    ==================
    ThreadSanitizer: reported 1 warnings

(cherry picked from commit 2e1dd56d0ba70c6434a1afce0a5b785244cafd6a)

diff --git a/lib/isc/Makefile.in b/lib/isc/Makefile.in
index b80cddb066031a7c2946a45114386e737e88749e..a4a44cbf1a2deb28555c7b58ebaeaa896abfa97d 100644 (file)
--- a/lib/isc/Makefile.in
+++ b/lib/isc/Makefile.in
@@ -55,7 +55,7 @@ OBJS =                pk11.@O@ pk11_result.@O@ \
                lex.@O@ lfsr.@O@ lib.@O@ log.@O@ \
                md.@O@ mem.@O@ mutexblock.@O@ \
                netmgr/netmgr.@O@ netmgr/tcp.@O@ netmgr/udp.@O@ \
-               netmgr/tcpdns.@O@ netmgr/tlsdns.@O@ netmgr/uverr2result.@O@ netmgr/uv-compat.@O@ \
+               netmgr/tcpdns.@O@ netmgr/tls.@O@ netmgr/tlsdns.@O@ netmgr/uverr2result.@O@ netmgr/uv-compat.@O@ \
                netaddr.@O@ netscope.@O@ nonce.@O@ openssl_shim.@O@ pool.@O@ \
                parseint.@O@ portset.@O@ queue.@O@ quota.@O@ \
                radix.@O@ random.@O@ ratelimiter.@O@ \

diff --git a/lib/isc/netmgr/Makefile.in b/lib/isc/netmgr/Makefile.in
index 5e6d47f79a21f940d97a434b078c26d319cff8fb..153e80f2f9653dc55b7c7c832642c13656560c6e 100644 (file)
--- a/lib/isc/netmgr/Makefile.in
+++ b/lib/isc/netmgr/Makefile.in
@@ -26,10 +26,10 @@ CDEFINES =
 CWARNINGS =
 
 # Alphabetically
-OBJS =         netmgr.@O@ tcp.@O@ udp.@O@ tcpdns.@O@ tlsdns.@O@ uverr2result.@O@ uv-compat.@O@
+OBJS =         netmgr.@O@ tcp.@O@ udp.@O@ tcpdns.@O@ tls.@O@ tlsdns.@O@ uverr2result.@O@ uv-compat.@O@
 
 # Alphabetically
-SRCS =         netmgr.c tcp.c udp.c tcpdns.c tlsdns.c uverr2result.c uv-compat.c
+SRCS =         netmgr.c tcp.c udp.c tcpdns.c tls.c tlsdns.c uverr2result.c uv-compat.c
 
 TARGETS =      ${OBJS}
 

diff --git a/lib/isc/netmgr/netmgr-int.h b/lib/isc/netmgr/netmgr-int.h
index d21634e6a1251151cf6e14e400b05258618a0e60..7a19af56d17d8eb2da824d02aaf5d489796fafd8 100644 (file)
--- a/lib/isc/netmgr/netmgr-int.h
+++ b/lib/isc/netmgr/netmgr-int.h
@@ -607,10 +607,10 @@ struct isc_nm {
         * milliseconds so they can be used directly with the libuv timer,
         * but they are configured in tenths of seconds.
         */
-       uint32_t init;
-       uint32_t idle;
-       uint32_t keepalive;
-       uint32_t advertised;
+       atomic_uint_fast32_t init;
+       atomic_uint_fast32_t idle;
+       atomic_uint_fast32_t keepalive;
+       atomic_uint_fast32_t advertised;
 
 #ifdef NETMGR_TRACE
        ISC_LIST(isc_nmsocket_t) active_sockets;

diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c
index 8f5c78d01b7de3dcf705d201ccd8b843fe8a82a2..b946ede5bf90cc6fe063d86096c7d7c2b70e2596 100644 (file)
--- a/lib/isc/netmgr/netmgr.c
+++ b/lib/isc/netmgr/netmgr.c
@@ -218,10 +218,10 @@ isc_nm_start(isc_mem_t *mctx, uint32_t workers) {
         * Default TCP timeout values.
         * May be updated by isc_nm_tcptimeouts().
         */
-       mgr->init = 30000;
-       mgr->idle = 30000;
-       mgr->keepalive = 30000;
-       mgr->advertised = 30000;
+       atomic_init(&mgr->init, 30000);
+       atomic_init(&mgr->idle, 30000);
+       atomic_init(&mgr->keepalive, 30000);
+       atomic_init(&mgr->advertised, 30000);
 
        isc_mutex_init(&mgr->reqlock);
        isc_mempool_create(mgr->mctx, sizeof(isc__nm_uvreq_t), &mgr->reqpool);
@@ -486,10 +486,10 @@ isc_nm_settimeouts(isc_nm_t *mgr, uint32_t init, uint32_t idle,
                   uint32_t keepalive, uint32_t advertised) {
        REQUIRE(VALID_NM(mgr));
 
-       mgr->init = init * 100;
-       mgr->idle = idle * 100;
-       mgr->keepalive = keepalive * 100;
-       mgr->advertised = advertised * 100;
+       atomic_store(&mgr->init, init * 100);
+       atomic_store(&mgr->idle, idle * 100);
+       atomic_store(&mgr->keepalive, keepalive * 100);
+       atomic_store(&mgr->advertised, advertised * 100);
 }
 
 void
@@ -498,19 +498,19 @@ isc_nm_gettimeouts(isc_nm_t *mgr, uint32_t *initial, uint32_t *idle,
        REQUIRE(VALID_NM(mgr));
 
        if (initial != NULL) {
-               *initial = mgr->init / 100;
+               *initial = atomic_load(&mgr->init) / 100;
        }
 
        if (idle != NULL) {
-               *idle = mgr->idle / 100;
+               *idle = atomic_load(&mgr->idle) / 100;
        }
 
        if (keepalive != NULL) {
-               *keepalive = mgr->keepalive / 100;
+               *keepalive = atomic_load(&mgr->keepalive) / 100;
        }
 
        if (advertised != NULL) {
-               *advertised = mgr->advertised / 100;
+               *advertised = atomic_load(&mgr->advertised) / 100;
        }
 }
 

diff --git a/lib/isc/netmgr/tcp.c b/lib/isc/netmgr/tcp.c
index 57d2ccc923f6da0d64ddbe8252b6ff99e84375b8..99209f410accc5940b69e73a91e57df6955211cd 100644 (file)
--- a/lib/isc/netmgr/tcp.c
+++ b/lib/isc/netmgr/tcp.c
@@ -790,9 +790,10 @@ isc__nm_tcp_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
        sock->recv_cbarg = cbarg;
        sock->recv_read = true;
        if (sock->read_timeout == 0) {
-               sock->read_timeout = (atomic_load(&sock->keepalive)
-                                             ? sock->mgr->keepalive
-                                             : sock->mgr->idle);
+               sock->read_timeout =
+                       (atomic_load(&sock->keepalive)
+                                ? atomic_load(&sock->mgr->keepalive)
+                                : atomic_load(&sock->mgr->idle));
        }
 
        ievent = isc__nm_get_netievent_tcpstartread(sock->mgr, sock);
@@ -959,9 +960,10 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
        req->uvbuf.len = nread;
 
        if (!atomic_load(&sock->client)) {
-               sock->read_timeout = (atomic_load(&sock->keepalive)
-                                             ? sock->mgr->keepalive
-                                             : sock->mgr->idle);
+               sock->read_timeout =
+                       (atomic_load(&sock->keepalive)
+                                ? atomic_load(&sock->mgr->keepalive)
+                                : atomic_load(&sock->mgr->idle));
        }
 
        isc__nm_readcb(sock, req, ISC_R_SUCCESS);
@@ -1100,7 +1102,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
 
        isc__nm_incstats(csock->mgr, csock->statsindex[STATID_ACCEPT]);
 
-       csock->read_timeout = csock->mgr->init;
+       csock->read_timeout = atomic_load(&csock->mgr->init);
 
        atomic_fetch_add(&ssock->parent->active_child_connections, 1);
 

diff --git a/lib/isc/netmgr/tcpdns.c b/lib/isc/netmgr/tcpdns.c
index 8cc0c4a654e3ba81648eb5e400f178c78a012bc7..0b976ecdc5ccc6e04019b450a4dde590538e9529 100644 (file)
--- a/lib/isc/netmgr/tcpdns.c
+++ b/lib/isc/netmgr/tcpdns.c
@@ -846,9 +846,10 @@ isc__nm_tcpdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
        sock->recv_cbarg = cbarg;
        sock->recv_read = true;
        if (sock->read_timeout == 0) {
-               sock->read_timeout = (atomic_load(&sock->keepalive)
-                                             ? sock->mgr->keepalive
-                                             : sock->mgr->idle);
+               sock->read_timeout =
+                       (atomic_load(&sock->keepalive)
+                                ? atomic_load(&sock->mgr->keepalive)
+                                : atomic_load(&sock->mgr->idle));
        }
 
        ievent = isc__nm_get_netievent_tcpdnsread(sock->mgr, sock);
@@ -1037,7 +1038,7 @@ read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t *buf) {
        sock->buf_len += len;
 
        if (!atomic_load(&sock->client)) {
-               sock->read_timeout = sock->mgr->idle;
+               sock->read_timeout = atomic_load(&sock->mgr->idle);
        }
 
        process_sock_buffer(sock);
@@ -1182,7 +1183,7 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
 
        isc__nm_incstats(csock->mgr, csock->statsindex[STATID_ACCEPT]);
 
-       csock->read_timeout = csock->mgr->init;
+       csock->read_timeout = atomic_load(&csock->mgr->init);
 
        csock->closehandle_cb = resume_processing;
 
@@ -1199,8 +1200,8 @@ accept_connection(isc_nmsocket_t *ssock, isc_quota_t *quota) {
         * reads.
         */
        csock->read_timeout = (atomic_load(&csock->keepalive)
-                                      ? csock->mgr->keepalive
-                                      : csock->mgr->idle);
+                                      ? atomic_load(&csock->mgr->keepalive)
+                                      : atomic_load(&csock->mgr->idle));
 
        isc_nmhandle_detach(&handle);
 

diff --git a/lib/isc/netmgr/tls.c b/lib/isc/netmgr/tls.c
index 1d80abcf47245fba6c111bc952a6520baa14a568..f685493f38452238fc2f29d1f206afb0680b45e3 100644 (file)
--- a/lib/isc/netmgr/tls.c
+++ b/lib/isc/netmgr/tls.c
@@ -336,7 +336,7 @@ tlslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
        isc__nmsocket_attach(tlslistensock, &tlssock->listener);
        isc_nmhandle_attach(handle, &tlssock->outerhandle);
        tlssock->peer = handle->sock->peer;
-       tlssock->read_timeout = handle->sock->mgr->init;
+       tlssock->read_timeout = atomic_load(&handle->sock->mgr->init);
        tlssock->tid = isc_nm_tid();
        tlssock->tls.server = true;
        tlssock->tls.state = TLS_INIT;

diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c
index b56b5ea1606c87e297db5bbb939a92c7069986e3..7359e0f452e01d03423da8d551056e817bce755b 100644 (file)
--- a/lib/isc/netmgr/tlsdns.c
+++ b/lib/isc/netmgr/tlsdns.c
@@ -156,7 +156,7 @@ dnslisten_acceptcb(isc_nmhandle_t *handle, isc_result_t result, void *cbarg) {
        isc_nmhandle_attach(handle, &dnssock->outerhandle);
 
        dnssock->peer = handle->sock->peer;
-       dnssock->read_timeout = handle->sock->mgr->init;
+       dnssock->read_timeout = atomic_load(&handle->sock->mgr->init);
        dnssock->tid = isc_nm_tid();
        dnssock->closehandle_cb = resume_processing;
 
@@ -329,8 +329,8 @@ dnslisten_readcb(isc_nmhandle_t *handle, isc_result_t eresult,
        dnssock->buf_len += len;
 
        dnssock->read_timeout = (atomic_load(&dnssock->keepalive)
-                                        ? dnssock->mgr->keepalive
-                                        : dnssock->mgr->idle);
+                                        ? atomic_load(&dnssock->mgr->keepalive)
+                                        : atomic_load(&dnssock->mgr->idle));
 
        do {
                isc_result_t result;
@@ -754,7 +754,7 @@ tlsdnsconnect_cb(isc_nmhandle_t *handle, isc_result_t result, void *arg) {
        isc_nmhandle_attach(handle, &dnssock->outerhandle);
 
        dnssock->peer = handle->sock->peer;
-       dnssock->read_timeout = handle->sock->mgr->init;
+       dnssock->read_timeout = atomic_load(&handle->sock->mgr->init);
        dnssock->tid = isc_nm_tid();
 
        atomic_init(&dnssock->client, true);
@@ -852,8 +852,8 @@ isc__nm_tlsdns_read(isc_nmhandle_t *handle, isc_nm_recv_cb_t cb, void *cbarg) {
        sock->recv_cbarg = cbarg;
 
        sock->read_timeout = (atomic_load(&sock->keepalive)
-                                     ? sock->mgr->keepalive
-                                     : sock->mgr->idle);
+                                     ? atomic_load(&sock->mgr->keepalive)
+                                     : atomic_load(&sock->mgr->idle));
 
        /*
         * Add a reference to the handle to keep it from being freed by