From d676ce8085e8dcbfefe9b0cacf485caa2d9cb18e Mon Sep 17 00:00:00 2001 From: Colin Vidal Date: Mon, 8 Sep 2025 10:12:53 +0200 Subject: [PATCH] remove query_ctx_t detach_client property Since the removal of NS_QUERY_QCTX_DESTROYED hook, there is no need for the `qctx->detach_client` object anymore, as this was designed to tell the plugin whether the client object is about to be, or is already, freed from memory. This is not needed anymore, as NS_QUERY_RESET is called _always_ when the client object is about to be freed from memory. Remove `detach_client` and tidy up the code a bit by including the freeing of the qctx object (when allocated) inside the qctx_destroy function instead of requiring extra calls. --- lib/ns/include/ns/query.h | 7 +-- lib/ns/query.c | 91 ++++++++++++++++----------------------- 2 files changed, 42 insertions(+), 56 deletions(-) diff --git a/lib/ns/include/ns/query.h b/lib/ns/include/ns/query.h index ed3ad846934..9fb6568837f 100644 --- a/lib/ns/include/ns/query.h +++ b/lib/ns/include/ns/query.h @@ -208,9 +208,8 @@ struct query_ctx { dns_fixedname_t wildcardname; /* name needing wcard proof */ dns_fixedname_t dsname; /* name needing DS */ - ns_client_t *client; /* client object */ - bool detach_client; /* client needs detaching */ - bool async; /* asynchronous hook running */ + ns_client_t *client; /* client object */ + bool async; /* asynchronous hook running */ dns_fetchresponse_t *fresp; /* recursion response */ @@ -230,6 +229,8 @@ struct query_ctx { dns_view_t *view; /* client view */ + bool allocated; /* qctx needs to be freed when destroying */ + isc_result_t result; /* query result */ int line; /* line to report error */ }; diff --git a/lib/ns/query.c b/lib/ns/query.c index 77419b77c7e..a89d6342e20 100644 --- a/lib/ns/query.c +++ b/lib/ns/query.c @@ -5202,9 +5202,13 @@ qctx_freedata(query_ctx_t *qctx) { static void qctx_destroy(query_ctx_t *qctx) { - CALL_HOOK_NORETURN(NS_QUERY_QCTX_DESTROYED, qctx); + if (qctx->view) { + dns_view_detach(&qctx->view); + } - dns_view_detach(&qctx->view); + if (qctx->allocated) { + isc_mem_put(qctx->client->manager->mctx, qctx, sizeof(*qctx)); + } } /* @@ -5224,33 +5228,40 @@ qctx_destroy(query_ctx_t *qctx) { * responsibility to do it if it's necessary. */ static void -qctx_save(query_ctx_t *src, query_ctx_t *tgt) { +qctx_save(query_ctx_t *src, query_ctx_t **targetp) { + query_ctx_t *target = isc_mem_get(src->client->manager->mctx, + sizeof(query_ctx_t)); + /* First copy all fields in a straightforward way */ - *tgt = *src; + *target = *src; /* Then "move" pointers (except client and view) */ - INITANDSAVE(tgt->dbuf, src->dbuf); - INITANDSAVE(tgt->fname, src->fname); - INITANDSAVE(tgt->tname, src->tname); - INITANDSAVE(tgt->rdataset, src->rdataset); - INITANDSAVE(tgt->sigrdataset, src->sigrdataset); - INITANDSAVE(tgt->noqname, src->noqname); - INITANDSAVE(tgt->fresp, src->fresp); - INITANDSAVE(tgt->db, src->db); - INITANDSAVE(tgt->version, src->version); - INITANDSAVE(tgt->node, src->node); - INITANDSAVE(tgt->zdb, src->zdb); - INITANDSAVE(tgt->znode, src->znode); - INITANDSAVE(tgt->zfname, src->zfname); - INITANDSAVE(tgt->zversion, src->zversion); - INITANDSAVE(tgt->zrdataset, src->zrdataset); - INITANDSAVE(tgt->zsigrdataset, src->zsigrdataset); - INITANDSAVE(tgt->rpz_st, src->rpz_st); - INITANDSAVE(tgt->zone, src->zone); + INITANDSAVE(target->dbuf, src->dbuf); + INITANDSAVE(target->fname, src->fname); + INITANDSAVE(target->tname, src->tname); + INITANDSAVE(target->rdataset, src->rdataset); + INITANDSAVE(target->sigrdataset, src->sigrdataset); + INITANDSAVE(target->noqname, src->noqname); + INITANDSAVE(target->fresp, src->fresp); + INITANDSAVE(target->db, src->db); + INITANDSAVE(target->version, src->version); + INITANDSAVE(target->node, src->node); + INITANDSAVE(target->zdb, src->zdb); + INITANDSAVE(target->znode, src->znode); + INITANDSAVE(target->zfname, src->zfname); + INITANDSAVE(target->zversion, src->zversion); + INITANDSAVE(target->zrdataset, src->zrdataset); + INITANDSAVE(target->zsigrdataset, src->zsigrdataset); + INITANDSAVE(target->rpz_st, src->rpz_st); + INITANDSAVE(target->zone, src->zone); /* View has to stay in 'src' for qctx_destroy. */ - tgt->view = NULL; - dns_view_attach(src->view, &tgt->view); + target->view = NULL; + dns_view_attach(src->view, &target->view); + + target->allocated = true; + + *targetp = target; } /*% @@ -5792,7 +5803,6 @@ async_restart(void *arg) { qctx_clean(qctx); qctx_freedata(qctx); qctx_destroy(qctx); - isc_mem_put(client->manager->mctx, qctx, sizeof(*qctx)); isc_nmhandle_detach(&handle); } @@ -6182,13 +6192,6 @@ fetch_callback(void *arg) { */ CTRACE(ISC_LOG_ERROR, "fetch cancelled"); query_error(client, DNS_R_SERVFAIL, __LINE__); - - /* - * Free any persistent plugin data that was allocated to - * service the client, then detach the client object. - */ - qctx.detach_client = true; - qctx_destroy(&qctx); } else { /* * Resume the find process. @@ -6208,10 +6211,9 @@ fetch_callback(void *arg) { errorloglevel, false); } } - - qctx_destroy(&qctx); } + qctx_destroy(&qctx); dns_resolver_destroyfetch(&fetch); } @@ -6643,13 +6645,6 @@ query_hookresume(void *arg) { */ qctx_clean(qctx); qctx_freedata(qctx); - - /* - * As we're almost done with this client, make sure any internal - * resource for hooks will be released (if necessary) via the - * QCTX_DESTROYED hook. - */ - qctx->detach_client = true; } else { switch (rev->hookpoint) { case NS_QUERY_SETUP: @@ -6727,7 +6722,6 @@ query_hookresume(void *arg) { isc_mem_put(hctx->mctx, rev, sizeof(*rev)); hctx->destroy(&hctx); qctx_destroy(qctx); - isc_mem_put(client->manager->mctx, qctx, sizeof(*qctx)); } isc_result_t @@ -6748,8 +6742,7 @@ ns_query_hookasync(query_ctx_t *qctx, ns_query_starthookasync_t runasync, goto cleanup; } - saved_qctx = isc_mem_get(client->manager->mctx, sizeof(*saved_qctx)); - qctx_save(qctx, saved_qctx); + qctx_save(qctx, &saved_qctx); result = runasync(saved_qctx, client->manager->mctx, arg, client->manager->loop, query_hookresume, client, &client->query.hookactx); @@ -6794,10 +6787,7 @@ cleanup: qctx_clean(saved_qctx); qctx_freedata(saved_qctx); qctx_destroy(saved_qctx); - isc_mem_put(client->manager->mctx, saved_qctx, - sizeof(*saved_qctx)); } - qctx->detach_client = true; return result; } @@ -11334,9 +11324,7 @@ ns_query_done(query_ctx_t *qctx) { { query_ctx_t *saved_qctx = NULL; qctx->client->query.restarts++; - saved_qctx = isc_mem_get(qctx->client->manager->mctx, - sizeof(*saved_qctx)); - qctx_save(qctx, saved_qctx); + qctx_save(qctx, &saved_qctx); isc_nmhandle_attach(qctx->client->inner.handle, &qctx->client->inner.restarthandle); isc_async_run(qctx->client->manager->loop, @@ -11390,7 +11378,6 @@ ns_query_done(query_ctx_t *qctx) { query_error(qctx->client, qctx->result, qctx->line); } - qctx->detach_client = true; return qctx->result; } @@ -11434,7 +11421,6 @@ ns_query_done(query_ctx_t *qctx) { CALL_HOOK(NS_QUERY_DONE_SEND, qctx); query_send(qctx->client); - qctx->detach_client = true; return qctx->result; @@ -11447,7 +11433,6 @@ cleanup: qctx_clean(qctx); qctx_freedata(qctx); if (!qctx->async) { - qctx->detach_client = true; query_error(qctx->client, DNS_R_SERVFAIL, __LINE__); } return result; -- 2.47.3