From 276bdcf5cff106bc24aad13af68fb350171438ca Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 7 Dec 2023 10:11:14 +0100 Subject: [PATCH] and fix another dns_qp_lookup() iterator bug There was yet another edge case in which an iterator could be positioned at the wrong node after dns_qp_lookup(). When searching for a key, it's possible to reach a leaf that matches at the given offset, but because the offset point is *after* the point where the search key differs from the leaf's contents, we are now at the wrong leaf. In other words, the bug fixed the previous commit for dead-end branches must also be applied on matched leaves. For example, if searching for the key "monpop", we could reach a branch containing "moop" and "moor". the branch offset point - i.e., the point after which the branch's leaves differ from each other - is the fourth character ("p" or "r"). The search key matches the fourth character "p", and takes that twig to the next node (which can be a branch for names starting with "moop", or could be a leaf node for "moop"). The old code failed to detect this condition, and would have incorrectly left the iterator pointing at some successor, and not at the predecessor of the "moop". To find the right predecessor in this case, we need to get to the previous branch and get the previous from there. This has been fixed and the unit test now includes several new scenarios for testing search names that match and unmatch on the offset but have a different character before the offset. --- lib/dns/qp.c | 36 ++++++++++++++++++++++++++---------- tests/dns/qp_test.c | 18 ++++++++++++++++++ 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 3a6d7147ad..bb212fdfba 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2056,7 +2056,8 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, dns_qpreader_t *qp = dns_qpreader(qpr); dns_qpkey_t search, found; size_t searchlen, foundlen; - size_t offset; + size_t offset = 0; + size_t difpos = 0; dns_qpnode_t *n = NULL; dns_qpchain_t oc; dns_qpiter_t it; @@ -2211,7 +2212,7 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, /* do the keys differ, and if so, where? */ foundlen = leaf_qpkey(qp, n, found); - offset = qpkey_compare(search, searchlen, found, foundlen); + difpos = qpkey_compare(search, searchlen, found, foundlen); /* * if we've been passed an iterator, we want it to point @@ -2230,21 +2231,36 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, * node, and we would already have positioned the iterator * at the predecessor. */ - if (getpred && matched) { - if (offset != QPKEY_EQUAL && - (offset <= searchlen && offset <= foundlen && - found[offset] > search[offset])) + if (getpred && matched && difpos != QPKEY_EQUAL) { + while (difpos < offset) { + if (difpos <= searchlen && difpos <= foundlen && + search[difpos] < found[difpos]) + { + iter->sp--; + prevleaf(iter); + n = iter->stack[iter->sp]; + } else { + break; + } + + foundlen = leaf_qpkey(qp, n, found); + difpos = qpkey_compare(search, searchlen, found, + foundlen); + } + + if (difpos <= searchlen && difpos <= foundlen && + search[difpos] < found[difpos]) { prevleaf(iter); } } - if (offset == QPKEY_EQUAL || offset == foundlen) { + if (difpos == QPKEY_EQUAL || difpos == foundlen) { SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); maybe_set_name(qp, n, foundname); - add_link(chain, n, offset); - if (offset == QPKEY_EQUAL) { + add_link(chain, n, difpos); + if (difpos == QPKEY_EQUAL) { return (ISC_R_SUCCESS); } else { return (DNS_R_PARTIALMATCH); @@ -2257,7 +2273,7 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, */ int len = chain->len; while (len-- > 0) { - if (offset >= chain->chain[len].offset) { + if (difpos >= chain->chain[len].offset) { n = chain->chain[len].node; SET_IF_NOT_NULL(pval_r, leaf_pval(n)); SET_IF_NOT_NULL(ival_r, leaf_ival(n)); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index b78281dfeb..44f8ded9b2 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -667,7 +667,16 @@ ISC_RUN_TEST_IMPL(predecessors) { { "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 }, + { "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 }, + { "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 }, { NULL, NULL, 0 } }; @@ -695,7 +704,16 @@ ISC_RUN_TEST_IMPL(predecessors) { { "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 }, + { "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 }, + { "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 }, { NULL, NULL, 0 } };