fix_iterator() could produce incoherent iterator stacks

the fix_iterator() function moves an iterator so that it points
to the predecessor of the searched-for name when that name doesn't
exist in the database. the tests only checked the correctness of
the top of the stack, however, and missed some cases where interior
branches in the stack could be missing or duplicated. in these
cases, the iterator would produce inconsistent results when walked.

the predecessors test case in qp_test has been updated to walk
each iterator to the end and ensure that the expected number of
nodes are found.

diff --git a/lib/dns/qp.c b/lib/dns/qp.c
index 7c04957342fda1db9373911821311bdd4e738882..bf7e34b5c4684c557a61a01ec7c7983e5e98bdc6 100644 (file)
--- a/lib/dns/qp.c
+++ b/lib/dns/qp.c
@@ -2029,13 +2029,14 @@ add_link(dns_qpchain_t *chain, dns_qpnode_t *node, size_t offset) {
        INSIST(chain->len <= DNS_NAME_MAXLABELS);
 }
 
-static inline void
+static inline dns_qpnode_t *
 prevleaf(dns_qpiter_t *it) {
        isc_result_t result = dns_qpiter_prev(it, NULL, NULL, NULL);
        if (result == ISC_R_NOMORE) {
                result = dns_qpiter_prev(it, NULL, NULL, NULL);
        }
        RUNTIME_CHECK(result == ISC_R_SUCCESS);
+       return (it->stack[it->sp]);
 }
 
 static inline dns_qpnode_t *
@@ -2046,6 +2047,7 @@ greatest_leaf(dns_qpreadable_t qpr, dns_qpnode_t *n, dns_qpiter_t *iter) {
                iter->stack[++iter->sp] = n;
                n = ref_ptr(qpr, ref);
        }
+       iter->stack[++iter->sp] = n;
        return (n);
 }
 
@@ -2126,12 +2128,13 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start,
                         * go to the parent branch and iterate back to the
                         * predecessor from that point.
                         */
+                       iter->stack[iter->sp] = NULL;
                        iter->sp--;
-                       prevleaf(iter);
-                       n = iter->stack[iter->sp];
+                       n = prevleaf(iter);
                        leaf = n;
                } else {
                        if (is_branch(n)) {
+                               iter->sp--;
                                n = greatest_leaf(qp, n, iter);
                        }
                        return (n);
@@ -2155,8 +2158,7 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start,
                         * wanted; use the iterator to walk back to the
                         * predecessor.
                         */
-                       prevleaf(iter);
-                       n = iter->stack[iter->sp--];
+                       n = prevleaf(iter);
                } else {
                        /*
                         * The name we want would've been after some twig in
@@ -2177,7 +2179,7 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start,
                 */
                if (to <= searchlen && to <= foundlen && search[to] < found[to])
                {
-                       prevleaf(iter);
+                       n = prevleaf(iter);
                }
        }
 
@@ -2272,6 +2274,8 @@ 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--;
                } 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 3889de5edaef8ebee1c714c1dc704bf1d275a05c..674a92cd4ff5b79bd8214991f814c10df603178a 100644 (file)
--- a/tests/dns/qp_test.c
+++ b/tests/dns/qp_test.c
@@ -581,6 +581,7 @@ struct check_predecessors {
        const char *query;
        const char *predecessor;
        isc_result_t result;
+       int remaining;
 };
 
 static void
@@ -589,11 +590,12 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) {
        dns_fixedname_t fn1, fn2;
        dns_name_t *name = dns_fixedname_initname(&fn1);
        dns_name_t *pred = dns_fixedname_initname(&fn2);
+       char *namestr = NULL;
 
        for (int i = 0; check[i].query != NULL; i++) {
                dns_qpiter_t it;
-               char *predname = NULL;
 
+               namestr = NULL;
                dns_test_namefromstring(check[i].query, &fn1);
                result = dns_qp_lookup(qp, name, NULL, &it, NULL, NULL, NULL);
 #if 0
@@ -621,15 +623,35 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) {
                }
                assert_int_equal(result, ISC_R_SUCCESS);
 
-               result = dns_name_tostring(pred, &predname, mctx);
+               result = dns_name_tostring(pred, &namestr, mctx);
 #if 0
                fprintf(stderr, "... expected predecessor %s got %s\n",
-                       check[i].predecessor, predname);
+                       check[i].predecessor, namestr);
 #endif
                assert_int_equal(result, ISC_R_SUCCESS);
+               assert_string_equal(namestr, check[i].predecessor);
 
