rename NS_QUERY_RESET to NS_QUERY_CLEANUP

query_reset() is called during query initialization, but the only
time the NS_QUERY_SETUP hook runs is when it's called from
query_cleanup().  it makes more sense to move the hook point to
there and rename it to NS_QUERY_CLEANUP.

this change caused a crash in the unit tests due to the view being
unnecessarily detached before ns__client_reset_cb() was called.
this has also been fixed.

diff --git a/bin/plugins/filter-a.c b/bin/plugins/filter-a.c
index e850739730462cf428a57925bcdc70cb354e6838..d5b4bb69aa4608d4007db20dbfa3f88abed5711c 100644 (file)
--- a/bin/plugins/filter-a.c
+++ b/bin/plugins/filter-a.c
@@ -164,7 +164,7 @@ install_hooks(ns_hooktable_t *hooktable, isc_mem_t *mctx,
        ns_hook_add(hooktable, mctx, NS_QUERY_PREP_RESPONSE_BEGIN,
                    &filter_prepresp);
        ns_hook_add(hooktable, mctx, NS_QUERY_DONE_SEND, &filter_donesend);
-       ns_hook_add(hooktable, mctx, NS_QUERY_RESET, &filter_reset);
+       ns_hook_add(hooktable, mctx, NS_QUERY_CLEANUP, &filter_reset);
 }
 
 /**

diff --git a/bin/plugins/filter-aaaa.c b/bin/plugins/filter-aaaa.c
index 3a0dc1a98c6c50fbfc83320e77d5856c71fae65f..4bdff94116a9b19f7de0d5fef2f1ec566565fa00 100644 (file)
--- a/bin/plugins/filter-aaaa.c
+++ b/bin/plugins/filter-aaaa.c
@@ -164,7 +164,7 @@ install_hooks(ns_hooktable_t *hooktable, isc_mem_t *mctx,
        ns_hook_add(hooktable, mctx, NS_QUERY_PREP_RESPONSE_BEGIN,
                    &filter_prepresp);
        ns_hook_add(hooktable, mctx, NS_QUERY_DONE_SEND, &filter_donesend);
-       ns_hook_add(hooktable, mctx, NS_QUERY_RESET, &filter_reset);
+       ns_hook_add(hooktable, mctx, NS_QUERY_CLEANUP, &filter_reset);
 }
 
 /**

diff --git a/bin/tests/system/hooks/driver/test-async.c b/bin/tests/system/hooks/driver/test-async.c
index 0e7158be8564262696e853371eb37ef5263fa6de..e412fdcbe72ecef7b113ddfbea4e9a8ea68fff18 100644 (file)
--- a/bin/tests/system/hooks/driver/test-async.c
+++ b/bin/tests/system/hooks/driver/test-async.c
@@ -96,7 +96,7 @@ install_hooks(ns_hooktable_t *hooktable, isc_mem_t *mctx,
 
        ns_hook_add(hooktable, mctx, NS_QUERY_SETUP, &async_setup);
        ns_hook_add(hooktable, mctx, NS_QUERY_DONE_BEGIN, &async_donebegin);
-       ns_hook_add(hooktable, mctx, NS_QUERY_RESET, &async_reset);
+       ns_hook_add(hooktable, mctx, NS_QUERY_CLEANUP, &async_reset);
 }
 
 static void

diff --git a/lib/ns/client.c b/lib/ns/client.c
index f3a3dc44b3c55456442465e8d48c5a399982fbd9..0bd85a7c278bdd3af388279264224365438d31c5 100644 (file)
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -257,7 +257,7 @@ ns_client_endrequest(ns_client_t *client) {
 #ifdef ENABLE_AFL
                if (client->manager->sctx->fuzztype == isc_fuzz_resolver) {
                        dns_adb_t *adb = NULL;
-                       dns_view_getadb(client->view, &adb);
+                       dns_view_getadb(client->inner.view, &adb);
                        if (adb != NULL) {
                                dns_adb_flush(adb);
                                dns_adb_detach(&adb);
@@ -774,7 +774,7 @@ renderend:
 #ifdef HAVE_DNSTAP
                /*
                 * Log dnstap data first, because client_sendpkg() may
-                * leave client->view set to NULL.
+                * leave client->inner.view set to NULL.
                 */
                if (client->inner.view != NULL) {
                        dns_dt_send(client->inner.view, dtmsgtype,
@@ -1843,7 +1843,7 @@ ns_client_setup_view(ns_client_t *client, isc_netaddr_t *netaddr) {
        /*
         * matchingview() returning anything other than DNS_R_WAIT means it's
         * not running in async mode, in which case 'result' must be equal to
-        * 'client->viewmatchresult'.
+        * 'client->inner.viewmatchresult'.
         */
        INSIST(result == client->inner.viewmatchresult);
 

diff --git a/lib/ns/include/ns/hooks.h b/lib/ns/include/ns/hooks.h
index 3b1b4b605737770b2c74f3dbc49cdb247678f4c9..0680eecfce319f7b988bfb95ea5d30b81565f773 100644 (file)
--- a/lib/ns/include/ns/hooks.h
+++ b/lib/ns/include/ns/hooks.h
@@ -385,7 +385,7 @@
 typedef enum {
        /* hookpoints from query.c */
        NS_QUERY_SETUP,
-       NS_QUERY_RESET,
+       NS_QUERY_CLEANUP,
        NS_QUERY_START_BEGIN,
        NS_QUERY_LOOKUP_BEGIN,
        NS_QUERY_RESUME_BEGIN,

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 5a8d646e4aeb71fa5be15407b103215e638f1115..f5f8d847095ad24e6bacdc4749d855010b45f8eb 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -304,14 +304,17 @@ ns__query_callhook_noreturn(uint8_t id, query_ctx_t *qctx,
 }
 
 /*
- * Call the specified hook function in every configured module that implements
- * that function. If any hook function returns NS_HOOK_RETURN, we
- * set 'result' and terminate processing by jumping to the 'cleanup' tag.
+ * Call the specified hook function in every configured module that
+ * implements that function. If any hook function returns NS_HOOK_RETURN,
+ * we set 'result' and terminate processing by jumping to the 'cleanup' tag.
+ *
+ * Before any hook point is reached, qctx->view must be initialized to
+ * point to a valid view.
  *
- * (Note that a hook function may set the 'result' to ISC_R_SUCCESS but
+ * Note: that a hook function may set the 'result' to ISC_R_SUCCESS but
  * still terminate processing within the calling function. That's why this
  * is a macro instead of a static function; it needs to be able to use
- * 'goto cleanup' regardless of the return value.)
+ * 'goto cleanup' regardless of the return value.
  */
 #define CALL_HOOK(_id, _qctx)                                               \
        if ((_qctx)->zhooks != NULL &&                                      \
@@ -339,8 +342,11 @@ ns__query_callhook_noreturn(uint8_t id, query_ctx_t *qctx,
  * codes are ignored. This is intended for use with initialization and
  * destruction calls which *must* run in every configured module.
  *
- * (This could be implemented as a static void function, but is left as a
- * macro for symmetry with CALL_HOOK above.)
+ * Before any hook point is reached, qctx->view must be initialized to
+ * point to a valid view.
+ *
+ * This could be implemented as a static void function, but is left as a
+ * macro for symmetry with CALL_HOOK above.
  */
 #define CALL_HOOK_NORETURN(_id, _qctx)                                    \
        if ((_qctx)->zhooks != NULL) {                                    \
@@ -806,27 +812,6 @@ query_reset(ns_client_t *client, bool everything) {
 
        CTRACE(ISC_LOG_DEBUG(3), "query_reset");
 
-       /*
-        * Set up a transient qctx so we can call the NS_QUERY_RESET hook;
-        * this will free resources being held by plugins for this
-        * query, if any were configured.
-        *
-        * Note that NS_QUERY_RESET hook is called only if the view is not
-        * NULL at this point. Otherwise, it means the client has been
-        * reset even before the query starts, so we should not call this
-        * hook as no other hook has been called before.
-        */
-       if (client->inner.view != NULL) {
-               query_ctx_t qctx = { .view = client->inner.view,
-                                    .client = client };
-               if (client->query.authzone != NULL) {
-                       qctx.zhooks =
-                               dns_zone_gethooktable(client->query.authzone);
-               }
-
-               CALL_HOOK_NORETURN(NS_QUERY_RESET, &qctx);
-       }
-
        /*
         * Cancel the fetch if it's running.
         */
@@ -920,6 +905,18 @@ query_reset(ns_client_t *client, bool everything) {
 
 static void
 query_cleanup(ns_client_t *client) {
+       /*
+        * Set up a transient qctx so we can call the NS_QUERY_CLEANUP hook;
+        * this will free resources being held by plugins for this
+        * query, if any were configured.
+        */
+       query_ctx_t qctx = { .view = client->inner.view, .client = client };
+       if (client->query.authzone != NULL) {
+               qctx.zhooks = dns_zone_gethooktable(client->query.authzone);
+       }
+
+       CALL_HOOK_NORETURN(NS_QUERY_CLEANUP, &qctx);
+
        query_reset(client, false);
 }
 

diff --git a/tests/ns/netmgr_wrap.c b/tests/ns/netmgr_wrap.c
index f2b265c329f3daec3e536cb5ea1f3039693a6988..c2e62352f93f526572e150cef04830d2c1bd79e9 100644 (file)
--- a/tests/ns/netmgr_wrap.c
+++ b/tests/ns/netmgr_wrap.c
@@ -84,7 +84,6 @@ isc_nmhandle_detach(isc_nmhandle_t **handlep) {
        INSIST(i < 32);
 
        if (atomic_fetch_sub(&client_refs[i], 1) == 1) {
-               dns_view_detach(&client->inner.view);
                client->inner.state = 4;
                ns__client_reset_cb(client);
                ns__client_put_cb(client);