and fix yet another dns_qp_lookup() iterator bug

This one is similar to the bug when searching for a key, reaching a
dead-end branch that doesn't match, because the branch offset point
is after the point where the search key differs.

This fixes the case where we are multiple levels deep. In other
words, we had a more-than-one matches *after* the point where the
search key differs.

For example, consider the following qp-trie:

branch: "[e]", "[m]":
 - leaf: "a.b.c.d.e"
 - branch: "moo[g]", "moo[k]", "moo[n]":
   - leaf: "moog"
   - branch: "mook[e]", "mook[o]"
     - leaf: "mooker"
     - leaf: "mooko"
   - leaf: "moon"

If searching for a key "monky", we would reach the branch with
twigs "moo[k]" and "moo[n]". The key matches on the 'k' on offset=4,
and reaches the branch with twigs "mook[e]" and "mook[o]". This time
we cannot find a twig that matches our key at offset=5, there is no
twig for 'y'. The closest name we found was "mooker".

Note that on a branch it can't detect it is on a dead branch because the
key is not encapsulated in a branch node.

In the previous code we considered "mooker" to be the successor of
"monky" and so we needed to the predecessor of "mooker" to find the
predecessor for "monky". However, since the search key alread differed
before entering this branch, this is not enough. We would be left with
"moog" as the predecessor of "monky", while in this example "a.b.c.d.e"
is the actual predecessor.

Instead, we need to go up a level, find the predecessor and check
again if we are on the right branch, and repeat the process until we
are.

Unit tests to cover the scenario are now added.

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index bb212fdfba1457aa4db8bfc0e1780c433101502f..1fd36b91ba2e74e0c122062f269592dbcc8cb657 100644 (file)
--- a/lib/dns/qp.c
+++ b/lib/dns/qp.c
@@ -2139,12 +2139,14 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
                         */
                        size_t to;
                        dns_qpnode_t *least = n;
+
+               least:
                        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) {
+                       if (to >= offset) {
                                /*
                                 * we're on the right branch, so find
                                 * the best match.
@@ -2180,9 +2182,12 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
                                 * we wanted, so iterate back to the
                                 * predecessor.
                                 */
+                               iter->sp--;
                                prevleaf(iter);
-                               n = iter->stack[iter->sp--];
-                       } else {
+                               n = iter->stack[iter->sp];
+                               least = n;
+                               goto least;
+                       } else if (is_branch(n)) {
                                /*
                                 * every leaf is less than the one we
                                 * wanted, so get the highest.

diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c
index 44f8ded9b2bcc48ed38ee4166c1c0504849e8877..3889de5edaef8ebee1c714c1dc704bf1d275a05c 100644 (file)
--- a/tests/dns/qp_test.c
+++ b/tests/dns/qp_test.c
@@ -635,11 +635,12 @@ 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.",   "moog.",      "mook.",
-                                   "moon.",      "moops.",     "" };
+       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.",    "mooker.",
+               "mooko.", "moon.",    "moops.",   ""
+       };
        int i = 0;
 
        dns_qp_create(mctx, &string_methods, NULL, &qp);
@@ -669,14 +670,19 @@ ISC_RUN_TEST_IMPL(predecessors) {
                { "moor.", "moops.", ISC_R_NOTFOUND },
                { "mopbop.", "moops.", ISC_R_NOTFOUND },
                { "moppop.", "moops.", ISC_R_NOTFOUND },
+               { "mopps.", "moops.", ISC_R_NOTFOUND },
                { "mopzop.", "moops.", ISC_R_NOTFOUND },
                { "mop.", "moops.", ISC_R_NOTFOUND },
                { "monbop.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "monpop.", "a.b.c.d.e.", ISC_R_NOTFOUND },
+               { "monps.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "monzop.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "mon.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "moop.", "moon.", ISC_R_NOTFOUND },
                { "moopser.", "moops.", ISC_R_NOTFOUND },
+               { "monky.", "a.b.c.d.e.", ISC_R_NOTFOUND },
+               { "monkey.", "a.b.c.d.e.", ISC_R_NOTFOUND },
+               { "monker.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { NULL, NULL, 0 }
        };
 
@@ -706,14 +712,19 @@ ISC_RUN_TEST_IMPL(predecessors) {
                { "moor.", "moops.", DNS_R_PARTIALMATCH },
                { "mopbop.", "moops.", DNS_R_PARTIALMATCH },
                { "moppop.", "moops.", DNS_R_PARTIALMATCH },
+               { "mopps.", "moops.", DNS_R_PARTIALMATCH },
                { "mopzop.", "moops.", DNS_R_PARTIALMATCH },
                { "mop.", "moops.", DNS_R_PARTIALMATCH },
                { "monbop.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { "monpop.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
+               { "monps.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { "monzop.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { "mon.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { "moop.", "moon.", DNS_R_PARTIALMATCH },
                { "moopser.", "moops.", DNS_R_PARTIALMATCH },
+               { "monky.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
+               { "monkey.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
+               { "monker.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { NULL, NULL, 0 }
        };