Merge branch '4702-qpiter-fix' into 'main'

dns_qp_lookup() doesn't handle searches for escaped characters well

Closes #4702

See merge request isc-projects/bind9!8999

CHANGES

@@ -1,3 +1,8 @@
+6381.	[bug]		dns_qp_lookup() could position the iterator at the
+			wrong predecessor when searching for names with
+			uncommon characters, which are encoded as two-octet
+			sequences in QP trie keys. [GL #4702]
+
 6380.	[func]		Queries and responses now emit distinct dnstap entries
 			for DoT and DoH. [GL #4523]
 

lib/dns/qp.c

@@ -2167,7 +2167,21 @@ fix_iterator(dns_qpreader_t *qp, dns_qpiter_t *iter, dns_qpkey_t search,
 			}
 
 			if (is_branch(n)) {
-				iter->stack[iter->sp--] = NULL;
+				/*
+				 * Pop up until we reach a branch that
+				 * differs earlier than the position we're
+				 * looking at. Note that because of escaped
+				 * characters, this might require popping
+				 * more than once.
+				 */
+				dns_qpnode_t *last = iter->stack[iter->sp];
+				while (iter->sp > 0 &&
+				       to < branch_key_offset(last))
+				{
+					n = last;
+					iter->stack[iter->sp--] = NULL;
+					last = iter->stack[iter->sp];
+				}
 				greatest_leaf(qp, n, iter);
 			}
 

tests/dns/qp_test.c

@@ -38,6 +38,8 @@
 #include <tests/dns.h>
 #include <tests/qp.h>
 
+bool verbose = false;
+
 ISC_RUN_TEST_IMPL(qpkey_name) {
 	struct {
 		const char *namestr;
@@ -56,9 +58,14 @@ ISC_RUN_TEST_IMPL(qpkey_name) {
 		},
 		{
 			.namestr = "\\000",
-			.key = { 0x03, 0x03, 0x02, 0x02 },
+			.key = { 0x03, 0x03, 0x02 },
 			.len = 3,
 		},
+		{
+			.namestr = "\\000\\009",
+			.key = { 0x03, 0x03, 0x03, 0x0c, 0x02 },
+			.len = 5,
+		},
 		{
 			.namestr = "com",
 			.key = { 0x16, 0x22, 0x20, 0x02 },
@@ -72,19 +79,19 @@ ISC_RUN_TEST_IMPL(qpkey_name) {
 		{
 			.namestr = "example.com.",
 			.key = { 0x02, 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14,
-				 0x20, 0x23, 0x1f, 0x18, 0x02, 0x02 },
+				 0x20, 0x23, 0x1f, 0x18, 0x02 },
 			.len = 13,
 		},
 		{
 			.namestr = "example.com",
 			.key = { 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14, 0x20,
-				 0x23, 0x1f, 0x18, 0x02, 0x02 },
+				 0x23, 0x1f, 0x18, 0x02 },
 			.len = 12,
 		},
 		{
 			.namestr = "EXAMPLE.COM",
 			.key = { 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14, 0x20,
-				 0x23, 0x1f, 0x18, 0x02, 0x02 },
+				 0x23, 0x1f, 0x18, 0x02 },
 			.len = 12,
 		},
 	};
@@ -101,6 +108,9 @@ ISC_RUN_TEST_IMPL(qpkey_name) {
 			dns_test_namefromstring(testcases[i].namestr, &fn1);
 		}
 		len = dns_qpkey_fromname(key, in);
+		if (verbose) {
+			qp_test_printkey(key, len);
+		}
 
 		assert_int_equal(testcases[i].len, len);
 		assert_memory_equal(testcases[i].key, key, len);
@@ -128,6 +138,9 @@ ISC_RUN_TEST_IMPL(qpkey_sort) {
 	} testcases[] = {
 		{ .namestr = "." },
 		{ .namestr = "\\000." },
+		{ .namestr = "\\000.\\000." },
+		{ .namestr = "\\000\\009." },
+		{ .namestr = "\\007." },
 		{ .namestr = "example.com." },
 		{ .namestr = "EXAMPLE.COM." },
 		{ .namestr = "www.example.com." },
@@ -599,8 +612,20 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) {
 	for (int i = 0; check[i].query != NULL; i++) {
 		dns_qpiter_t it;
 
-		namestr = NULL;
 		dns_test_namefromstring(check[i].query, &fn1);
+
+		/*
+		 * normalize the expected predecessor name, in
+		 * case it has escaped characters, so we can compare
+		 * apples to apples.
+		 */
+		dns_fixedname_t fn3;
+		dns_name_t *expred = dns_fixedname_initname(&fn3);
+		char *predstr = NULL;
+		dns_test_namefromstring(check[i].predecessor, &fn3);
+		result = dns_name_tostring(expred, &predstr, mctx);
+		assert_int_equal(result, ISC_R_SUCCESS);
+
 		result = dns_qp_lookup(qp, name, NULL, &it, NULL, NULL, NULL);
 #if 0
 		fprintf(stderr, "%s: expected %s got %s\n", check[i].query,
@@ -630,15 +655,16 @@ check_predecessors(dns_qp_t *qp, struct check_predecessors check[]) {
 		result = dns_name_tostring(pred, &namestr, mctx);
 #if 0
 		fprintf(stderr, "... expected predecessor %s got %s\n",
-			check[i].predecessor, namestr);
+			predstr, namestr);
 #endif
 		assert_int_equal(result, ISC_R_SUCCESS);
-		assert_string_equal(namestr, check[i].predecessor);
+		assert_string_equal(namestr, predstr);
 
 #if 0
 		fprintf(stderr, "%d: remaining names after %s:\n", i, namestr);
 #endif
 		isc_mem_free(mctx, namestr);
+		isc_mem_free(mctx, predstr);
 
 		int j = 0;
 		while (dns_qpiter_next(&it, name, NULL, NULL) == ISC_R_SUCCESS)
@@ -841,6 +867,27 @@ ISC_RUN_TEST_IMPL(fixiterator) {
 
 	check_predecessors(qp, check3);
 	dns_qp_destroy(&qp);
+
+	const char insert4[][64] = { ".", "\\000.", "\\000.\\000.",
+				     "\\000\\009.", "" };
+	i = 0;
+
+	dns_qp_create(mctx, &string_methods, NULL, &qp);
+	while (insert4[i][0] != '\0') {
+		insert_str(qp, insert4[i++]);
+	}
+
+	static struct check_predecessors check4[] = {
+		{ "\\007.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 },
+		{ "\\009.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 },
+		{ "\\045.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 },
+		{ "\\044.", "\\000\\009.", DNS_R_PARTIALMATCH, 0 },
+		{ "\\000.", ".", ISC_R_SUCCESS, 3 },
+		{ NULL, NULL, 0, 0 },
+	};
+
+	check_predecessors(qp, check4);
+	dns_qp_destroy(&qp);
 }
 
 ISC_TEST_LIST_START

tests/include/tests/qp.h

@@ -84,3 +84,9 @@ qp_test_dumptrie(dns_qpreadable_t qp);
  */
 void
 qp_test_dumpdot(dns_qp_t *qp);
+
+/*
+ * Print the name encoded in a QP key.
+ */
+void
+qp_test_printkey(const dns_qpkey_t key, size_t keylen);

tests/libtest/qp.c

@@ -23,6 +23,7 @@
 #include <isc/urcu.h>
 #include <isc/util.h>
 
+#include <dns/fixedname.h>
 #include <dns/name.h>
 #include <dns/qp.h>
 #include <dns/types.h>
@@ -338,4 +339,15 @@ qp_test_dumpdot(dns_qp_t *qp) {
 	printf("}\n");
 }
 
+void
+qp_test_printkey(const dns_qpkey_t key, size_t keylen) {
+	dns_fixedname_t fn;
+	dns_name_t *n = dns_fixedname_initname(&fn);
+	char txt[DNS_NAME_FORMATSIZE];
+
+	dns_qpkey_toname(key, keylen, n);
+	dns_name_format(n, txt, sizeof(txt));
+	printf("%s%s\n", txt, dns_name_isabsolute(n) ? "." : "");
+}
+
 /**********************************************************************/