From 84f79cd16413185e4bfd222afa9d3854a4e485a3 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 20 Dec 2023 12:38:12 -0800 Subject: [PATCH] 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. --- lib/dns/qp.c | 16 +++-- tests/dns/qp_test.c | 166 +++++++++++++++++++++++++------------------- 2 files changed, 104 insertions(+), 78 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 7c04957342..bf7e34b5c4 100644 --- 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 3889de5eda..674a92cd4f 100644 --- 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);