From 2dff9266248928729a44bd0e7b525941a4187407 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Wed, 10 Apr 2024 16:13:48 -0400 Subject: [PATCH] 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); }