ComputerSurge/bind9
3
0
Fork 0
Code Pull Requests 1 Activity

Merge branch '4659-rootkeysentinel-test-fails-for-certain-values-of-oldid' into 'main'

Browse Source 
fix_iterator() bug causes DNSSEC NXDOMAIN responses to be broken

Closes #4659

See merge request isc-projects/bind9!8942
This commit is contained in:
Evan Hunt
2024-04-25 18:10:29 +00:00
parent b1184d916d 6f4ef40ccd
commit 7d289e5333
3 changed files with 129 additions and 65 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
CHANGES
View File

@@ -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]

112
lib/dns/qp.c
View File

@@ -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);

79
tests/dns/qp_test.c
View File

@@ -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);
}