From dd39b16a8d2e0d03c19e90f6c0fb6ad438f9c690 Mon Sep 17 00:00:00 2001 From: Evan Hunt Date: Sun, 3 Dec 2023 14:02:29 -0800 Subject: [PATCH] fix dns_qp_lookup() iterator bug in some cases it was possible for the iterator to be positioned in the wrong place by dns_qp_lookup(). previously, when a leaf node was found which matched the search key at its parent branch's offset point, but did not match after that point, the code incorrectly assumed the leaf it had found was a successor to the searched-for name, and stepped the iterator back to find a predecessor. however, it was possible for the non-matching leaf to be the predecessor, in which case stepping the iterator back was wrong. (for example: a branch contains "aba" and "abcd", and we are searching for "abcde". we step down to the twig matching the letter "c" in position 3. "abcd" is the predecessor of "abcde", so the iterator is already correctly positioned, but because the twig was an exact match, we would have moved it back one step to "aba".) this previously went unnoticed due to a mistake in the qp_test unit test, which had the wrong expected result for the test case that should have detected the error. both the code and the test have been fixed. --- lib/dns/qp.c | 16 ++++++++++------ tests/dns/qp_test.c | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/dns/qp.c b/lib/dns/qp.c index ee78b98c30..f71f4dd925 100644 --- a/lib/dns/qp.c +++ b/lib/dns/qp.c @@ -2179,18 +2179,22 @@ dns_qp_lookup(dns_qpreadable_t qpr, const dns_name_t *name, * the predecessor name for a non-exact match. * * if 'matched' is true, then the search ended at a leaf. - * if it was not an exact match, then we're now pointing at - * the immediate successor of the searched-for name, and can - * use the qpiter stack we've constructed to step back to - * the predecessor. if it was an exact match, we don't need to - * do anything. + * if it was not an exact match, then we're now pointing + * at either the immediate predecessor or the immediate + * successor of the searched-for name; if successor, we can + * now use the qpiter stack we've constructed to step back to + * the predecessor. if we're pointed at the predecessor + * or it was an exact match, we don't need to do anything. * * if 'matched' is false, then the search failed at a branch * node, and we would already have positioned the iterator * at the predecessor. */ if (getpred && matched) { - if (offset != QPKEY_EQUAL) { + if (offset != QPKEY_EQUAL && + (offset <= searchlen && offset <= foundlen && + found[offset] > search[offset])) + { prevleaf(iter); } } diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index d68ac263c1..28ea2266b7 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -660,7 +660,7 @@ ISC_RUN_TEST_IMPL(predecessors) { { "a.b.c.e.f.", "a.b.c.d.e.", ISC_R_NOTFOUND }, { "z.y.x.", "a.b.c.d.e.", ISC_R_NOTFOUND }, { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, - { "z.z.z.z.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, + { "z.z.z.z.k.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, { "0.b.c.d.e.", "x.k.c.d.", ISC_R_NOTFOUND }, @@ -686,7 +686,7 @@ ISC_RUN_TEST_IMPL(predecessors) { { "a.b.c.e.f.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, { "z.y.x.", "a.b.c.d.e.", DNS_R_PARTIALMATCH }, { "w.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, - { "z.z.z.z.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, + { "z.z.z.z.k.c.d.", "x.k.c.d.", DNS_R_PARTIALMATCH }, { "w.k.c.d.", "a.b.c.d.", DNS_R_PARTIALMATCH }, { "d.a.", "e.d.c.b.a.", DNS_R_PARTIALMATCH }, { "0.b.c.d.e.", "x.k.c.d.", DNS_R_PARTIALMATCH },