yet another fix_iterator() bug

under some circumstances it was possible for the iterator to
be set to the first leaf in a set of twigs, when it should have
been set to the last.

a unit test has been added to test this scenario. if there is a
a tree containing the following values: {".", "abb.", "abc."}, and
we query for "acb.", previously the iterator would be positioned at
"abb." instead of "abc.".

the tree structure is:
    branch (offset 1, ".")
      branch (offset 3, ".ab")
        leaf (".abb")
        leaf (".abc")

we find the branch with offset 3 (indicating that its twigs differ
from each other in the third position of the label, "abB" vs "abC").
but the search key differs from the found keys at position 2
("aC" vs "aB").  we look up the bit value in position 3 of the
search key ("B"), and incorrectly follow it onto the wrong twig
("abB").

to correct for this, we need to check for the case where the search
key is greater than the found key in a position earlier than the
branch offset. if it is, then we need to pop from the current leaf
to its parent, and get the greatest leaf from there.

a further change is needed to ensure that we don't do this twice;
when we've moved to a new leaf and the point of difference between
it and the search key even earlier than before, then we're definitely
at a predecessor node and there's no need to continue the loop.

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index 686c3561f5f35eb9203490ac26678d2dabaad629..e381b7c324a27e2f9c5a31a88101a15bc2bba47c 100644 (file)
--- a/lib/dns/qp.c
+++ b/lib/dns/qp.c
@@ -2152,15 +2152,42 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start,
                        n = iter->stack[iter->sp];
                        leaf = n;
                } else {
+                       if (to <= searchlen && to <= foundlen && iter->sp > 0) {
+                               /*
+                                * If we're here, search[to] >= found[to],
+                                * meaning every leaf in this set of twigs
+                                * is less than the one we wanted. It's
+                                * possible we're already positioned at
+                                * the correct predecessor, but it's not
+                                * guaranteed, so we pop up to the parent
+                                * branch, and find the greatest leaf from
+                                * there.
+                                */
+                               if (!is_branch(n)) {
+                                       iter->stack[iter->sp--] = NULL;
+                                       n = iter->stack[iter->sp];
+                               }
+                       }
+
                        if (is_branch(n)) {
-                               iter->sp--;
+                               iter->stack[iter->sp--] = NULL;
                                n = greatest_leaf(qp, n, iter);
                        }
                        return (n);
                }
 
                foundlen = leaf_qpkey(qp, leaf, found);
-               to = qpkey_compare(search, searchlen, found, foundlen);
+
+               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 (leaf);
+               }
+               to = nto;
        }
 
        if (is_branch(n)) {
@@ -2293,8 +2320,7 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
                         */
                        n = fix_iterator(qp, iter, n, search, searchlen, bit,
                                         offset);
-                       iter->stack[iter->sp] = NULL;
-                       iter->sp--;
+                       iter->stack[iter->sp--] = NULL;
                } else {
                        /*
                         * this branch is a dead end, and the predecessor

diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c
index 6938cb9fd879f646a4f0ee923c04beed1ee71c7c..d49fe674b30685229f1262c7e8af34e01c64f850 100644 (file)
--- a/tests/dns/qp_test.c
+++ b/tests/dns/qp_test.c
@@ -765,29 +765,29 @@ ISC_RUN_TEST_IMPL(predecessors) {
  */
 ISC_RUN_TEST_IMPL(fixiterator) {
        dns_qp_t *qp = NULL;
-       const char insert[][32] = { "dynamic.",
-                                   "a.dynamic.",
-                                   "aaaa.dynamic.",
-                                   "cdnskey.dynamic.",
-                                   "cds.dynamic.",
-                                   "cname.dynamic.",
-                                   "dname.dynamic.",
-                                   "dnskey.dynamic.",
-                                   "ds.dynamic.",
-                                   "mx.dynamic.",
-                                   "ns.dynamic.",
-                                   "nsec.dynamic.",
-                                   "private-cdnskey.dynamic.",
-                                   "private-dnskey.dynamic.",
-                                   "rrsig.dynamic.",
-                                   "txt.dynamic.",
-                                   "trailing.",
-                                   "" };
+       const char insert1[][32] = { "dynamic.",
+                                    "a.dynamic.",
+                                    "aaaa.dynamic.",
+                                    "cdnskey.dynamic.",
+                                    "cds.dynamic.",
+                                    "cname.dynamic.",
+                                    "dname.dynamic.",
+                                    "dnskey.dynamic.",
+                                    "ds.dynamic.",
+                                    "mx.dynamic.",
+                                    "ns.dynamic.",
+                                    "nsec.dynamic.",
+                                    "private-cdnskey.dynamic.",
+                                    "private-dnskey.dynamic.",
+                                    "rrsig.dynamic.",
+                                    "txt.dynamic.",
+                                    "trailing.",
+                                    "" };
        int i = 0;
 
        dns_qp_create(mctx, &string_methods, NULL, &qp);
-       while (insert[i][0] != '\0') {
-               insert_str(qp, insert[i++]);
+       while (insert1[i][0] != '\0') {
+               insert_str(qp, insert1[i++]);
        }
 
        static struct check_predecessors check1[] = {
@@ -800,7 +800,25 @@ ISC_RUN_TEST_IMPL(fixiterator) {
        };
 
        check_predecessors(qp, check1);
+       dns_qp_destroy(&qp);
+
+       const char insert2[][64] = { ".", "abb.", "abc.", "" };
+       i = 0;
+
+       dns_qp_create(mctx, &string_methods, NULL, &qp);
+       while (insert2[i][0] != '\0') {
+               insert_str(qp, insert2[i++]);
+       }
+
+       static struct check_predecessors check2[] = {
+               { "acb.", "abc.", DNS_R_PARTIALMATCH, 0 },
+               { "acc.", "abc.", DNS_R_PARTIALMATCH, 0 },
+               { "abbb.", "abb.", DNS_R_PARTIALMATCH, 1 },
+               { "aab.", ".", DNS_R_PARTIALMATCH, 2 },
+               { NULL, NULL, 0, 0 }
+       };
 
+       check_predecessors(qp, check2);
        dns_qp_destroy(&qp);
 }