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.

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index f71f4dd92566eea188212048cd45a9eb5be76e0d..3a6d7147ad9a8b8f3bbe59e6182f38ef92a697dc 100644 (file)
--- 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.
+                                */
+
+                               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])
+                       {
+                               /*
+                                * every leaf is greater than the one
+                                * we wanted, so iterate back to the
+                                * predecessor.
                                 */
                                prevleaf(iter);
-                               n = iter->stack[iter->sp];
+                               n = iter->stack[iter->sp--];
                        } else {
                                /*
-                                * 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
-                                * predecessor.
+                                * every leaf is less than the one we
+                                * wanted, so get the highest.
                                 */
-                               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);
-                               }
+                               n = greatest_leaf(qp, n, iter);
                        }
                } else {
                        /*

diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c
index 28ea2266b7afc1905571df7ddda72b9b3004e430..b78281dfeb36a8e8bfeb61f87f08fbd8ad0d26e7 100644 (file)
--- 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 }
        };