and fix another dns_qp_lookup() iterator bug

There was yet 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 leaf that matches at the given offset,
but because the offset point is *after* the point where the search key
differs from the leaf's contents, we are now at the wrong leaf.

In other words, the bug fixed the previous commit for dead-end branches
must also be applied on matched leaves.

For example, if searching for the key "monpop", we could reach a branch
containing "moop" and "moor". the branch offset point - i.e., the point
after which the branch's leaves differ from each other - is the
fourth character ("p" or "r"). The search key matches the fourth
character "p", and takes that twig to the next node (which can be
a branch for names starting with "moop", or could be a leaf node for
"moop").

The old code failed to detect this condition, and would have
incorrectly left the iterator pointing at some successor, and not
at the predecessor of the "moop".

To find the right predecessor in this case, we need to get to the
previous branch and get the previous from there.

This has been fixed and the unit test now includes several new
scenarios for testing search names that match and unmatch on the
offset but have a different character before the offset.

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index 3a6d7147ad9a8b8f3bbe59e6182f38ef92a697dc..bb212fdfba1457aa4db8bfc0e1780c433101502f 100644 (file)
--- a/lib/dns/qp.c
+++ b/lib/dns/qp.c
@@ -2056,7 +2056,8 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
        dns_qpreader_t *qp = dns_qpreader(qpr);
        dns_qpkey_t search, found;
        size_t searchlen, foundlen;
-       size_t offset;
+       size_t offset = 0;
+       size_t difpos = 0;
        dns_qpnode_t *n = NULL;
        dns_qpchain_t oc;
        dns_qpiter_t it;
@@ -2211,7 +2212,7 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
 
        /* do the keys differ, and if so, where? */
        foundlen = leaf_qpkey(qp, n, found);
-       offset = qpkey_compare(search, searchlen, found, foundlen);
+       difpos = qpkey_compare(search, searchlen, found, foundlen);
 
        /*
         * if we've been passed an iterator, we want it to point
@@ -2230,21 +2231,36 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
         * node, and we would already have positioned the iterator
         * at the predecessor.
         */
-       if (getpred && matched) {
-               if (offset != QPKEY_EQUAL &&
-                   (offset <= searchlen && offset <= foundlen &&
-                    found[offset] > search[offset]))
+       if (getpred && matched && difpos != QPKEY_EQUAL) {
+               while (difpos < offset) {
+                       if (difpos <= searchlen && difpos <= foundlen &&
+                           search[difpos] < found[difpos])
+                       {
+                               iter->sp--;
+                               prevleaf(iter);
+                               n = iter->stack[iter->sp];
+                       } else {
+                               break;
+                       }
+
+                       foundlen = leaf_qpkey(qp, n, found);
+                       difpos = qpkey_compare(search, searchlen, found,
+                                              foundlen);
+               }
+
+               if (difpos <= searchlen && difpos <= foundlen &&
+                   search[difpos] < found[difpos])
                {
                        prevleaf(iter);
                }
        }
 
-       if (offset == QPKEY_EQUAL || offset == foundlen) {
+       if (difpos == QPKEY_EQUAL || difpos == foundlen) {
                SET_IF_NOT_NULL(pval_r, leaf_pval(n));
                SET_IF_NOT_NULL(ival_r, leaf_ival(n));
                maybe_set_name(qp, n, foundname);
-               add_link(chain, n, offset);
-               if (offset == QPKEY_EQUAL) {
+               add_link(chain, n, difpos);
+               if (difpos == QPKEY_EQUAL) {
                        return (ISC_R_SUCCESS);
                } else {
                        return (DNS_R_PARTIALMATCH);
@@ -2257,7 +2273,7 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
         */
        int len = chain->len;
        while (len-- > 0) {
-               if (offset >= chain->chain[len].offset) {
+               if (difpos >= chain->chain[len].offset) {
                        n = chain->chain[len].node;
                        SET_IF_NOT_NULL(pval_r, leaf_pval(n));
                        SET_IF_NOT_NULL(ival_r, leaf_ival(n));

diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c
index b78281dfeb36a8e8bfeb61f87f08fbd8ad0d26e7..44f8ded9b2bcc48ed38ee4166c1c0504849e8877 100644 (file)
--- a/tests/dns/qp_test.c
+++ b/tests/dns/qp_test.c
@@ -667,7 +667,16 @@ ISC_RUN_TEST_IMPL(predecessors) {
                { "b.d.", "c.b.b.", ISC_R_NOTFOUND },
                { "mon.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "moor.", "moops.", ISC_R_NOTFOUND },
+               { "mopbop.", "moops.", ISC_R_NOTFOUND },
+               { "moppop.", "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 },
+               { "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 },
                { NULL, NULL, 0 }
        };
 
@@ -695,7 +704,16 @@ ISC_RUN_TEST_IMPL(predecessors) {
                { "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 },
+               { "mopbop.", "moops.", DNS_R_PARTIALMATCH },
+               { "moppop.", "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 },
+               { "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 },
                { NULL, NULL, 0 }
        };