From 2dff9266248928729a44bd0e7b525941a4187407 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 10 Apr 2024 16:13:48 -0400 Subject: [PATCH 1/5] yet another fix_iterator() bug under some circumstances it was possible for the iterator to be set to the first leaf in a set of twigs, when it should have been set to the last. a unit test has been added to test this scenario. if there is a a tree containing the following values: {".", "abb.", "abc."}, and we query for "acb.", previously the iterator would be positioned at "abb." instead of "abc.". the tree structure is: branch (offset 1, ".") branch (offset 3, ".ab") leaf (".abb") leaf (".abc") we find the branch with offset 3 (indicating that its twigs differ from each other in the third position of the label, "abB" vs "abC"). but the search key differs from the found keys at position 2 ("aC" vs "aB"). we look up the bit value in position 3 of the search key ("B"), and incorrectly follow it onto the wrong twig ("abB"). to correct for this, we need to check for the case where the search key is greater than the found key in a position earlier than the branch offset. if it is, then we need to pop from the current leaf to its parent, and get the greatest leaf from there. a further change is needed to ensure that we don't do this twice; when we've moved to a new leaf and the point of difference between it and the search key even earlier than before, then we're definitely at a predecessor node and there's no need to continue the loop. --- lib/dns/qp.c | 34 ++++++++++++++++++++++---- tests/dns/qp_test.c | 58 +++++++++++++++++++++++++++++---------------- 2 files changed, 68 insertions(+), 24 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 686c3561f5..e381b7c324 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2152,15 +2152,42 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpnode_t *start, n = iter->stack[iter->sp]; leaf = n; } else { + 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]; + } + } + if (is_branch(n)) { - iter->sp--; + iter->stack[iter->sp--] = NULL; n = greatest_leaf(qp, n, iter); } return (n); } foundlen = leaf_qpkey(qp, leaf, found); - to = qpkey_compare(search, searchlen, found, foundlen); + + 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 (leaf); + } + to = nto; } if (is_branch(n)) { @@ -2293,8 +2320,7 @@ 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--; + iter->stack[iter->sp--] = NULL; } 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 6938cb9fd8..d49fe674b3 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,25 @@ 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); } From 66dbff596b223e6084c113f61293ee4f0f0caaf2 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 18 Apr 2024 16:25:48 -0700 Subject: [PATCH 2/5] clean up fix_iterator() arguments the value passed as 'start' was redundant; it's always the same as the current top of the iterator stack. --- lib/dns/qp.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index e381b7c324..047338faa4 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2100,12 +2100,11 @@ anyleaf(dns_qpreader_t *qp, dns_qpnode_t *n) { * 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) { +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 *n = iter->stack[iter->sp]; dns_qpnode_t *leaf = anyleaf(qp, n); foundlen = leaf_qpkey(qp, leaf, found); @@ -2318,7 +2317,7 @@ 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, + n = fix_iterator(qp, iter, search, searchlen, bit, offset); iter->stack[iter->sp--] = NULL; } else { @@ -2346,7 +2345,7 @@ 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); + n = fix_iterator(qp, iter, search, searchlen, bit, offset); } /* do the keys differ, and if so, where? */ From b1b1ca8ca405cc53e255e0fce008f8fa3a7a3656 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 18 Apr 2024 12:01:25 -0700 Subject: [PATCH 3/5] add another broken testcase --- tests/dns/qp_test.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index d49fe674b3..efd1470360 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -820,6 +820,27 @@ ISC_RUN_TEST_IMPL(fixiterator) { 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); } ISC_TEST_LIST_START From 237123e500a8b74c075aeccf37e391d08c2df362 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Fri, 19 Apr 2024 15:57:32 -0700 Subject: [PATCH 4/5] simplify code by removing return values where possible fix_iterator() and related functions are quite difficult to read. perhaps it would be a little clearer if we didn't assign values to variables that won't subsequently be used, or unnecessarily pop the stack and then push the same value back onto it. also, in dns_qp_lookup() we previously called fix_iterator(), removed the leaf from the top of the iterator stack, and then added it back on. this would be clearer if we just push the leaf onto the stack when we need to, but leave the stack alone when it's already complete. --- lib/dns/qp.c | 75 +++++++++++++++++++++++++--------------------------- 1 file changed, 36 insertions(+), 39 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 047338faa4..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,18 +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 * +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 = iter->stack[iter->sp]; - dns_qpnode_t *leaf = anyleaf(qp, n); + 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; } /* @@ -2122,7 +2118,8 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, */ if (to < branch_key_offset(iter->stack[0])) { dns_qpiter_init(qp, iter); - return (prevleaf(iter)); + prevleaf(iter); + return; } /* @@ -2145,11 +2142,12 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, * 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 (to <= searchlen && to <= foundlen && iter->sp > 0) { /* @@ -2170,13 +2168,13 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, if (is_branch(n)) { iter->stack[iter->sp--] = NULL; - n = greatest_leaf(qp, n, iter); + greatest_leaf(qp, n, iter); } - return (n); + + return; } - foundlen = leaf_qpkey(qp, leaf, found); - + foundlen = leaf_qpkey(qp, n, found); size_t nto = qpkey_compare(search, searchlen, found, foundlen); if (nto < to) { /* @@ -2184,7 +2182,7 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, * even earlier point, so no further improvement is * possible. */ - return (leaf); + return; } to = nto; } @@ -2203,32 +2201,29 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search, * 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 @@ -2310,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 @@ -2317,9 +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, search, searchlen, bit, - offset); - iter->stack[iter->sp--] = NULL; + fix_iterator(qp, iter, search, searchlen, bit, offset); + n = iter->stack[iter->sp]; } else { /* * this branch is a dead end, and the predecessor @@ -2333,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) { @@ -2345,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, 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); From 6f4ef40ccd55c7c71f38eb3f52c2f40757d878c0 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Thu, 25 Apr 2024 10:30:47 -0700 Subject: [PATCH 5/5] CHANGES for [GL #4659] --- CHANGES | 3 +++ 1 file changed, 3 insertions(+) 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]