fix dns_qp_lookup() iterator bug

in some cases it was possible for the iterator to be positioned in the
wrong place by dns_qp_lookup(). previously, when a leaf node was found
which matched the search key at its parent branch's offset point, but
did not match after that point, the code incorrectly assumed the leaf
it had found was a successor to the searched-for name, and stepped the
iterator back to find a predecessor.  however, it was possible for the
non-matching leaf to be the predecessor, in which case stepping the
iterator back was wrong.

(for example: a branch contains "aba" and "abcd", and we are searching
for "abcde". we step down to the twig matching the letter "c" in
position 3. "abcd" is the predecessor of "abcde", so the iterator is
already correctly positioned, but because the twig was an exact match,
we would have moved it back one step to "aba".)

this previously went unnoticed due to a mistake in the qp_test unit
test, which had the wrong expected result for the test case that should
have detected the error. both the code and the test have been fixed.

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index ee78b98c307274deb42f2feeed8cd30b7efd6860..f71f4dd92566eea188212048cd45a9eb5be76e0d 100644 (file)
--- a/lib/dns/qp.c
+++ b/lib/dns/qp.c
@@ -2179,18 +2179,22 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name,
         * the predecessor name for a non-exact match.
         *
         * if 'matched' is true, then the search ended at a leaf.
-        * if it was not an exact match, then we're now pointing at
-        * the immediate successor of the searched-for name, and can
-        * use the qpiter stack we've constructed to step back to
-        * the predecessor. if it was an exact match, we don't need to
-        * do anything.
+        * if it was not an exact match, then we're now pointing
+        * at either the immediate predecessor or the immediate
+        * successor of the searched-for name; if successor, we can
+        * now use the qpiter stack we've constructed to step back to
+        * the predecessor. if we're pointed at the predecessor
+        * or it was an exact match, we don't need to do anything.
         *
         * if 'matched' is false, then the search failed at a branch
         * node, and we would already have positioned the iterator
         * at the predecessor.
         */
        if (getpred && matched) {
-               if (offset != QPKEY_EQUAL) {
+               if (offset != QPKEY_EQUAL &&
+                   (offset <= searchlen && offset <= foundlen &&
+                    found[offset] > search[offset]))
+               {
                        prevleaf(iter);
                }
        }

diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c
index d68ac263c1b666918927c534ba25578e073ebdda..28ea2266b7afc1905571df7ddda72b9b3004e430 100644 (file)
--- a/tests/dns/qp_test.c
+++ b/tests/dns/qp_test.c
@@ -660,7 +660,7 @@ ISC_RUN_TEST_IMPL(predecessors) {
                { "a.b.c.e.f.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "z.y.x.", "a.b.c.d.e.", ISC_R_NOTFOUND },
                { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH },
-               { "z.z.z.z.k.c.d.", "a.b.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 },
@@ -686,7 +686,7 @@ ISC_RUN_TEST_IMPL(predecessors) {
                { "a.b.c.e.f.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { "z.y.x.", "a.b.c.d.e.", DNS_R_PARTIALMATCH },
                { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH },
-               { "z.z.z.z.k.c.d.", "a.b.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 },