diff --git a/CHANGES b/CHANGES index 1efab4c2e0..98eca3480b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,6 @@ +6379. [bug] A QP iterator bug could result in DNSSEC validation + failing because the wrong NSEC was returned. [GL #4659] + 6378. [func] The option to specify the number of UDP dispatches was previously removed. An attempt to use the option now prints a warning. [GL #1879] diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 686c3561f5..0329f424cb 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2028,17 +2028,16 @@ add_link(dns_qpchain_t *chain, dns_qpnode_t *node, size_t offset) { INSIST(chain->len <= DNS_NAME_MAXLABELS); } -static inline dns_qpnode_t * +static inline void 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 * +static inline void greatest_leaf(dns_qpreadable_t qpr, dns_qpnode_t *n, dns_qpiter_t *iter) { while (is_branch(n)) { dns_qpref_t ref = branch_twigs_ref(n) + branch_twigs_size(n) - @@ -2047,7 +2046,6 @@ greatest_leaf(dns_qpreadable_t qpr, dns_qpnode_t *n, dns_qpiter_t *iter) { n = ref_ptr(qpr, ref); } iter->stack[++iter->sp] = n; - return (n); } static inline dns_qpnode_t * @@ -2099,19 +2097,16 @@ anyleaf(dns_qpreader_t *qp, dns_qpnode_t *n) { * the parent branch. We can use the qpiter stack to step back one leaf to * the predecessor. */ -static dns_qpnode_t * -fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start, - dns_qpkey_t search, size_t searchlen, dns_qpshift_t bit, - size_t offset) { +static void +fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, + size_t searchlen, dns_qpshift_t bit, size_t offset) { dns_qpkey_t found; - size_t foundlen, to; - dns_qpnode_t *n = start; - dns_qpnode_t *leaf = anyleaf(qp, n); + dns_qpnode_t *n = iter->stack[iter->sp]; + size_t foundlen = leaf_qpkey(qp, anyleaf(qp, n), found); + size_t to = qpkey_compare(search, searchlen, found, foundlen); - foundlen = leaf_qpkey(qp, leaf, found); - to = qpkey_compare(search, searchlen, found, foundlen); if (to == QPKEY_EQUAL) { - return (leaf); + return; } /* @@ -2123,7 +2118,8 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start, */ if (to < branch_key_offset(iter->stack[0])) { dns_qpiter_init(qp, iter); - return (prevleaf(iter)); + prevleaf(iter); + return; } /* @@ -2146,21 +2142,49 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start, * This was the first domain; move the iterator * one step back from the origin and return. */ - return (prevleaf(iter)); + prevleaf(iter); + return; } + RUNTIME_CHECK(result == ISC_R_SUCCESS); n = iter->stack[iter->sp]; - leaf = n; } else { - if (is_branch(n)) { - iter->sp--; - n = greatest_leaf(qp, n, iter); + if (to <= searchlen && to <= foundlen && iter->sp > 0) { + /* + * If we're here, search[to] >= found[to], + * meaning every leaf in this set of twigs + * is less than the one we wanted. It's + * possible we're already positioned at + * the correct predecessor, but it's not + * guaranteed, so we pop up to the parent + * branch, and find the greatest leaf from + * there. + */ + if (!is_branch(n)) { + iter->stack[iter->sp--] = NULL; + n = iter->stack[iter->sp]; + } } - return (n); + + if (is_branch(n)) { + iter->stack[iter->sp--] = NULL; + greatest_leaf(qp, n, iter); + } + + return; } - foundlen = leaf_qpkey(qp, leaf, found); - to = qpkey_compare(search, searchlen, found, foundlen); + foundlen = leaf_qpkey(qp, n, found); + size_t nto = qpkey_compare(search, searchlen, found, foundlen); + if (nto < to) { + /* + * We've moved to a new leaf and it differs at an + * even earlier point, so no further improvement is + * possible. + */ + return; + } + to = nto; } if (is_branch(n)) { @@ -2177,32 +2201,29 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start, * wanted; use the iterator to walk back to the * predecessor. */ - n = prevleaf(iter); + prevleaf(iter); } else { /* * The name we want would've been after some twig in - * this branch. point n to that twig, then walk down - * to the highest leaf in that subtree to get - * the predecessor. + * this branch. walk down from that twig to the + * highest leaf in that subtree to get the predecessor. */ - n = greatest_leaf(qp, twigs + pos - 1, iter); + greatest_leaf(qp, twigs + pos - 1, iter); } - } else { /* - * We're on the right leaf, either the iterator already points - * to the rightful predecessor, or it points to an immediate - * successor. If the latter, we can now use the qpiter stack - * we've constructed to step back to the predecessor. Otherwise, - * we don't have to do anything anymore. + * We're on the right leaf, so either the iterator already + * points to the rightful predecessor, or it points to an + * immediate successor. If the latter, we can now use the + * qpiter stack we've constructed to step back to the + * predecessor. Otherwise, we don't have to do anything + * anymore. */ if (to <= searchlen && to <= foundlen && search[to] < found[to]) { - n = prevleaf(iter); + prevleaf(iter); } } - - return (n); } isc_result_t @@ -2284,6 +2305,7 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, * the loop. */ n = branch_twig_ptr(qp, n, bit); + iter->stack[++iter->sp] = n; } else if (setiter) { /* * this branch is a dead end. however, the caller @@ -2291,10 +2313,8 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, * for the predecessor of the searched-for-name; * that will break the loop. */ - n = fix_iterator(qp, iter, n, search, searchlen, bit, - offset); - iter->stack[iter->sp] = NULL; - iter->sp--; + fix_iterator(qp, iter, search, searchlen, bit, offset); + n = iter->stack[iter->sp]; } else { /* * this branch is a dead end, and the predecessor @@ -2308,9 +2328,8 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, /* walk down to find the leftmost leaf */ n = anyleaf(qp, twigs); } + iter->stack[++iter->sp] = n; } - - iter->stack[++iter->sp] = n; } if (matched && setiter) { @@ -2320,10 +2339,13 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, * and if the caller passed us an iterator, * then we might need to reposition it. */ - n = fix_iterator(qp, iter, n, search, searchlen, bit, offset); + fix_iterator(qp, iter, search, searchlen, bit, offset); + n = iter->stack[iter->sp]; } - /* do the keys differ, and if so, where? */ + /* at this point, n can only be a leaf node */ + INSIST(!is_branch(n)); + foundlen = leaf_qpkey(qp, n, found); offset = qpkey_compare(search, searchlen, found, foundlen); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 6938cb9fd8..efd1470360 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -765,29 +765,29 @@ ISC_RUN_TEST_IMPL(predecessors) { */ 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.", - "trailing.", - "" }; + const char insert1[][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.", + "trailing.", + "" }; int i = 0; dns_qp_create(mctx, &string_methods, NULL, &qp); - while (insert[i][0] != '\0') { - insert_str(qp, insert[i++]); + while (insert1[i][0] != '\0') { + insert_str(qp, insert1[i++]); } static struct check_predecessors check1[] = { @@ -800,7 +800,46 @@ ISC_RUN_TEST_IMPL(fixiterator) { }; check_predecessors(qp, check1); + dns_qp_destroy(&qp); + const char insert2[][64] = { ".", "abb.", "abc.", "" }; + i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + while (insert2[i][0] != '\0') { + insert_str(qp, insert2[i++]); + } + + static struct check_predecessors check2[] = { + { "acb.", "abc.", DNS_R_PARTIALMATCH, 0 }, + { "acc.", "abc.", DNS_R_PARTIALMATCH, 0 }, + { "abbb.", "abb.", DNS_R_PARTIALMATCH, 1 }, + { "aab.", ".", DNS_R_PARTIALMATCH, 2 }, + { NULL, NULL, 0, 0 } + }; + + check_predecessors(qp, check2); + dns_qp_destroy(&qp); + + const char insert3[][64] = { "example.", + "key-is-13779.example.", + "key-is-14779.example.", + "key-not-13779.example.", + "key-not-14779.example.", + "" }; + i = 0; + + dns_qp_create(mctx, &string_methods, NULL, &qp); + while (insert3[i][0] != '\0') { + insert_str(qp, insert3[i++]); + } + + static struct check_predecessors check3[] = { { "key-is-21556.example.", + "key-is-14779.example.", + DNS_R_PARTIALMATCH, 2 }, + { NULL, NULL, 0, 0 } }; + + check_predecessors(qp, check3); dns_qp_destroy(&qp); }