-               assert_string_equal(predname, check[i].predecessor);
-               isc_mem_free(mctx, predname);
+#if 0
+               fprintf(stderr, "%d: remaining names after %s:\n", i, namestr);
+#endif
+               isc_mem_free(mctx, namestr);
+
+               int j = 0;
+               while (dns_qpiter_next(&it, name, NULL, NULL) == ISC_R_SUCCESS)
+               {
+#if 0
+                       result = dns_name_tostring(name, &namestr, mctx);
+                       assert_int_equal(result, ISC_R_SUCCESS);
+                       fprintf(stderr, "%s%s", j > 0 ? "->" : "", namestr);
+                       isc_mem_free(mctx, namestr);
+#endif
+                       j++;
+               }
+#if 0
+               fprintf(stderr, "\n...expected %d got %d\n",
+                       check[i].remaining, j);
+#endif
+               assert_int_equal(j, check[i].remaining);
        }
 }
 
@@ -650,40 +672,40 @@ ISC_RUN_TEST_IMPL(predecessors) {
 
        /* first check: no root label in the database */
        static struct check_predecessors check1[] = {
-               { ".", "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 },
-               { "ddd.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH },
-               { "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.", "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 },
-               { "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 }
+               { ".", "moops.", ISC_R_NOTFOUND, 0 },
+               { "a.", "moops.", ISC_R_SUCCESS, 0 },
+               { "b.a.", "a.", ISC_R_SUCCESS, 14 },
+               { "b.", "e.d.c.b.a.", ISC_R_SUCCESS, 11 },
+               { "aaa.a.", "a.", DNS_R_PARTIALMATCH, 14 },
+               { "ddd.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH, 11 },
+               { "d.c.", "c.b.b.", ISC_R_NOTFOUND, 9 },
+               { "1.2.c.b.a.", "c.b.a.", DNS_R_PARTIALMATCH, 12 },
+               { "a.b.c.e.f.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "z.y.x.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH, 6 },
+               { "z.z.z.z.k.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH, 6 },
+               { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH, 7 },
+               { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH, 11 },
+               { "0.b.c.d.e.", "x.k.c.d.", ISC_R_NOTFOUND, 6 },
+               { "b.d.", "c.b.b.", ISC_R_NOTFOUND, 9 },
+               { "mon.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "moor.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "mopbop.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "moppop.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "mopps.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "mopzop.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "mop.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "monbop.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "monpop.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "monps.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "monzop.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "mon.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "moop.", "moon.", ISC_R_NOTFOUND, 1 },
+               { "moopser.", "moops.", ISC_R_NOTFOUND, 0 },
+               { "monky.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "monkey.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { "monker.", "a.b.c.d.e.", ISC_R_NOTFOUND, 5 },
+               { NULL, NULL, 0, 0 }
        };
 
        check_predecessors(qp, check1);
@@ -693,39 +715,39 @@ ISC_RUN_TEST_IMPL(predecessors) {
        insert_str(qp, root);
 
        static struct check_predecessors check2[] = {
-               { ".", "moops.", ISC_R_SUCCESS },
-               { "a.", ".", ISC_R_SUCCESS },
-               { "b.a.", "a.", ISC_R_SUCCESS },
-               { "b.", "e.d.c.b.a.", ISC_R_SUCCESS },
-               { "aaa.a.", "a.", DNS_R_PARTIALMATCH },
-               { "ddd.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH },
-               { "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.", "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 },
-               { "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 }
+               { ".", "moops.", ISC_R_SUCCESS, 0 },
+               { "a.", ".", ISC_R_SUCCESS, 15 },
+               { "b.a.", "a.", ISC_R_SUCCESS, 14 },
+               { "b.", "e.d.c.b.a.", ISC_R_SUCCESS, 11 },
+               { "aaa.a.", "a.", DNS_R_PARTIALMATCH, 14 },
+               { "ddd.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH, 11 },
+               { "d.c.", "c.b.b.", DNS_R_PARTIALMATCH, 9 },
+               { "1.2.c.b.a.", "c.b.a.", DNS_R_PARTIALMATCH, 12 },
+               { "a.b.c.e.f.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "z.y.x.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH, 6 },
+               { "z.z.z.z.k.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH, 6 },
+               { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH, 7 },
+               { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH, 11 },
+               { "0.b.c.d.e.", "x.k.c.d.", DNS_R_PARTIALMATCH, 6 },
+               { "mon.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "moor.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "mopbop.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "moppop.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "mopps.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "mopzop.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "mop.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "monbop.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "monpop.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "monps.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "monzop.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "mon.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "moop.", "moon.", DNS_R_PARTIALMATCH, 1 },
+               { "moopser.", "moops.", DNS_R_PARTIALMATCH, 0 },
+               { "monky.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "monkey.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { "monker.", "a.b.c.d.e.", DNS_R_PARTIALMATCH, 5 },
+               { NULL, NULL, 0, 0 }
        };
 
        check_predecessors(qp, check2);