From 606232b8d5f28012fdfef870718bc1e6b5fd3ef1 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sat, 23 Sep 2023 01:02:17 -0700 Subject: [PATCH] remove DNS_QPFIND_NOEXACT since dns_qp_findname_ancestor() can now return a chain object, it is no longer necessary to provide a _NOEXACT search option. if we want to look up the closest ancestor of a name, we can just do a normal search, and if successful, retrieve the second-to-last node from the QP chain. this makes ancestor lookups slightly more complicated for the caller, but allows us to simplify the code in dns_qp_findname_ancestor(), making it easier to ensure correctness. this was a fairly rare use case: outside of unit tests, DNS_QPFIND_NOEXACT was only used in the zone table, which has now been updated to use the QP chain. the equivalent RBT feature is only used by the resolver for cache lookups of 'atparent' types (i.e, DS records). --- lib/dns/forward.c | 3 +- lib/dns/include/dns/qp.h | 14 +---- lib/dns/keytable.c | 6 +- lib/dns/nametree.c | 2 +- lib/dns/nta.c | 3 +- lib/dns/qp.c | 39 ++---------- lib/dns/zt.c | 23 ++++--- tests/dns/qp_test.c | 127 +++++++++++++++------------------------ 8 files changed, 73 insertions(+), 144 deletions(-) diff --git a/lib/dns/forward.c b/lib/dns/forward.c index d885df68b4..fc21a1a4ce 100644 --- a/lib/dns/forward.c +++ b/lib/dns/forward.c @@ -168,8 +168,7 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name, REQUIRE(VALID_FWDTABLE(fwdtable)); dns_qpmulti_query(fwdtable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { dns_forwarders_t *fwdrs = pval; *forwardersp = fwdrs; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 86ddbd3f45..51c46cf964 100644 --- a/lib/dns/include/dns/qp.h +++ b/lib/dns/include/dns/qp.h @@ -281,13 +281,6 @@ typedef enum dns_qpgc { DNS_QPGC_ALL, } dns_qpgc_t; -/*% - * Options for fancy searches such as `dns_qp_findname_ancestor()` - */ -typedef enum dns_qpfind { - DNS_QPFIND_NOEXACT = 1 << 0, -} dns_qpfind_t; - /*********************************************************************** * * functions - create, destory, enquire @@ -495,15 +488,12 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r, isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_qpfind_t options, dns_name_t *foundname, - dns_qpchain_t *chain, void **pval_r, uint32_t *ival_r); + dns_name_t *foundname, dns_qpchain_t *chain, + void **pval_r, uint32_t *ival_r); /*%< * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the * given DNS name. * - * If the DNS_QPFIND_NOEXACT option is set, find the closest ancestor - * domain that is not equal to the search name. - * * If 'foundname' is not NULL, it is updated to contain the name found. * * If 'chain' is not NULL, it is updated to contain a QP chain with diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 12866f9d9f..0b805fd9c4 100644 --- a/lib/dns/keytable.c +++ b/lib/dns/keytable.c @@ -518,8 +518,7 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(foundname != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); keynode = pval; if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { @@ -548,8 +547,7 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name, REQUIRE(wantdnssecp != NULL); dns_qpmulti_query(keytable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { keynode = pval; if (foundname != NULL) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 0fd1110a16..661253f3aa 100644 --- a/lib/dns/nametree.c +++ b/lib/dns/nametree.c @@ -289,7 +289,7 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name, REQUIRE(VALID_NAMETREE(nametree)); dns_qpmulti_query(nametree->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, found, NULL, + result = dns_qp_findname_ancestor(&qpr, name, found, NULL, (void **)&node, NULL); if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) { switch (nametree->type) { diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 51364142ea..4223ec27f5 100644 --- a/lib/dns/nta.c +++ b/lib/dns/nta.c @@ -413,8 +413,7 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now, RWLOCK(&ntatable->rwlock, isc_rwlocktype_read); dns_qpmulti_query(ntatable->table, &qpr); - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, &pval, - NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, NULL, &pval, NULL); nta = pval; switch (result) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 163793c6c7..3f25e8bae6 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -360,25 +360,6 @@ qpkey_compare(const dns_qpkey_t key_a, const size_t keylen_a, return (QPKEY_EQUAL); } -/* - * Given a key constructed by dns_qpkey_fromname(), trim it down to the last - * label boundary before the `max` length. - * - * This is used when searching a trie for the best match for a name. - */ -static size_t -qpkey_trim_label(dns_qpkey_t key, size_t len, size_t max) { - size_t stop = 0; - for (size_t offset = 0; offset < max; offset++) { - if (qpkey_bit(key, len, offset) == SHIFT_NOBYTE && - qpkey_bit(key, len, offset + 1) != SHIFT_NOBYTE) - { - stop = offset + 1; - } - } - return (stop); -} - /*********************************************************************** * * allocator wrappers @@ -2023,27 +2004,19 @@ add_link(dns_qpchain_t *chain, qp_node_t *node, size_t offset) { isc_result_t dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, - dns_qpfind_t options, dns_name_t *foundname, - dns_qpchain_t *chain, void **pval_r, - uint32_t *ival_r) { + dns_name_t *foundname, dns_qpchain_t *chain, + void **pval_r, uint32_t *ival_r) { dns_qpreader_t *qp = dns_qpreader(qpr); dns_qpkey_t search, found; size_t searchlen, foundlen; size_t offset; qp_node_t *n = NULL; - isc_result_t result; dns_qpchain_t oc; REQUIRE(QP_VALID(qp)); REQUIRE(foundname == NULL || ISC_MAGIC_VALID(name, DNS_NAME_MAGIC)); searchlen = dns_qpkey_fromname(search, name); - if ((options & DNS_QPFIND_NOEXACT) != 0) { - searchlen = qpkey_trim_label(search, searchlen, searchlen); - result = DNS_R_PARTIALMATCH; - } else { - result = ISC_R_SUCCESS; - } if (chain == NULL) { chain = &oc; @@ -2121,11 +2094,9 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name, SET_IF_NOT_NULL(ival_r, leaf_ival(n)); maybe_set_name(qp, n, foundname); if (offset == QPKEY_EQUAL) { - if ((options & DNS_QPFIND_NOEXACT) == 0) { - /* add the exact match to the chain */ - add_link(chain, n, offset); - } - return (result); + /* add the exact match to the chain */ + add_link(chain, n, offset); + return (ISC_R_SUCCESS); } else { return (DNS_R_PARTIALMATCH); } diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 60e5d52a08..7d317506fb 100644 --- a/lib/dns/zt.c +++ b/lib/dns/zt.c @@ -171,21 +171,26 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options, void *pval = NULL; dns_ztfind_t exactmask = DNS_ZTFIND_NOEXACT | DNS_ZTFIND_EXACT; dns_ztfind_t exactopts = options & exactmask; + dns_qpchain_t chain; REQUIRE(VALID_ZT(zt)); REQUIRE(exactopts != exactmask); dns_qpmulti_query(zt->multi, &qpr); - if (exactopts == DNS_ZTFIND_EXACT) { - result = dns_qp_getname(&qpr, name, &pval, NULL); - } else if (exactopts == DNS_ZTFIND_NOEXACT) { - result = dns_qp_findname_ancestor(&qpr, name, - DNS_QPFIND_NOEXACT, NULL, - NULL, &pval, NULL); - } else { - result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, NULL, - &pval, NULL); + result = dns_qp_findname_ancestor(&qpr, name, NULL, &chain, &pval, + NULL); + if (exactopts == DNS_ZTFIND_EXACT && result == DNS_R_PARTIALMATCH) { + result = ISC_R_NOTFOUND; + } else if (exactopts == DNS_ZTFIND_NOEXACT && result == ISC_R_SUCCESS) { + /* get pval from the previous chain link */ + int len = dns_qpchain_length(&chain); + if (len >= 2) { + dns_qpchain_node(&chain, len - 2, NULL, &pval, NULL); + result = DNS_R_PARTIALMATCH; + } else { + result = ISC_R_NOTFOUND; + } } dns_qpread_destroy(zt->multi, &qpr); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 8f05cfbcdf..782383d1a3 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -335,7 +335,6 @@ const dns_qpmethods_t string_methods = { struct check_partialmatch { const char *query; - dns_qpfind_t options; isc_result_t result; const char *found; }; @@ -350,13 +349,13 @@ check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) { void *pval = NULL; dns_test_namefromstring(check[i].query, &fn1); - result = dns_qp_findname_ancestor(qp, name, check[i].options, - foundname, NULL, &pval, NULL); + result = dns_qp_findname_ancestor(qp, name, foundname, NULL, + &pval, NULL); #if 0 - fprintf(stderr, "%s (flags %u) %s (expected %s) " + fprintf(stderr, "%s %s (expected %s) " "value \"%s\" (expected \"%s\")\n", - check[i].query, check[i].options, + check[i].query, isc_result_totext(result), isc_result_totext(check[i].result), (char *)pval, check[i].found); @@ -426,26 +425,19 @@ ISC_RUN_TEST_IMPL(partialmatch) { } static struct check_partialmatch check1[] = { - { "a.b.", 0, ISC_R_SUCCESS, "a.b." }, - { "a.b.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "b." }, - { "b.c.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, - { "bar.", 0, ISC_R_NOTFOUND, NULL }, - { "f.bar.", 0, ISC_R_NOTFOUND, NULL }, - { "foo.bar.", 0, ISC_R_SUCCESS, "foo.bar." }, - { "foo.bar.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, - { "foooo.bar.", 0, ISC_R_NOTFOUND, NULL }, - { "w.foo.bar.", 0, DNS_R_PARTIALMATCH, "foo.bar." }, - { "www.foo.bar.", 0, DNS_R_PARTIALMATCH, "foo.bar." }, - { "web.foo.bar.", 0, ISC_R_SUCCESS, "web.foo.bar." }, - { "webby.foo.bar.", 0, DNS_R_PARTIALMATCH, "foo.bar." }, - { "my.web.foo.bar.", 0, DNS_R_PARTIALMATCH, "web.foo.bar." }, - { "web.foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, - "foo.bar." }, - { "my.web.foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, - "web.foo.bar." }, - { "my.other.foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, - "foo.bar." }, - { NULL, 0, 0, NULL }, + { "a.b.", ISC_R_SUCCESS, "a.b." }, + { "b.c.", ISC_R_NOTFOUND, NULL }, + { "bar.", ISC_R_NOTFOUND, NULL }, + { "f.bar.", ISC_R_NOTFOUND, NULL }, + { "foo.bar.", ISC_R_SUCCESS, "foo.bar." }, + { "foooo.bar.", ISC_R_NOTFOUND, NULL }, + { "w.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { "www.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { "web.foo.bar.", ISC_R_SUCCESS, "web.foo.bar." }, + { "webby.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { "my.web.foo.bar.", DNS_R_PARTIALMATCH, "web.foo.bar." }, + { "my.other.foo.bar.", DNS_R_PARTIALMATCH, "foo.bar." }, + { NULL }, }; check_partialmatch(qp, check1); @@ -454,13 +446,11 @@ ISC_RUN_TEST_IMPL(partialmatch) { insert_str(qp, insert[i++]); static struct check_partialmatch check2[] = { - { "b.c.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, - { "bar.", 0, DNS_R_PARTIALMATCH, "." }, - { "foo.bar.", 0, ISC_R_SUCCESS, "foo.bar." }, - { "foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, - { "bar", 0, ISC_R_NOTFOUND, NULL }, - { "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." }, - { NULL, 0, 0, NULL }, + { "b.c.", DNS_R_PARTIALMATCH, "." }, + { "bar.", DNS_R_PARTIALMATCH, "." }, + { "foo.bar.", ISC_R_SUCCESS, "foo.bar." }, + { "bar", ISC_R_NOTFOUND, NULL }, + { NULL }, }; check_partialmatch(qp, check2); @@ -471,35 +461,26 @@ ISC_RUN_TEST_IMPL(partialmatch) { dns_qpkey_t rootkey = { SHIFT_NOBYTE }; result = dns_qp_deletekey(qp, rootkey, 1, NULL, NULL); assert_int_equal(result, ISC_R_SUCCESS); - check_partialmatch( - qp, - (struct check_partialmatch[]){ - { "bar", 0, ISC_R_NOTFOUND, NULL }, - { "bar", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, - { "bar.", 0, ISC_R_NOTFOUND, NULL }, - { "bar.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL }, - { NULL, 0, 0, NULL }, - }); + check_partialmatch(qp, (struct check_partialmatch[]){ + { "bar", ISC_R_NOTFOUND, NULL }, + { "bar.", ISC_R_NOTFOUND, NULL }, + { NULL }, + }); /* what if there's a root node with an empty key? */ INSIST(insert[i][0] == '\0'); insert_str(qp, insert[i++]); - check_partialmatch( - qp, - (struct check_partialmatch[]){ - { "bar", 0, DNS_R_PARTIALMATCH, "" }, - { "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" }, - { "bar.", 0, DNS_R_PARTIALMATCH, "" }, - { "bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" }, - { NULL, 0, 0, NULL }, - }); + check_partialmatch(qp, (struct check_partialmatch[]){ + { "bar", DNS_R_PARTIALMATCH, "" }, + { "bar.", DNS_R_PARTIALMATCH, "" }, + { NULL }, + }); dns_qp_destroy(&qp); } struct check_qpchain { const char *query; - dns_qpfind_t options; isc_result_t result; unsigned int length; const char *names[10]; @@ -515,13 +496,12 @@ 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_findname_ancestor(qp, name, check[i].options, - NULL, &chain, NULL, NULL); + result = dns_qp_findname_ancestor(qp, name, NULL, &chain, NULL, + NULL); #if 0 - fprintf(stderr, "%s (%d) %s (expected %s), " - "len %d (expected %d)\n", - check[i].query, check[i].options, + fprintf(stderr, "%s %s (expected %s), " + "len %d (expected %d)\n", check[i].query, isc_result_totext(result), isc_result_totext(check[i].result), dns_qpchain_length(&chain), check[i].length); @@ -557,38 +537,25 @@ ISC_RUN_TEST_IMPL(qpchain) { */ const char insert[][16] = { ".", "a.", "b.", "c.b.a.", "e.d.c.b.a.", "c.b.b.", "c.d.", "a.b.c.d.", - "a.b.c.d.e.", "" }; + "a.b.c.d.e.", "b.a.", "" }; while (insert[i][0] != '\0') { insert_str(qp, insert[i++]); } static struct check_qpchain check1[] = { - { "b.", 0, ISC_R_SUCCESS, 2 }, - { "c.", 0, DNS_R_PARTIALMATCH, 1 }, - { "e.d.c.b.a.", 0, ISC_R_SUCCESS, 4 }, - { "a.b.c.d.", 0, ISC_R_SUCCESS, 3 }, - { "a.b.c.d.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, 2 }, - { NULL, 0, 0, 0 }, + { "b.", ISC_R_SUCCESS, 2, { ".", "b." } }, + { "b.a.", ISC_R_SUCCESS, 3, { ".", "a.", "b.a." } }, + { "c.", DNS_R_PARTIALMATCH, 1, { "." } }, + { "e.d.c.b.a.", + ISC_R_SUCCESS, + 5, + { ".", "a.", "b.a.", "c.b.a.", "e.d.c.b.a." } }, + { "a.b.c.d.", ISC_R_SUCCESS, 3, { ".", "c.d.", "a.b.c.d." } }, + { "b.c.d.", DNS_R_PARTIALMATCH, 2, { ".", "c.d." } }, + { NULL, 0, 0, { NULL } }, }; - check1[0].names[0] = "."; - check1[0].names[1] = "b."; - - check1[1].names[0] = "."; - - check1[2].names[0] = "."; - check1[2].names[1] = "a."; - check1[2].names[2] = "c.b.a."; - check1[2].names[3] = "e.d.c.b.a."; - - check1[3].names[0] = "."; - check1[3].names[1] = "c.d."; - check1[3].names[2] = "a.b.c.d."; - - check1[4].names[0] = "."; - check1[4].names[1] = "c.d."; - check_qpchain(qp, check1); dns_qp_destroy(&qp); }