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)

diff --git a/lib/dns/adb.c b/lib/dns/adb.c
index 2eef79bb47e0f9e3bbb462c0f0cc7cb0dfea6dc7..209870c17084024850bbfccee1432084197078c5 100644 (file)
--- 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 ce605f0c6ccd28db8be6fd3971b60c7fb7d8ab63..f3386aab5781d7e87767bb0c17a3491f374349cc 100644 (file)
--- 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 52063d16b20df7c9da41b6189db53baac64d6410..665c12bc20f0b61ecafcb54837af5271ea70c7ab 100644 (file)
--- 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 f04bb5737be2ff67690df49bdf7124040b7c6e24..b06d37b08717222848551b2439636982e46b5251 100644 (file)
--- 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 d02371c95004c50ca23f92fd4e680a8429e50649..c60fd9a671740a9587d78cdb8b24e7354a455f0f 100644 (file)
--- 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) {
+       sigset_t sset;
+
        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;
+       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 a8344693b3c090cca1b7732df1f725aa0b0af02e..5edb0957f0ffa0107ee9fafa16963663025a78c9 100644 (file)
--- a/lib/isc/include/isc/atomic.h
+++ b/lib/isc/include/isc/atomic.h
@@ -19,6 +19,8 @@
 #include <isc/stdatomic.h>
 #endif
 
+#include <isc/util.h>
+
 /*
  * 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 5250a28e23c66981aa057a618545afda89692430..bebe0e5c6cbceb1bacfde802629bfe9b064b6c73 100644 (file)
--- 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 cc7263d914e748fb4ffd8e2733003214708bdf20..edb2a9dc66af50a82429baabb3dccb75ff94b05a 100644 (file)
--- 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 0c8e91fce815060eb4861ec82b9de40d80eb5ed7..e099a62bed0dd35ca350a638306fc0b62b0e58e2 100644 (file)
--- 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 e77d6421cf8c3cf3996300b1b98e21555d0f73d2..5465db5b73bd346b6ea6ec7b515709f74bd576ea 100644 (file)
--- 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(&quota->used, 1) > 0);
+       used = atomic_fetch_sub_release(&quota->used, 1);
+       INSIST(used > 0);
 }
 
 static isc_result_t

diff --git a/lib/isc/task.c b/lib/isc/task.c
index cd846048daaabd597ef7447932c83befc258a33c..df0e91207e82a80b78ea2e836920dc3ba98fc2b3 100644 (file)
--- 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 a2264a3945282a98a5c2d83ca2a7a5bccc1e64e2..4dc848260ea9f2c5c4b676a678d9b0d651df2deb 100644 (file)
--- 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