From: Ondřej Surý Date: Wed, 2 Dec 2020 08:52:39 +0000 (+0100) Subject: Fix the data race in accessing the isc_nm_t timers X-Git-Tag: v9.16.11~17^2~15 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=48759bd04799989acff8985739c49e702a600949;p=thirdparty%2Fbind9.git 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 (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 (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 (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 (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 (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 b80cddb0660..a4a44cbf1a2 100644 --- 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 5e6d47f79a2..153e80f2f96 100644 --- 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 d21634e6a12..7a19af56d17 100644 --- 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 8f5c78d01b7..b946ede5bf9 100644 --- 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 57d2ccc923f..99209f410ac 100644 --- 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 8cc0c4a654e..0b976ecdc5c 100644 --- 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 1d80abcf472..f685493f384 100644 --- 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 b56b5ea1606..7359e0f452e 100644 --- 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