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

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

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