From 84f79cd16413185e4bfd222afa9d3854a4e485a3 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 20 Dec 2023 12:38:12 -0800 Subject: [PATCH 1/2] 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); From ea9a8cb392ff59438a911485742b220d40f24d6f Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 20 Dec 2023 00:32:57 -0800 Subject: [PATCH 2/2] prevent an infinite loop in fix_iterator() it was possible for fix_iterator() to get stuck in a loop while trying to find the predecessor of a missing node. this has been fixed and a regression test has been added. --- lib/dns/qp.c | 13 +++++++++++-- tests/dns/qp_test.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index bf7e34b5c4..4ea05e0968 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2115,6 +2115,17 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start, return (leaf); } + /* + * Special case: if the search key differs even before the root + * key offset, it means the name desired either precedes or + * follows the entire range of names in the database, and + * popping up the stack won't help us, so just move the + * iterator one step back from the origin and return. + */ + if (to < branch_key_offset(iter->stack[0])) { + dns_qpiter_init(qp, iter); + return (prevleaf(iter)); + } /* * As long as the branch offset point is after the point where the * search key differs, we need to branch up and find a better leaf @@ -2128,8 +2139,6 @@ 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--; n = prevleaf(iter); leaf = n; } else { diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 674a92cd4f..b9c3afcc5f 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -755,6 +755,48 @@ ISC_RUN_TEST_IMPL(predecessors) { dns_qp_destroy(&qp); } +/* + * this is a regression test for an infinite loop that could + * previously occur in fix_iterator() + */ +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.", + "" }; + int i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + while (insert[i][0] != '\0') { + insert_str(qp, insert[i++]); + } + + static struct check_predecessors check1[] = { + { "newtext.dynamic.", "mx.dynamic.", DNS_R_PARTIALMATCH, 6 }, + { "absent.", "txt.dynamic.", ISC_R_NOTFOUND, 0 }, + { "nonexistent.", "txt.dynamic.", ISC_R_NOTFOUND, 0 }, + { NULL, NULL, 0, 0 } + }; + + check_predecessors(qp, check1); + + dns_qp_destroy(&qp); +} + ISC_TEST_LIST_START ISC_TEST_ENTRY(qpkey_name) ISC_TEST_ENTRY(qpkey_sort) @@ -762,6 +804,7 @@ ISC_TEST_ENTRY(qpiter) ISC_TEST_ENTRY(partialmatch) ISC_TEST_ENTRY(qpchain) ISC_TEST_ENTRY(predecessors) +ISC_TEST_ENTRY(fixiterator) ISC_TEST_LIST_END ISC_TEST_MAIN