From 60a33ae6bb7ecdb59aaf78e8806c9ec129563765 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Mon, 4 Dec 2023 11:21:40 -0800 Subject: [PATCH] fix another dns_qp_lookup() iterator bug there was another edge case in which an iterator could be positioned at the wrong node after dns_qp_lookup(). when searching for a key, it's possible to reach a dead-end branch that doesn't match, because the branch offset point is *after* the point where the search key differs from the branch's contents. for example, if searching for the key "mop", we could reach a branch containing "moon" and "moor". the branch offset point - i.e., the point after which the branch's leaves differ from each other - is the fourth character ("n" or "r"). however, both leaves differ from the search key at position *three* ("o" or "p"). the old code failed to detect this condition, and would have incorrectly left the iterator pointing at some lower value and not at "moor". this has been fixed and the unit test now includes this scenario. --- lib/dns/qp.c | 86 +++++++++++++++++++++++++++++++++------------ tests/dns/qp_test.c | 26 ++++++++------ 2 files changed, 79 insertions(+), 33 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index f71f4dd925..3a6d7147ad 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2038,6 +2038,17 @@ prevleaf(dns_qpiter_t *it) { RUNTIME_CHECK(result == ISC_R_SUCCESS); } +static inline dns_qpnode_t * +greatest_leaf(dns_qpreadable_t qpr, dns_qpnode_t *n, dns_qpiter_t *iter) { + while (is_branch(n)) { + dns_qpref_t ref = branch_twigs_ref(n) + branch_twigs_size(n) - + 1; + iter->stack[++iter->sp] = n; + n = ref_ptr(qpr, ref); + } + return (n); +} + 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, @@ -2118,35 +2129,64 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, n = branch_twig_ptr(qp, n, bit); } else if (getpred) { /* - * this branch is a dead end, but the caller - * passed us an iterator: position it at the - * predecessor node. + * this branch is a dead end. however, the caller + * passed us an iterator, so we need to find the + * predecessor of the searched-for-name. + * first step: find out if we've overshot + * the search key; we do that by finding an + * arbitrary leaf to compare against. */ - dns_qpweight_t pos = branch_twig_pos(n, bit); - if (pos == 0) { + size_t to; + dns_qpnode_t *least = n; + while (is_branch(least)) { + least = branch_twigs(qp, least); + } + foundlen = leaf_qpkey(qp, least, found); + to = qpkey_compare(search, searchlen, found, foundlen); + if (to == offset) { /* - * this entire branch is greater than - * the key we wanted, so we step back to - * the predecessor using the iterator. + * we're on the right branch, so find + * the best match. */ - prevleaf(iter); - n = iter->stack[iter->sp]; - } else { + + dns_qpweight_t pos = branch_twig_pos(n, bit); + if (pos == 0) { + /* + * every leaf in the branch is greater + * than the one we wanted; use the + * iterator to walk back to the + * predecessor. + */ + prevleaf(iter); + n = iter->stack[iter->sp--]; + } else { + /* + * the name we want would've been + * after some twig in this + * branch. point n to that twig, + * then walk down to the highest + * leaf in that subtree to get the + * predecessor. + */ + n = greatest_leaf(qp, twigs + pos - 1, + iter); + } + } else if (to <= searchlen && to <= foundlen && + search[to] < found[to]) + { /* - * the name we want would've been between - * two twigs in this branch. point n to the - * lesser of those, then walk down to the - * highest leaf in that subtree to get the + * every leaf is greater than the one + * we wanted, so iterate back to the * predecessor. */ - n = twigs + pos - 1; - while (is_branch(n)) { - prefetch_twigs(qp, n); - iter->stack[++iter->sp] = n; - pos = branch_twigs_size(n) - 1; - n = ref_ptr(qp, - branch_twigs_ref(n) + pos); - } + prevleaf(iter); + n = iter->stack[iter->sp--]; + } else { + /* + * every leaf is less than the one we + * wanted, so get the highest. + */ + n = greatest_leaf(qp, n, iter); } } else { /* diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 28ea2266b7..b78281dfeb 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -635,11 +635,11 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { ISC_RUN_TEST_IMPL(predecessors) { dns_qp_t *qp = NULL; - 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.", - "b.a.", "x.k.c.d.", "" - }; + 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.", "b.a.", + "x.k.c.d.", "moog.", "mook.", + "moon.", "moops.", "" }; int i = 0; dns_qp_create(mctx, &string_methods, NULL, &qp); @@ -649,8 +649,8 @@ ISC_RUN_TEST_IMPL(predecessors) { /* first check: no root label in the database */ static struct check_predecessors check1[] = { - { ".", "a.b.c.d.e.", ISC_R_NOTFOUND }, - { "a.", "a.b.c.d.e.", ISC_R_SUCCESS }, + { ".", "moops.", ISC_R_NOTFOUND }, + { "a.", "moops.", ISC_R_SUCCESS }, { "b.a.", "a.", ISC_R_SUCCESS }, { "b.", "e.d.c.b.a.", ISC_R_SUCCESS }, { "aaa.a.", "a.", DNS_R_PARTIALMATCH }, @@ -658,13 +658,16 @@ ISC_RUN_TEST_IMPL(predecessors) { { "d.c.", "c.b.b.", ISC_R_NOTFOUND }, { "1.2.c.b.a.", "c.b.a.", DNS_R_PARTIALMATCH }, { "a.b.c.e.f.", "a.b.c.d.e.", ISC_R_NOTFOUND }, - { "z.y.x.", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "z.y.x.", "moops.", ISC_R_NOTFOUND }, { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, { "z.z.z.z.k.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, { "0.b.c.d.e.", "x.k.c.d.", ISC_R_NOTFOUND }, { "b.d.", "c.b.b.", ISC_R_NOTFOUND }, + { "mon.", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "moor.", "moops.", ISC_R_NOTFOUND }, + { "mop.", "moops.", ISC_R_NOTFOUND }, { NULL, NULL, 0 } }; @@ -675,7 +678,7 @@ ISC_RUN_TEST_IMPL(predecessors) { insert_str(qp, root); static struct check_predecessors check2[] = { - { ".", "a.b.c.d.e.", ISC_R_SUCCESS }, + { ".", "moops.", ISC_R_SUCCESS }, { "a.", ".", ISC_R_SUCCESS }, { "b.a.", "a.", ISC_R_SUCCESS }, { "b.", "e.d.c.b.a.", ISC_R_SUCCESS }, @@ -684,12 +687,15 @@ ISC_RUN_TEST_IMPL(predecessors) { { "d.c.", "c.b.b.", DNS_R_PARTIALMATCH }, { "1.2.c.b.a.", "c.b.a.", DNS_R_PARTIALMATCH }, { "a.b.c.e.f.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, - { "z.y.x.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, + { "z.y.x.", "moops.", DNS_R_PARTIALMATCH }, { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, { "z.z.z.z.k.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, { "0.b.c.d.e.", "x.k.c.d.", DNS_R_PARTIALMATCH }, + { "mon.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, + { "moor.", "moops.", DNS_R_PARTIALMATCH }, + { "mop.", "moops.", DNS_R_PARTIALMATCH }, { NULL, NULL, 0 } };