From 91de4f6490df803352d482fd1c2e164d3d487a33 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Fri, 10 May 2024 17:19:15 +0200 Subject: [PATCH 1/4] Refactor fix_iterator The code below the if/else construction could only be run if the 'if' code path was taken. Move the code into the 'if' code block so that it is more easier to read. --- lib/dns/qp.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index e8ff8f66a7..3f8b0ec293 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2148,6 +2148,19 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, RUNTIME_CHECK(result == ISC_R_SUCCESS); n = iter->stack[iter->sp]; + + foundlen = leaf_qpkey(qp, n, found); + size_t nto = qpkey_compare(search, searchlen, found, + foundlen); + if (nto < to) { + /* + * We've moved to a new leaf and it differs at + * an even earlier point, so no further + * improvement is possible. + */ + return; + } + to = nto; } else { if (to <= searchlen && to <= foundlen && iter->sp > 0) { /* @@ -2187,18 +2200,6 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, return; } - - foundlen = leaf_qpkey(qp, n, found); - size_t nto = qpkey_compare(search, searchlen, found, foundlen); - if (nto < to) { - /* - * We've moved to a new leaf and it differs at an - * even earlier point, so no further improvement is - * possible. - */ - return; - } - to = nto; } if (is_branch(n)) { From 5d96f11693b144e7dc692568a45e55b48341e6e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Petr=20=C5=A0pa=C4=8Dek?= Date: Fri, 10 May 2024 13:34:10 +0200 Subject: [PATCH 2/4] Unit test to demonstrate issue #4717 Iterator in lookup() call must be non-NULL to trigger the issue. Run chain tests twice, once without iterator and second time with iterator. --- tests/dns/qp_test.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 9cfab6509b..32d855543d 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -520,7 +520,8 @@ struct check_qpchain { }; static void -check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { +check_qpchainiter(dns_qp_t *qp, struct check_qpchain check[], + dns_qpiter_t *iter) { for (int i = 0; check[i].query != NULL; i++) { isc_result_t result; dns_fixedname_t fn1; @@ -529,9 +530,8 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { dns_qpchain_init(qp, &chain); dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_lookup(qp, name, NULL, NULL, &chain, NULL, + result = dns_qp_lookup(qp, name, NULL, iter, &chain, NULL, NULL); - #if 0 fprintf(stderr, "%s %s (expected %s), " "len %d (expected %d)\n", check[i].query, @@ -539,13 +539,13 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { isc_result_totext(check[i].result), dns_qpchain_length(&chain), check[i].length); #endif + assert_int_equal(result, check[i].result); assert_int_equal(dns_qpchain_length(&chain), check[i].length); for (unsigned int j = 0; j < check[i].length; j++) { dns_fixedname_t fn2, fn3; dns_name_t *expected = dns_fixedname_initname(&fn2); dns_name_t *found = dns_fixedname_initname(&fn3); - dns_test_namefromstring(check[i].names[j], &fn2); dns_qpchain_node(&chain, j, found, NULL, NULL); #if 0 @@ -559,6 +559,14 @@ check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { } } +static void +check_qpchain(dns_qp_t *qp, struct check_qpchain check[]) { + dns_qpiter_t iter; + dns_qpiter_init(qp, &iter); + check_qpchainiter(qp, check, NULL); + check_qpchainiter(qp, check, &iter); +} + ISC_RUN_TEST_IMPL(qpchain) { dns_qp_t *qp = NULL; const char insert[][16] = { ".", "a.", "b.", @@ -592,6 +600,24 @@ ISC_RUN_TEST_IMPL(qpchain) { check_qpchain(qp, check1); dns_qp_destroy(&qp); + + const char insert2[][16] = { "a.", "d.b.a.", "z.d.b.a.", "" }; + + i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + + while (insert2[i][0] != '\0') { + insert_str(qp, insert2[i++]); + } + + static struct check_qpchain check2[] = { + { "f.c.b.a.", DNS_R_PARTIALMATCH, 1, { "a." } }, + { NULL, 0, 0, { NULL } }, + }; + + check_qpchain(qp, check2); + dns_qp_destroy(&qp); } struct check_predecessors { From b6815de316958bba570403b8ae95c71e9915af6c Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 13 May 2024 15:56:15 -0700 Subject: [PATCH 3/4] Fix QP chain on partial match When searching for a requested name in dns_qp_lookup(), we may add a leaf node to the QP chain, then subsequently determine that the branch we were on was a dead end. When that happens, the chain can be left holding a pointer to a node that is *not* an ancestor of the requested name. We correct for this by unwinding any chain links with an offset value greater or equal to that of the node we found. --- lib/dns/qp.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 3f8b0ec293..3b73f4755b 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2241,6 +2241,23 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, } } +/* + * When searching for a requested name in dns_qp_lookup(), we might add + * a leaf node to the chain, then subsequently determine that it was a + * dead end. When this happens, the chain can be left holding a node + * that is *not* an ancestor of the requested name. We correct for that + * here. + */ +static void +fix_chain(dns_qpchain_t *chain, size_t offset) { + while (chain->len > 0 && chain->chain[chain->len - 1].offset >= offset) + { + chain->len--; + chain->chain[chain->len].node = NULL; + chain->chain[chain->len].offset = 0; + } +} + isc_result_t dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, dns_name_t *foundname, dns_qpiter_t *iter, dns_qpchain_t *chain, @@ -2364,16 +2381,20 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, foundlen = leaf_qpkey(qp, n, found); offset = qpkey_compare(search, searchlen, found, foundlen); + /* the search ended with an exact or partial match */ if (offset == QPKEY_EQUAL || offset == foundlen) { + isc_result_t result = ISC_R_SUCCESS; + + if (offset == foundlen) { + fix_chain(chain, offset); + result = DNS_R_PARTIALMATCH; + } + add_link(chain, n, offset); + SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); maybe_set_name(qp, n, foundname); - add_link(chain, n, offset); - if (offset == QPKEY_EQUAL) { - return (ISC_R_SUCCESS); - } else { - return (DNS_R_PARTIALMATCH); - } + return (result); } /* From 730e532cac7c3ef95cdb96a8defdaf453fdaa767 Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Tue, 14 May 2024 08:55:42 +0200 Subject: [PATCH 4/4] Test check_predecessors with chain In addition to testing check_qpchain in combination with qpiter, test check_predecessors in combination with a qpchain. --- tests/dns/qp_test.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 32d855543d..19ffc7f3d9 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -628,7 +628,8 @@ struct check_predecessors { }; static void -check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { +check_predecessors_withchain(dns_qp_t *qp, struct check_predecessors check[], + dns_qpchain_t *chain) { isc_result_t result; dns_fixedname_t fn1, fn2; dns_name_t *name = dns_fixedname_initname(&fn1); @@ -652,7 +653,7 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { result = dns_name_tostring(expred, &predstr, mctx); assert_int_equal(result, ISC_R_SUCCESS); - result = dns_qp_lookup(qp, name, NULL, &it, NULL, NULL, NULL); + result = dns_qp_lookup(qp, name, NULL, &it, chain, NULL, NULL); #if 0 fprintf(stderr, "%s: expected %s got %s\n", check[i].query, isc_result_totext(check[i].result), @@ -711,6 +712,14 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { } } +static void +check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { + dns_qpchain_t chain; + dns_qpchain_init(qp, &chain); + check_predecessors_withchain(qp, check, NULL); + check_predecessors_withchain(qp, check, &chain); +} + ISC_RUN_TEST_IMPL(predecessors) { dns_qp_t *qp = NULL; const char insert[][16] = {