From 696506e1642af90d0e35d8714f02f1a128893c85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 6 Jun 2023 12:48:23 +0200 Subject: [PATCH 1/2] Fix extra detach when dns_validator create_fetch() detects deadlock When create_fetch() in the dns_validator unit detects deadlock, it returns DNS_R_NOVALIDSIG, but it didn't attach to the validator. The other condition to returning result != ISC_R_SUCCESS would be error from dns_resolver_createfetch(). The caller (in two places out of three) would detect the error condition and always detach from the validator. Move the dns_validator_detach() on dns_resolver_createfetch() error condition to create_fetch() function and cleanup the extra detaches in seek_dnskey() and get_dsset(). --- lib/dns/include/dns/validator.h | 6 +++++- lib/dns/validator.c | 37 +++++++++++++++------------------ 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/lib/dns/include/dns/validator.h b/lib/dns/include/dns/validator.h index 19eae8bb5a..d42cdca637 100644 --- a/lib/dns/include/dns/validator.h +++ b/lib/dns/include/dns/validator.h @@ -165,7 +165,11 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, /*%< * Start a DNSSEC validation. * - * This validates a response to the question given by + * On success (which is guaranteed as long as the view has valid + * trust anchors), `validatorp` is updated to point to the new + * validator. The caller is responsible for detaching it. + * + * The validator will validate a response to the question given by * 'name' and 'type'. * * To validate a positive response, the response data is diff --git a/lib/dns/validator.c b/lib/dns/validator.c index 617f03124e..3447d0482b 100644 --- a/lib/dns/validator.c +++ b/lib/dns/validator.c @@ -926,6 +926,7 @@ static isc_result_t create_fetch(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, isc_job_cb callback, const char *caller) { unsigned int fopts = 0; + isc_result_t result; disassociate_rdatasets(val); @@ -946,10 +947,16 @@ create_fetch(dns_validator_t *val, dns_name_t *name, dns_rdatatype_t type, validator_logcreate(val, name, type, caller, "fetch"); dns_validator_ref(val); - return (dns_resolver_createfetch( + + result = dns_resolver_createfetch( val->view->resolver, name, type, NULL, NULL, NULL, NULL, 0, fopts, 0, NULL, val->loop, callback, val, &val->frdataset, - &val->fsigrdataset, &val->fetch)); + &val->fsigrdataset, &val->fetch); + if (result != ISC_R_SUCCESS) { + dns_validator_detach(&val); + } + + return (result); } /*% @@ -1192,7 +1199,6 @@ seek_dnskey(dns_validator_t *val) { dns_rdatatype_dnskey, fetch_callback_dnskey, "seek_dnskey"); if (result != ISC_R_SUCCESS) { - dns_validator_detach(&val); return (result); } return (DNS_R_WAIT); @@ -1646,7 +1652,6 @@ get_dsset(dns_validator_t *val, dns_name_t *tname, isc_result_t *resp) { fetch_callback_ds, "validate_dnskey"); *resp = DNS_R_WAIT; if (result != ISC_R_SUCCESS) { - dns_validator_detach(&val); *resp = result; } return (ISC_R_COMPLETE); @@ -3004,12 +3009,18 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, dns_validator_t **validatorp) { isc_result_t result = ISC_R_FAILURE; dns_validator_t *val = NULL; + dns_keytable_t *kt = NULL; REQUIRE(name != NULL); REQUIRE(rdataset != NULL || (rdataset == NULL && sigrdataset == NULL && message != NULL)); REQUIRE(validatorp != NULL && *validatorp == NULL); + result = dns_view_getsecroots(view, &kt); + if (result != ISC_R_SUCCESS) { + return (result); + } + val = isc_mem_get(view->mctx, sizeof(*val)); *val = (dns_validator_t){ .tid = isc_tid(), .result = ISC_R_FAILURE, @@ -3018,22 +3029,17 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, .name = name, .type = type, .options = options, + .keytable = kt, .link = ISC_LINK_INITIALIZER, .loop = loop, .cb = cb, .arg = arg }; isc_refcount_init(&val->references, 1); - + dns_view_attach(view, &val->view); if (message != NULL) { dns_message_attach(message, &val->message); } - dns_view_attach(view, &val->view); - - result = dns_view_getsecroots(val->view, &val->keytable); - if (result != ISC_R_SUCCESS) { - goto cleanup; - } val->mustbesecure = dns_resolver_getmustbesecure(view->resolver, name); dns_rdataset_init(&val->fdsset); @@ -3052,15 +3058,6 @@ dns_validator_create(dns_view_t *view, dns_name_t *name, dns_rdatatype_t type, *validatorp = val; return (ISC_R_SUCCESS); - -cleanup: - if (val->message != NULL) { - dns_message_detach(&val->message); - } - isc_mem_put(view->mctx, val, sizeof(*val)); - dns_view_detach(&view); - - return (result); } void From 1d03cac78a492ac83e23c69e939c49ec1da8244a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ond=C5=99ej=20Sur=C3=BD?= Date: Tue, 6 Jun 2023 13:05:35 +0200 Subject: [PATCH 2/2] Add CHANGES note for [GL #4115] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index e822224060..4151a56666 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +6189. [bug] Fix an extra dns_validator deatch when encountering + deadling which would lead to assertion failure. + [GL #4115] + 6188. [performance] Reduce memory consumption by allocating properly sized send buffers for stream-based transports. [GL #4038]