From a6c6ad048da64a7435ec3fea8cb69f7431084ee0 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 25 Mar 2024 14:20:24 +0000 Subject: [PATCH 1/3] Remove a redundant log message and a comment If val->result is not ISC_R_SUCCESS, a similar message is logged further down in the function. Remove the redundant log message. Also remove an unnecessary code comment line. --- lib/dns/validator.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 5681c89a36..d80c22b04c 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -1638,11 +1638,7 @@ validate_answer_finish(void *arg) { dns_validator_t *val = arg; isc_result_t result = ISC_R_UNSET; - if (val->result != ISC_R_SUCCESS) { - validator_log(val, ISC_LOG_DEBUG(3), - "failed to verify rdataset: %s", - isc_result_totext(val->result)); - } else { + if (val->result == ISC_R_SUCCESS) { dns_rdataset_trimttl(val->rdataset, val->sigrdataset, val->siginfo, val->start, val->view->acceptexpired); @@ -1659,7 +1655,6 @@ validate_answer_finish(void *arg) { switch (val->result) { case ISC_R_CANCELED: - /* The validation was canceled */ validator_log(val, ISC_LOG_DEBUG(3), "validation was canceled"); validate_async_done(val, val->result); return; From a5ea7bcd257f6f314d5ac34f49245526c4538371 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 25 Mar 2024 14:35:20 +0000 Subject: [PATCH 2/3] Rename and fix dns_validator_destroy() to dns_validator_shutdown() Since the dns_validator_destroy() function doesn't guarantee that it destroys the validator, rename it to dns_validator_shutdown() and require explicit dns_validator_detach() to follow. Enforce the documented function requirement that the validator must be completed when the function is called. Make sure to set val->name to NULL when the function is called, so that the owner of the validator may destroy the name, even if the validator is not destroyed immediately. This should be safe, because the name can be used further only for logging by the offloaded work callbacks when they detect that the validator is already canceled/complete, and the logging function has a condition to use the name only when it is non-NULL. --- lib/dns/include/dns/validator.h | 8 ++++---- lib/dns/resolver.c | 7 ++++--- lib/dns/validator.c | 33 +++++++++++++++++++-------------- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 0b0222c5c6..c68c5555b9 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -235,17 +235,17 @@ dns_validator_cancel(dns_validator_t *validator); */ void -dns_validator_destroy(dns_validator_t **validatorp); +dns_validator_shutdown(dns_validator_t *val); /*%< - * Destroy a DNSSEC validator. + * Release the name associated with the DNSSEC validator. * * Requires: - *\li '*validatorp' points to a valid DNSSEC validator. + * \li 'val' points to a valid DNSSEC validator. * \li The validator must have completed and sent its completion * event. * * Ensures: - *\li All resources used by the validator are freed. + *\li The name associated with the DNSSEC validator is released. */ #if DNS_VALIDATOR_TRACE diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index c30cea1724..6fe8ba448b 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -5557,10 +5557,11 @@ cleanup_fetchctx: /* * val->name points to name on a message on one of the - * queries on the fetch context so the validator has to - * be destroyed first. + * queries on the fetch context so the name has to be + * released first with a dns_validator_shutdown() call. */ - dns_validator_destroy(&val); + dns_validator_shutdown(val); + dns_validator_detach(&val); fetchctx_detach(&fctx); INSIST(node == NULL); } diff --git a/lib/dns/validator.c b/lib/dns/validator.c index d80c22b04c..2ba8ce77d0 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -614,7 +614,8 @@ validator_callback_dnskey(void *arg) { cleanup: dns_validator_detach(&subvalidator->parent); - dns_validator_destroy(&subvalidator); + dns_validator_shutdown(subvalidator); + dns_validator_detach(&subvalidator); validate_async_done(val, result); } @@ -672,7 +673,8 @@ validator_callback_ds(void *arg) { cleanup: dns_validator_detach(&subvalidator->parent); - dns_validator_destroy(&subvalidator); + dns_validator_shutdown(subvalidator); + dns_validator_detach(&subvalidator); validate_async_done(val, result); } @@ -714,7 +716,8 @@ validator_callback_cname(void *arg) { cleanup: dns_validator_detach(&subvalidator->parent); - dns_validator_destroy(&subvalidator); + dns_validator_shutdown(subvalidator); + dns_validator_detach(&subvalidator); validate_async_done(val, result); } @@ -813,7 +816,8 @@ validator_callback_nsec(void *arg) { cleanup: dns_validator_detach(&subvalidator->parent); - dns_validator_destroy(&subvalidator); + dns_validator_shutdown(subvalidator); + dns_validator_detach(&subvalidator); validate_async_done(val, result); } @@ -3427,20 +3431,21 @@ destroy_validator(dns_validator_t *val) { } void -dns_validator_destroy(dns_validator_t **validatorp) { - dns_validator_t *val = NULL; - - REQUIRE(validatorp != NULL); - - val = *validatorp; - *validatorp = NULL; - +dns_validator_shutdown(dns_validator_t *val) { REQUIRE(VALID_VALIDATOR(val)); + REQUIRE(COMPLETE(val)); REQUIRE(val->tid == isc_tid()); - validator_log(val, ISC_LOG_DEBUG(4), "dns_validator_destroy"); + validator_log(val, ISC_LOG_DEBUG(4), "dns_validator_shutdown"); - dns_validator_detach(&val); + /* + * The validation is now complete and the owner is no longer interested + * in any further results. If there are still callback events queued up + * which hold a validator reference, they should not be allowed to use + * val->name during logging, because the owner may destroy it after this + * function is called. + */ + val->name = NULL; } static void From 88d826ac5d0719a43b25c528b3571ca7a0b1df1e Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 2 Apr 2024 10:43:35 +0000 Subject: [PATCH 3/3] Add a CHANGES note for [GL #4654] --- CHANGES | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES b/CHANGES index ddf889645e..bd72ded702 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,10 @@ +6367. [bug] Since the dns_validator_destroy() function doesn't + guarantee that it destroys the validator, rename it to + dns_validator_shutdown() and require explicit + dns_validator_detach() to follow. Implement an expected + behavior of the function to release a name associated + with the validator. [GL #4654] + 6366. [bug] An assertion could be triggered in the QPDB cache when encountering a delegation below a DNAME. [GL #4652]