From ab8a0c4b5a03857c3701dda47a924fde775a23ec Mon Sep 17 00:00:00 2001 From: Matthijs Mekking Date: Thu, 7 Dec 2023 11:51:23 +0100 Subject: [PATCH] and fix yet another dns_qp_lookup() iterator bug This one is similar to the bug when searching for a key, reaching a dead-end branch that doesn't match, because the branch offset point is after the point where the search key differs. This fixes the case where we are multiple levels deep. In other words, we had a more-than-one matches *after* the point where the search key differs. For example, consider the following qp-trie: branch: "[e]", "[m]": - leaf: "a.b.c.d.e" - branch: "moo[g]", "moo[k]", "moo[n]": - leaf: "moog" - branch: "mook[e]", "mook[o]" - leaf: "mooker" - leaf: "mooko" - leaf: "moon" If searching for a key "monky", we would reach the branch with twigs "moo[k]" and "moo[n]". The key matches on the 'k' on offset=4, and reaches the branch with twigs "mook[e]" and "mook[o]". This time we cannot find a twig that matches our key at offset=5, there is no twig for 'y'. The closest name we found was "mooker". Note that on a branch it can't detect it is on a dead branch because the key is not encapsulated in a branch node. In the previous code we considered "mooker" to be the successor of "monky" and so we needed to the predecessor of "mooker" to find the predecessor for "monky". However, since the search key alread differed before entering this branch, this is not enough. We would be left with "moog" as the predecessor of "monky", while in this example "a.b.c.d.e" is the actual predecessor. Instead, we need to go up a level, find the predecessor and check again if we are on the right branch, and repeat the process until we are. Unit tests to cover the scenario are now added. --- lib/dns/qp.c | 11 ++++++++--- tests/dns/qp_test.c | 21 ++++++++++++++++----- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index bb212fdfba..1fd36b91ba 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2139,12 +2139,14 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, */ size_t to; dns_qpnode_t *least = n; + + least: while (is_branch(least)) { least = branch_twigs(qp, least); } foundlen = leaf_qpkey(qp, least, found); to = qpkey_compare(search, searchlen, found, foundlen); - if (to == offset) { + if (to >= offset) { /* * we're on the right branch, so find * the best match. @@ -2180,9 +2182,12 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, * we wanted, so iterate back to the * predecessor. */ + iter->sp--; prevleaf(iter); - n = iter->stack[iter->sp--]; - } else { + n = iter->stack[iter->sp]; + least = n; + goto least; + } else if (is_branch(n)) { /* * every leaf is less than the one we * wanted, so get the highest. diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index 44f8ded9b2..3889de5eda 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -635,11 +635,12 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) { ISC_RUN_TEST_IMPL(predecessors) { dns_qp_t *qp = NULL; - const char insert[][16] = { "a.", "b.", "c.b.a.", - "e.d.c.b.a.", "c.b.b.", "c.d.", - "a.b.c.d.", "a.b.c.d.e.", "b.a.", - "x.k.c.d.", "moog.", "mook.", - "moon.", "moops.", "" }; + const char insert[][16] = { + "a.", "b.", "c.b.a.", "e.d.c.b.a.", + "c.b.b.", "c.d.", "a.b.c.d.", "a.b.c.d.e.", + "b.a.", "x.k.c.d.", "moog.", "mooker.", + "mooko.", "moon.", "moops.", "" + }; int i = 0; dns_qp_create(mctx, &string_methods, NULL, &qp); @@ -669,14 +670,19 @@ ISC_RUN_TEST_IMPL(predecessors) { { "moor.", "moops.", ISC_R_NOTFOUND }, { "mopbop.", "moops.", ISC_R_NOTFOUND }, { "moppop.", "moops.", ISC_R_NOTFOUND }, + { "mopps.", "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 }, + { "monps.", "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 }, + { "monky.", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "monkey.", "a.b.c.d.e.", ISC_R_NOTFOUND }, + { "monker.", "a.b.c.d.e.", ISC_R_NOTFOUND }, { NULL, NULL, 0 } }; @@ -706,14 +712,19 @@ ISC_RUN_TEST_IMPL(predecessors) { { "moor.", "moops.", DNS_R_PARTIALMATCH }, { "mopbop.", "moops.", DNS_R_PARTIALMATCH }, { "moppop.", "moops.", DNS_R_PARTIALMATCH }, + { "mopps.", "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 }, + { "monps.", "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 }, + { "monky.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, + { "monkey.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, + { "monker.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, { NULL, NULL, 0 } };