From b061e86d17bf97dc6f31ff6ad270dd45308877fa Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 21 Oct 2021 02:28:48 -0700 Subject: [PATCH] REQUIRE should not have side effects it's a style violation to have REQUIRE or INSIST contain code that must run for the server to work. this was being done with some atomic_compare_exchange calls. these have been cleaned up. uses of atomic_compare_exchange in assertions have been replaced with a new macro atomic_compare_exchange_enforced, which uses RUNTIME_CHECK to ensure that the exchange was successful. (cherry picked from commit a499794984a17760969bd509a0c253aa2f2ecb6c) --- lib/dns/adb.c | 10 ++++++++-- lib/dns/dispatch.c | 5 +++-- lib/dns/resolver.c | 19 ++++++++++++------- lib/dns/zone.c | 5 +++-- lib/isc/app.c | 31 +++++++++++++++++-------------- lib/isc/include/isc/atomic.h | 6 ++++++ lib/isc/mem.c | 7 +++++-- lib/isc/netmgr/netmgr.c | 19 +++++++++---------- lib/isc/netmgr/tlsdns.c | 12 ++++++------ lib/isc/quota.c | 5 ++++- lib/isc/task.c | 7 ++++--- lib/isc/tls.c | 6 ++---- 12 files changed, 79 insertions(+), 53 deletions(-) diff --git a/lib/dns/adb.c b/lib/dns/adb.c index 2eef79bb47..209870c170 100644 --- a/lib/dns/adb.c +++ b/lib/dns/adb.c @@ -4672,16 +4672,22 @@ dns_adbentry_overquota(dns_adbentry_t *entry) { void dns_adb_beginudpfetch(dns_adb_t *adb, dns_adbaddrinfo_t *addr) { + uint_fast32_t active; + REQUIRE(DNS_ADB_VALID(adb)); REQUIRE(DNS_ADBADDRINFO_VALID(addr)); - INSIST(atomic_fetch_add_relaxed(&addr->entry->active, 1) != UINT32_MAX); + active = atomic_fetch_add_relaxed(&addr->entry->active, 1); + INSIST(active != UINT32_MAX); } void dns_adb_endudpfetch(dns_adb_t *adb, dns_adbaddrinfo_t *addr) { + uint_fast32_t active; + REQUIRE(DNS_ADB_VALID(adb)); REQUIRE(DNS_ADBADDRINFO_VALID(addr)); - INSIST(atomic_fetch_sub_release(&addr->entry->active, 1) != 0); + active = atomic_fetch_sub_release(&addr->entry->active, 1); + INSIST(active != 0); } diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index ce605f0c6c..f3386aab57 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -1694,12 +1694,13 @@ startrecv(isc_nmhandle_t *handle, dns_dispatch_t *disp, dns_dispentry_t *resp) { case isc_socktype_tcp: REQUIRE(disp != NULL); + LOCK(&disp->lock); REQUIRE(disp->handle == NULL); - REQUIRE(atomic_compare_exchange_strong( + atomic_compare_exchange_enforced( &disp->tcpstate, &(uint_fast32_t){ DNS_DISPATCHSTATE_CONNECTING }, - DNS_DISPATCHSTATE_CONNECTED)); + DNS_DISPATCHSTATE_CONNECTED); isc_nmhandle_attach(handle, &disp->handle); dns_dispatch_attach(disp, &(dns_dispatch_t *){ NULL }); diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 52063d16b2..665c12bc20 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -2989,6 +2989,7 @@ fctx_finddone(isc_task_t *task, isc_event_t *event) { bool want_try = false; bool want_done = false; unsigned int bucketnum; + uint_fast32_t pending; REQUIRE(VALID_FCTX(fctx)); res = fctx->res; @@ -3000,7 +3001,8 @@ fctx_finddone(isc_task_t *task, isc_event_t *event) { bucketnum = fctx->bucketnum; LOCK(&res->buckets[bucketnum].lock); - INSIST(atomic_fetch_sub_release(&fctx->pending, 1) > 0); + pending = atomic_fetch_sub_release(&fctx->pending, 1); + INSIST(pending > 0); if (ADDRWAIT(fctx)) { /* @@ -4352,6 +4354,7 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) { struct tried *tried = NULL; unsigned int bucketnum; bool bucket_empty = false; + uint_fast32_t nfctx; REQUIRE(VALID_FCTX(fctx)); REQUIRE(ISC_LIST_EMPTY(fctx->events)); @@ -4371,7 +4374,8 @@ fctx_destroy(fetchctx_t *fctx, bool exiting) { ISC_LIST_UNLINK(res->buckets[bucketnum].fctxs, fctx, link); - INSIST(atomic_fetch_sub_release(&res->nfctx, 1) > 0); + nfctx = atomic_fetch_sub_release(&res->nfctx, 1); + INSIST(nfctx > 0); dec_stats(res, dns_resstatscounter_nfetch); @@ -4693,6 +4697,7 @@ fctx_create(dns_resolver_t *res, isc_task_t *task, const dns_name_t *name, isc_interval_t interval; unsigned int findoptions = 0; char buf[DNS_NAME_FORMATSIZE + DNS_RDATATYPE_FORMATSIZE + 1]; + uint_fast32_t nfctx; size_t p; /* @@ -4978,7 +4983,8 @@ fctx_create(dns_resolver_t *res, isc_task_t *task, const dns_name_t *name, ISC_LIST_APPEND(res->buckets[bucketnum].fctxs, fctx, link); - INSIST(atomic_fetch_add_relaxed(&res->nfctx, 1) < UINT32_MAX); + nfctx = atomic_fetch_add_relaxed(&res->nfctx, 1); + INSIST(nfctx < UINT32_MAX); inc_stats(res, dns_resstatscounter_nfetch); @@ -10405,8 +10411,7 @@ prime_done(isc_task_t *task, isc_event_t *event) { res->primefetch = NULL; UNLOCK(&res->primelock); - INSIST(atomic_compare_exchange_strong_acq_rel(&res->priming, - &(bool){ true }, false)); + atomic_compare_exchange_enforced(&res->priming, &(bool){ true }, false); if (fevent->result == ISC_R_SUCCESS && res->view->cache != NULL && res->view->hints != NULL) @@ -10474,8 +10479,8 @@ dns_resolver_prime(dns_resolver_t *res) { if (result != ISC_R_SUCCESS) { isc_mem_put(res->mctx, rdataset, sizeof(*rdataset)); - INSIST(atomic_compare_exchange_strong_acq_rel( - &res->priming, &(bool){ true }, false)); + atomic_compare_exchange_enforced( + &res->priming, &(bool){ true }, false); } inc_stats(res, dns_resstatscounter_priming); } diff --git a/lib/dns/zone.c b/lib/dns/zone.c index f04bb5737b..b06d37b087 100644 --- a/lib/dns/zone.c +++ b/lib/dns/zone.c @@ -13414,8 +13414,9 @@ stub_request_nameserver_address(struct stub_cb_args *args, bool ipv4, stub_glue_response_cb, request, &request->request); if (result != ISC_R_SUCCESS) { - INSIST(atomic_fetch_sub_release(&args->stub->pending_requests, - 1) > 1); + uint_fast32_t pr; + pr = atomic_fetch_sub_release(&args->stub->pending_requests, 1); + INSIST(pr > 1); zone_debuglog(zone, "stub_send_query", 1, "dns_request_createvia() failed: %s", isc_result_totext(result)); diff --git a/lib/isc/app.c b/lib/isc/app.c index d02371c950..c60fd9a671 100644 --- a/lib/isc/app.c +++ b/lib/isc/app.c @@ -84,6 +84,10 @@ handle_signal(int sig, void (*handler)(int)) { isc_result_t isc_app_ctxstart(isc_appctx_t *ctx) { + int presult; + sigset_t sset; + char strbuf[ISC_STRERRORSIZE]; + REQUIRE(VALID_APPCTX(ctx)); /* @@ -103,10 +107,6 @@ isc_app_ctxstart(isc_appctx_t *ctx) { atomic_init(&ctx->want_reload, false); atomic_init(&ctx->blocked, false); - int presult; - sigset_t sset; - char strbuf[ISC_STRERRORSIZE]; - /* * Always ignore SIGPIPE. */ @@ -283,8 +283,8 @@ isc_result_t isc_app_run(void) { isc_result_t result; - REQUIRE(atomic_compare_exchange_strong_acq_rel(&is_running, - &(bool){ false }, true)); + atomic_compare_exchange_enforced(&is_running, &(bool){ false }, true); + result = isc_app_ctxrun(&isc_g_appctx); atomic_store_release(&is_running, false); @@ -380,11 +380,13 @@ isc_app_finish(void) { void isc_app_block(void) { - REQUIRE(atomic_load_acquire(&isc_g_appctx.running)); - REQUIRE(atomic_compare_exchange_strong_acq_rel(&isc_g_appctx.blocked, - &(bool){ false }, true)); - sigset_t sset; + + REQUIRE(atomic_load_acquire(&isc_g_appctx.running)); + + atomic_compare_exchange_enforced(&isc_g_appctx.blocked, + &(bool){ false }, true); + blockedthread = pthread_self(); RUNTIME_CHECK(sigemptyset(&sset) == 0 && sigaddset(&sset, SIGINT) == 0 && @@ -394,13 +396,14 @@ isc_app_block(void) { void isc_app_unblock(void) { - REQUIRE(atomic_load_acquire(&isc_g_appctx.running)); - REQUIRE(atomic_compare_exchange_strong_acq_rel(&isc_g_appctx.blocked, - &(bool){ true }, false)); + sigset_t sset; + REQUIRE(atomic_load_acquire(&isc_g_appctx.running)); REQUIRE(blockedthread == pthread_self()); - sigset_t sset; + atomic_compare_exchange_enforced(&isc_g_appctx.blocked, &(bool){ true }, + false); + RUNTIME_CHECK(sigemptyset(&sset) == 0 && sigaddset(&sset, SIGINT) == 0 && sigaddset(&sset, SIGTERM) == 0); diff --git a/lib/isc/include/isc/atomic.h b/lib/isc/include/isc/atomic.h index a8344693b3..5edb0957f0 100644 --- a/lib/isc/include/isc/atomic.h +++ b/lib/isc/include/isc/atomic.h @@ -19,6 +19,8 @@ #include #endif +#include + /* * We define a few additional macros to make things easier */ @@ -71,3 +73,7 @@ #define atomic_compare_exchange_strong_acq_rel(o, e, d) \ atomic_compare_exchange_strong_explicit( \ (o), (e), (d), memory_order_acq_rel, memory_order_acquire) + +/* compare/exchange that MUST succeed */ +#define atomic_compare_exchange_enforced(o, e, d) \ + RUNTIME_CHECK(atomic_compare_exchange_strong((o), (e), (d))) diff --git a/lib/isc/mem.c b/lib/isc/mem.c index 5250a28e23..bebe0e5c6c 100644 --- a/lib/isc/mem.c +++ b/lib/isc/mem.c @@ -414,12 +414,15 @@ mem_getstats(isc_mem_t *ctx, size_t size) { static void mem_putstats(isc_mem_t *ctx, void *ptr, size_t size) { struct stats *stats = stats_bucket(ctx, size); + uint_fast32_t s, g; UNUSED(ptr); - INSIST(atomic_fetch_sub_release(&ctx->inuse, size) >= size); + s = atomic_fetch_sub_release(&ctx->inuse, size); + INSIST(s >= size); - INSIST(atomic_fetch_sub_release(&stats->gets, 1) >= 1); + g = atomic_fetch_sub_release(&stats->gets, 1); + INSIST(g >= 1); decrement_malloced(ctx, size); } diff --git a/lib/isc/netmgr/netmgr.c b/lib/isc/netmgr/netmgr.c index cc7263d914..edb2a9dc66 100644 --- a/lib/isc/netmgr/netmgr.c +++ b/lib/isc/netmgr/netmgr.c @@ -428,8 +428,7 @@ isc_nm_pause(isc_nm_t *mgr) { } UNLOCK(&mgr->lock); - REQUIRE(atomic_compare_exchange_strong(&mgr->paused, &(bool){ false }, - true)); + atomic_compare_exchange_enforced(&mgr->paused, &(bool){ false }, true); } static void @@ -479,8 +478,7 @@ isc_nm_resume(isc_nm_t *mgr) { } UNLOCK(&mgr->lock); - REQUIRE(atomic_compare_exchange_strong(&mgr->paused, &(bool){ true }, - false)); + atomic_compare_exchange_enforced(&mgr->paused, &(bool){ true }, false); isc__nm_drop_interlocked(mgr); } @@ -1744,6 +1742,7 @@ nmhandle_free(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { static void nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { bool reuse = false; + uint_fast32_t ah; /* * We do all of this under lock to avoid races with socket @@ -1756,7 +1755,8 @@ nmhandle_deactivate(isc_nmsocket_t *sock, isc_nmhandle_t *handle) { ISC_LIST_UNLINK(sock->active_handles, handle, active_link); #endif - INSIST(atomic_fetch_sub(&sock->ah, 1) > 0); + ah = atomic_fetch_sub(&sock->ah, 1); + INSIST(ah > 0); #if !__SANITIZE_ADDRESS__ && !__SANITIZE_THREAD__ if (atomic_load(&sock->active)) { @@ -1950,8 +1950,8 @@ isc__nm_failed_connect_cb(isc_nmsocket_t *sock, isc__nm_uvreq_t *req, isc__nmsocket_timer_stop(sock); uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock); - INSIST(atomic_compare_exchange_strong(&sock->connecting, - &(bool){ true }, false)); + atomic_compare_exchange_enforced(&sock->connecting, &(bool){ true }, + false); isc__nmsocket_clearcb(sock); isc__nm_connectcb(sock, req, eresult, async); @@ -2002,9 +2002,8 @@ isc__nmsocket_connecttimeout_cb(uv_timer_t *timer) { /* * Mark the connection as timed out and shutdown the socket. */ - - INSIST(atomic_compare_exchange_strong(&sock->timedout, &(bool){ false }, - true)); + atomic_compare_exchange_enforced(&sock->timedout, &(bool){ false }, + true); isc__nmsocket_clearcb(sock); isc__nmsocket_shutdown(sock); } diff --git a/lib/isc/netmgr/tlsdns.c b/lib/isc/netmgr/tlsdns.c index 0c8e91fce8..e099a62bed 100644 --- a/lib/isc/netmgr/tlsdns.c +++ b/lib/isc/netmgr/tlsdns.c @@ -214,8 +214,8 @@ isc__nm_async_tlsdnsconnect(isc__networker_t *worker, isc__netievent_t *ev0) { result = tlsdns_connect_direct(sock, req); if (result != ISC_R_SUCCESS) { - INSIST(atomic_compare_exchange_strong(&sock->connecting, - &(bool){ true }, false)); + atomic_compare_exchange_enforced(&sock->connecting, + &(bool){ true }, false); isc__nmsocket_clearcb(sock); isc__nm_connectcb(sock, req, result, true); atomic_store(&sock->active, false); @@ -422,8 +422,8 @@ failure: sock->tid = isc_nm_tid(); } - INSIST(atomic_compare_exchange_strong(&sock->connecting, - &(bool){ true }, false)); + atomic_compare_exchange_enforced(&sock->connecting, &(bool){ true }, + false); isc__nmsocket_clearcb(sock); isc__nm_connectcb(sock, req, result, true); atomic_store(&sock->closed, true); @@ -1173,8 +1173,8 @@ tls_cycle_input(isc_nmsocket_t *sock) { uv_handle_set_data((uv_handle_t *)&sock->read_timer, sock); - INSIST(atomic_compare_exchange_strong( - &sock->connecting, &(bool){ true }, false)); + atomic_compare_exchange_enforced( + &sock->connecting, &(bool){ true }, false); isc__nm_connectcb(sock, req, ISC_R_SUCCESS, true); } async_tlsdns_cycle(sock); diff --git a/lib/isc/quota.c b/lib/isc/quota.c index e77d6421cf..5465db5b73 100644 --- a/lib/isc/quota.c +++ b/lib/isc/quota.c @@ -121,6 +121,8 @@ dequeue(isc_quota_t *quota) { static void quota_release(isc_quota_t *quota) { + uint_fast32_t used; + /* * This is opportunistic - we might race with a failing quota_attach_cb * and not detect that something is waiting, but eventually someone will @@ -141,7 +143,8 @@ quota_release(isc_quota_t *quota) { } } - INSIST(atomic_fetch_sub_release("a->used, 1) > 0); + used = atomic_fetch_sub_release("a->used, 1); + INSIST(used > 0); } static isc_result_t diff --git a/lib/isc/task.c b/lib/isc/task.c index cd846048da..df0e91207e 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -1127,10 +1127,11 @@ isc_task_beginexclusive(isc_task_t *task) { void isc_task_endexclusive(isc_task_t *task) { - isc_taskmgr_t *manager; + isc_taskmgr_t *manager = NULL; REQUIRE(VALID_TASK(task)); REQUIRE(task->state == task_state_running); + manager = task->manager; if (isc_log_wouldlog(isc_lctx, ISC_LOG_DEBUG(1))) { @@ -1147,8 +1148,8 @@ isc_task_endexclusive(isc_task_t *task) { "exclusive task mode: %s", "ended"); } - REQUIRE(atomic_compare_exchange_strong(&manager->exclusive_req, - &(bool){ true }, false)); + atomic_compare_exchange_enforced(&manager->exclusive_req, + &(bool){ true }, false); } void diff --git a/lib/isc/tls.c b/lib/isc/tls.c index a2264a3945..4dc848260e 100644 --- a/lib/isc/tls.c +++ b/lib/isc/tls.c @@ -128,8 +128,7 @@ tls_initialize(void) { "seeded' message in the OpenSSL FAQ)"); } - REQUIRE(atomic_compare_exchange_strong(&init_done, &(bool){ false }, - true)); + atomic_compare_exchange_enforced(&init_done, &(bool){ false }, true); } void @@ -167,8 +166,7 @@ tls_shutdown(void) { } #endif - REQUIRE(atomic_compare_exchange_strong(&shut_done, &(bool){ false }, - true)); + atomic_compare_exchange_enforced(&shut_done, &(bool){ false }, true); } void