modify dns_qp_findname_ancestor() to return found name

add a 'foundname' parameter to dns_qp_findname_ancestor(),
and use it to set the found name in dns_nametree.

this required adding a dns_qpkey_toname() function; that was
done by moving qp_test_keytoname() from the test library to qp.c.
added some more test cases and fixed bugs with the handling of
relative and empty names.

fuzz/dns_qpkey_name.c

@@ -56,7 +56,7 @@ LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
 
 	/* verify round-trip conversion of first name */
 	size_t keylen = dns_qpkey_fromname(key, namein);
-	qp_test_keytoname(key, keylen, nameout);
+	dns_qpkey_toname(key, keylen, nameout);
 
 	assert(dns_name_equal(namein, nameout));
 

lib/dns/forward.c

@@ -168,7 +168,7 @@ dns_fwdtable_find(dns_fwdtable_t *fwdtable, const dns_name_t *name,
 	REQUIRE(VALID_FWDTABLE(fwdtable));
 
 	dns_qpmulti_query(fwdtable->table, &qpr);
-	result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL);
+	result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL);
 	if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) {
 		dns_forwarders_t *fwdrs = pval;
 		*forwardersp = fwdrs;

lib/dns/include/dns/qp.h

@@ -431,6 +431,17 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name);
  * \li  the length of the key
  */
 
+void
+dns_qpkey_toname(const dns_qpkey_t key, size_t keylen, dns_name_t *name);
+/*%<
+ * Convert a trie lookup key back into a DNS name.
+ *
+ * Requires:
+ * \li  `name` is a pointer to a valid `dns_name_t`
+ * \li  `name->buffer` is not NULL
+ * \li  `name->offsets` is not NULL
+ */
+
 isc_result_t
 dns_qp_getkey(dns_qpreadable_t qpr, const dns_qpkey_t search_key,
 	      size_t search_keylen, void **pval_r, uint32_t *ival_r);
@@ -469,7 +480,8 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r,
 
 isc_result_t
 dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name,
-			 dns_qpfind_t options, void **pval_r, uint32_t *ival_r);
+			 dns_qpfind_t options, dns_name_t *foundname,
+			 void **pval_r, uint32_t *ival_r);
 /*%<
  * Find a leaf in a qp-trie that is an ancestor domain of, or equal to, the
  * given DNS name.
@@ -477,12 +489,16 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name,
  * If the DNS_QPFIND_NOEXACT option is set, find the closest ancestor
  * domain that is not equal to the search name.
  *
+ * If 'foundname' is not NULL, it is updated to contain the name found.
+ *
  * The leaf values are assigned to whichever of `*pval_r` and `*ival_r`
  * are not null, unless the return value is ISC_R_NOTFOUND.
  *
  * Requires:
  * \li  `qpr` is a pointer to a readable qp-trie
  * \li  `name` is a pointer to a valid `dns_name_t`
+ * \li  `foundname` is a pointer to a valid `dns_name_t` with
+ *       buffer and offset space available, or is NULL.
  *
  * Returns:
  * \li  ISC_R_SUCCESS if an exact match was found

lib/dns/keytable.c

@@ -518,7 +518,7 @@ dns_keytable_finddeepestmatch(dns_keytable_t *keytable, const dns_name_t *name,
 	REQUIRE(foundname != NULL);
 
 	dns_qpmulti_query(keytable->table, &qpr);
-	result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL);
+	result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL);
 	keynode = pval;
 
 	if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) {
@@ -547,7 +547,7 @@ dns_keytable_issecuredomain(dns_keytable_t *keytable, const dns_name_t *name,
 	REQUIRE(wantdnssecp != NULL);
 
 	dns_qpmulti_query(keytable->table, &qpr);
-	result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL);
+	result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL);
 	if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) {
 		keynode = pval;
 		if (foundname != NULL) {

lib/dns/nametree.c

@@ -289,12 +289,9 @@ dns_nametree_covered(dns_nametree_t *nametree, const dns_name_t *name,
 	REQUIRE(VALID_NAMETREE(nametree));
 
 	dns_qpmulti_query(nametree->table, &qpr);
-	result = dns_qp_findname_ancestor(&qpr, name, 0, (void **)&node, NULL);
+	result = dns_qp_findname_ancestor(&qpr, name, 0, found, (void **)&node,
+					  NULL);
 	if (result == ISC_R_SUCCESS || result == DNS_R_PARTIALMATCH) {
-		if (found != NULL) {
-			dns_name_copy(node->name, found);
-		}
-
 		switch (nametree->type) {
 		case DNS_NAMETREE_BOOL:
 			ret = node->set;

lib/dns/nta.c

@@ -413,7 +413,7 @@ dns_ntatable_covered(dns_ntatable_t *ntatable, isc_stdtime_t now,
 
 	RWLOCK(&ntatable->rwlock, isc_rwlocktype_read);
 	dns_qpmulti_query(ntatable->table, &qpr);
-	result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL);
+	result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval, NULL);
 	nta = pval;
 
 	switch (result) {

lib/dns/qp.c

@@ -176,7 +176,7 @@ initialize_bits_for_byte(void) {
 			qp_shift_t letter = byte - 'A';
 			qp_shift_t bit = after_esc + skip_punct + letter;
 			dns_qp_bits_for_byte[byte] = bit;
-			/* to simplify reverse conversion in the tests */
+			/* to simplify reverse conversion */
 			bit_two++;
 		} else {
 			/* non-hostname characters need to be escaped */
@@ -216,7 +216,11 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name) {
 	dns_fixedname_t fixed;
 
 	REQUIRE(ISC_MAGIC_VALID(name, DNS_NAME_MAGIC));
-	REQUIRE(name->labels > 0);
+
+	if (name->labels == 0) {
+		key[0] = SHIFT_NOBYTE;
+		return (0);
+	}
 
 	if (name->offsets == NULL) {
 		dns_name_t *clone = dns_fixedname_initname(&fixed);
@@ -245,6 +249,85 @@ dns_qpkey_fromname(dns_qpkey_t key, const dns_name_t *name) {
 	return (len);
 }
 
+void
+dns_qpkey_toname(const dns_qpkey_t key, size_t keylen, dns_name_t *name) {
+	size_t locs[DNS_NAME_MAXLABELS];
+	size_t loc = 0, opos = 0;
+	size_t offset;
+
+	REQUIRE(ISC_MAGIC_VALID(name, DNS_NAME_MAGIC));
+	REQUIRE(name->buffer != NULL);
+	REQUIRE(name->offsets != NULL);
+
+	if (keylen == 0) {
+		dns_name_reset(name);
+		return;
+	}
+
+	isc_buffer_clear(name->buffer);
+
+	/* Scan the key looking for label boundaries */
+	for (offset = 0; offset <= keylen; offset++) {
+		INSIST(key[offset] >= SHIFT_NOBYTE &&
+		       key[offset] < SHIFT_OFFSET);
+		INSIST(loc < DNS_NAME_MAXLABELS);
+		if (qpkey_bit(key, keylen, offset) == SHIFT_NOBYTE) {
+			if (qpkey_bit(key, keylen, offset + 1) == SHIFT_NOBYTE)
+			{
+				locs[loc] = offset + 1;
+				goto scanned;
+			}
+			locs[loc++] = offset + 1;
+		} else if (offset == 0) {
+			/* This happens for a relative name */
+			locs[loc++] = offset;
+		}
+	}
+	UNREACHABLE();
+scanned:
+
+	/*
+	 * In the key the labels are encoded in reverse order, so
+	 * we step backward through the label boundaries, then forward
+	 * through the labels, to create the DNS wire format data.
+	 */
+	name->labels = loc;
+	while (loc-- > 0) {
+		uint8_t len = 0, *lenp = NULL;
+
+		/* Add a length byte to the name data and set an offset */
+		lenp = isc_buffer_used(name->buffer);
+		isc_buffer_putuint8(name->buffer, 0);
+		name->offsets[opos++] = name->length++;
+
+		/* Convert from escaped byte ranges to ASCII */
+		for (offset = locs[loc]; offset < locs[loc + 1] - 1; offset++) {
+			uint8_t bit = qpkey_bit(key, keylen, offset);
+			uint8_t byte = dns_qp_byte_for_bit[bit];
+			if (qp_common_character(byte)) {
+				isc_buffer_putuint8(name->buffer, byte);
+			} else {
+				byte += key[++offset] - SHIFT_BITMAP;
+				isc_buffer_putuint8(name->buffer, byte);
+			}
+			len++;
+		}
+
+		name->length += len;
+		*lenp = len;
+	}
+
+	/* Add a root label for absolute names */
+	if (key[0] == SHIFT_NOBYTE) {
+		name->attributes.absolute = true;
+		isc_buffer_putuint8(name->buffer, 0);
+		name->offsets[opos++] = name->length++;
+		name->labels++;
+	}
+
+	name->ndata = isc_buffer_base(name->buffer);
+}
+
 /*
  * Sentinel value for equal keys
  */
@@ -1811,8 +1894,8 @@ dns_qp_getname(dns_qpreadable_t qpr, const dns_name_t *name, void **pval_r,
 
 isc_result_t
 dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name,
-			 dns_qpfind_t options, void **pval_r,
+			 dns_qpfind_t options, dns_name_t *foundname,
-			 uint32_t *ival_r) {
+			 void **pval_r, uint32_t *ival_r) {
 	dns_qpreader_t *qp = dns_qpreader(qpr);
 	dns_qpkey_t search, found;
 	size_t searchlen, foundlen;
@@ -1827,6 +1910,7 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name,
 	} label[DNS_NAME_MAXLABELS];
 
 	REQUIRE(QP_VALID(qp));
+	REQUIRE(foundname == NULL || ISC_MAGIC_VALID(name, DNS_NAME_MAGIC));
 
 	searchlen = dns_qpkey_fromname(search, name);
 	if ((options & DNS_QPFIND_NOEXACT) != 0) {
@@ -1888,6 +1972,9 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name,
 	if (offset == QPKEY_EQUAL || offset == foundlen) {
 		SET_IF_NOT_NULL(pval_r, leaf_pval(n));
 		SET_IF_NOT_NULL(ival_r, leaf_ival(n));
+		if (foundname != NULL) {
+			dns_qpkey_toname(found, foundlen, foundname);
+		}
 		if (offset == QPKEY_EQUAL) {
 			return (result);
 		} else {
@@ -1899,6 +1986,10 @@ dns_qp_findname_ancestor(dns_qpreadable_t qpr, const dns_name_t *name,
 			n = ref_ptr(qp, label[labels].ref);
 			SET_IF_NOT_NULL(pval_r, leaf_pval(n));
 			SET_IF_NOT_NULL(ival_r, leaf_ival(n));
+			if (foundname != NULL) {
+				foundlen = leaf_qpkey(qp, n, found);
+				dns_qpkey_toname(found, foundlen, foundname);
+			}
 			return (DNS_R_PARTIALMATCH);
 		}
 	}

lib/dns/zt.c

@@ -181,9 +181,10 @@ dns_zt_find(dns_zt_t *zt, const dns_name_t *name, dns_ztfind_t options,
 		result = dns_qp_getname(&qpr, name, &pval, NULL);
 	} else if (exactopts == DNS_ZTFIND_NOEXACT) {
 		result = dns_qp_findname_ancestor(
-			&qpr, name, DNS_QPFIND_NOEXACT, &pval, NULL);
+			&qpr, name, DNS_QPFIND_NOEXACT, NULL, &pval, NULL);
 	} else {
-		result = dns_qp_findname_ancestor(&qpr, name, 0, &pval, NULL);
+		result = dns_qp_findname_ancestor(&qpr, name, 0, NULL, &pval,
+						  NULL);
 	}
 	dns_qpread_destroy(zt->multi, &qpr);
 

tests/bench/qplookups.c

@@ -254,7 +254,7 @@ main(int argc, char **argv) {
 	start = isc_time_monotonic();
 	for (i = 0; i < n; i++) {
 		name = dns_fixedname_name(&items[i]);
-		dns_qp_findname_ancestor(qp, name, 0, NULL, NULL);
+		dns_qp_findname_ancestor(qp, name, 0, NULL, NULL, NULL);
 	}
 	stop = isc_time_monotonic();
 
@@ -280,7 +280,7 @@ main(int argc, char **argv) {
 			++search->ndata[1];
 		}
 
-		dns_qp_findname_ancestor(qp, search, 0, NULL, NULL);
+		dns_qp_findname_ancestor(qp, search, 0, NULL, NULL, NULL);
 	}
 	stop = isc_time_monotonic();
 

tests/dns/qp_test.c

@@ -44,6 +44,11 @@ ISC_RUN_TEST_IMPL(qpkey_name) {
 		uint8_t key[512];
 		size_t len;
 	} testcases[] = {
+		{
+			.namestr = "",
+			.key = { 0x02 },
+			.len = 0,
+		},
 		{
 			.namestr = ".",
 			.key = { 0x02, 0x02 },
@@ -54,6 +59,16 @@ ISC_RUN_TEST_IMPL(qpkey_name) {
 			.key = { 0x03, 0x03, 0x02, 0x02 },
 			.len = 3,
 		},
+		{
+			.namestr = "com",
+			.key = { 0x16, 0x22, 0x20, 0x02 },
+			.len = 4,
+		},
+		{
+			.namestr = "com.",
+			.key = { 0x02, 0x16, 0x22, 0x20, 0x02 },
+			.len = 5,
+		},
 		{
 			.namestr = "example.com.",
 			.key = { 0x02, 0x16, 0x22, 0x20, 0x02, 0x18, 0x2b, 0x14,
@@ -80,15 +95,21 @@ ISC_RUN_TEST_IMPL(qpkey_name) {
 		dns_fixedname_t fn1, fn2;
 		dns_name_t *in = NULL, *out = NULL;
 
+		in = dns_fixedname_initname(&fn1);
+		if (testcases[i].len != 0) {
 			dns_test_namefromstring(testcases[i].namestr, &fn1);
-		in = dns_fixedname_name(&fn1);
+		}
 		len = dns_qpkey_fromname(key, in);
 
 		assert_int_equal(testcases[i].len, len);
 		assert_memory_equal(testcases[i].key, key, len);
+		/* also check key correctness for empty name */
+		if (len == 0) {
+			assert_int_equal(testcases[i].key[0], ((char *)key)[0]);
+		}
 
 		out = dns_fixedname_initname(&fn2);
-		qp_test_keytoname(key, len, out);
+		dns_qpkey_toname(key, len, out);
 		assert_true(dns_name_equal(in, out));
 	}
 }
@@ -258,19 +279,47 @@ static void
 check_partialmatch(dns_qp_t *qp, struct check_partialmatch check[]) {
 	for (int i = 0; check[i].query != NULL; i++) {
 		isc_result_t result;
-		dns_fixedname_t fixed;
+		dns_fixedname_t fn1, fn2;
-		dns_name_t *name = dns_fixedname_name(&fixed);
+		dns_name_t *name = dns_fixedname_initname(&fn1);
+		dns_name_t *foundname = dns_fixedname_initname(&fn2);
 		void *pval = NULL;
 
+		dns_test_namefromstring(check[i].query, &fn1);
+		result = dns_qp_findname_ancestor(qp, name, check[i].options,
+						  foundname, &pval, NULL);
+
 #if 0
-		fprintf(stderr, "%s %u %s %s\n", check[i].query,
+		fprintf(stderr, "%s (flags %u) %s (expected %s) "
-			check[i].options, isc_result_totext(check[i].result),
+			"value \"%s\" (expected \"%s\")\n",
+			check[i].query, check[i].options,
+			isc_result_totext(result),
+			isc_result_totext(check[i].result), (char *)pval,
 			check[i].found);
 #endif
-		dns_test_namefromstring(check[i].query, &fixed);
+
-		result = dns_qp_findname_ancestor(qp, name, check[i].options,
-						  &pval, NULL);
 		assert_int_equal(result, check[i].result);
+		if (result == ISC_R_SUCCESS) {
+			assert_true(dns_name_equal(name, foundname));
+		} else if (result == DNS_R_PARTIALMATCH) {
+			/*
+			 * there are cases where we may have passed a
+			 * query name that was relative to the zone apex,
+			 * and gotten back an absolute name from the
+			 * partial match. it's also possible for an
+			 * absolute query to get a partial match on a
+			 * node that had an empty name. in these cases,
+			 * sanity checking the relations between name
+			 * and foundname can trigger an assertion, so
+			 * let's just skip them.
+			 */
+			if (dns_name_isabsolute(name) ==
+			    dns_name_isabsolute(foundname))
+			{
+				assert_false(dns_name_equal(name, foundname));
+				assert_true(
+					dns_name_issubdomain(name, foundname));
+			}
+		}
 		if (check[i].found == NULL) {
 			assert_null(pval);
 		} else {
@@ -291,6 +340,7 @@ insert_str(dns_qp_t *qp, const char *str) {
 ISC_RUN_TEST_IMPL(partialmatch) {
 	isc_result_t result;
 	dns_qp_t *qp = NULL;
+	int i = 0;
 
 	dns_qp_create(mctx, &string_methods, NULL, &qp);
 
@@ -302,7 +352,10 @@ ISC_RUN_TEST_IMPL(partialmatch) {
 		"fooo.bar.", "web.foo.bar.", ".",	"",
 	};
 
-	int i = 0;
+	/*
+	 * omit the root node for now, otherwise we'll get "partial match"
+	 * results when we want "not found".
+	 */
 	while (insert[i][0] != '.') {
 		insert_str(qp, insert[i++]);
 	}
@@ -340,19 +393,39 @@ ISC_RUN_TEST_IMPL(partialmatch) {
 		{ "bar.", 0, DNS_R_PARTIALMATCH, "." },
 		{ "foo.bar.", 0, ISC_R_SUCCESS, "foo.bar." },
 		{ "foo.bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." },
+		{ "bar", 0, ISC_R_NOTFOUND, NULL },
+		{ "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "." },
 		{ NULL, 0, 0, NULL },
 	};
 	check_partialmatch(qp, check2);
 
-	/* what if entries in the trie are relative to the zone apex? */
+	/*
+	 * what if entries in the trie are relative to the zone apex
+	 * and there's no root node?
+	 */
 	dns_qpkey_t rootkey = { SHIFT_NOBYTE };
 	result = dns_qp_deletekey(qp, rootkey, 1, NULL, NULL);
 	assert_int_equal(result, ISC_R_SUCCESS);
+	check_partialmatch(
+		qp,
+		(struct check_partialmatch[]){
+			{ "bar", 0, ISC_R_NOTFOUND, NULL },
+			{ "bar", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL },
+			{ "bar.", 0, ISC_R_NOTFOUND, NULL },
+			{ "bar.", DNS_QPFIND_NOEXACT, ISC_R_NOTFOUND, NULL },
+			{ NULL, 0, 0, NULL },
+		});
+
+	/* what if there's a root node with an empty key? */
 	INSIST(insert[i][0] == '\0');
 	insert_str(qp, insert[i++]);
-	check_partialmatch(qp, (struct check_partialmatch[]){
+	check_partialmatch(
+		qp,
+		(struct check_partialmatch[]){
 			{ "bar", 0, DNS_R_PARTIALMATCH, "" },
+			{ "bar", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" },
 			{ "bar.", 0, DNS_R_PARTIALMATCH, "" },
+			{ "bar.", DNS_QPFIND_NOEXACT, DNS_R_PARTIALMATCH, "" },
 			{ NULL, 0, 0, NULL },
 		});
 

tests/include/tests/qp.h

@@ -29,13 +29,6 @@ qp_test_bittoascii(uint8_t bit);
 const char *
 qp_test_keytoascii(dns_qpkey_t key, size_t len);
 
-/*
- * Convert a trie lookup key back into a DNS name. Unlike the previous
- * functions, this is a complete inverse of dns_qpkey_fromname().
- */
-void
-qp_test_keytoname(const dns_qpkey_t key, size_t len, dns_name_t *name);
-
 /*
  * The maximum height of the trie
  */

tests/libtest/qp.c

@@ -61,80 +61,6 @@ qp_test_keytoascii(dns_qpkey_t key, size_t len) {
 	return ((const char *)key);
 }
 
-void
-qp_test_keytoname(const dns_qpkey_t key, size_t keylen, dns_name_t *name) {
-	size_t locs[DNS_NAME_MAXLABELS];
-	size_t loc = 0, opos = 0;
-	size_t offset;
-
-	REQUIRE(ISC_MAGIC_VALID(name, DNS_NAME_MAGIC));
-	REQUIRE(name->buffer != NULL);
-	REQUIRE(name->offsets != NULL);
-
-	isc_buffer_clear(name->buffer);
-
-	/* Scan the key looking for label boundaries */
-	for (offset = 0; offset <= keylen; offset++) {
-		INSIST(key[offset] >= SHIFT_NOBYTE &&
-		       key[offset] < SHIFT_OFFSET);
-		INSIST(loc < DNS_NAME_MAXLABELS);
-		if (qpkey_bit(key, keylen, offset) == SHIFT_NOBYTE) {
-			if (qpkey_bit(key, keylen, offset + 1) == SHIFT_NOBYTE)
-			{
-				locs[loc] = offset + 1;
-				goto scanned;
-			}
-			locs[loc++] = offset + 1;
-		} else if (offset == 0) {
-			/* This happens for a relative name */
-			locs[loc++] = offset;
-		}
-	}
-	UNREACHABLE();
-scanned:
-
-	/*
-	 * In the key the labels are encoded in reverse order, so
-	 * we step backward through the label boundaries, then forward
-	 * through the labels, to create the DNS wire format data.
-	 */
-	name->labels = loc;
-	while (loc-- > 0) {
-		uint8_t len = 0, *lenp = NULL;
-
-		/* Add a length byte to the name data and set an offset */
-		lenp = isc_buffer_used(name->buffer);
-		isc_buffer_putuint8(name->buffer, 0);
-		name->offsets[opos++] = name->length++;
-
-		/* Convert from escaped byte ranges to ASCII */
-		for (offset = locs[loc]; offset < locs[loc + 1] - 1; offset++) {
-			uint8_t bit = qpkey_bit(key, keylen, offset);
-			uint8_t byte = dns_qp_byte_for_bit[bit];
-			if (qp_common_character(byte)) {
-				isc_buffer_putuint8(name->buffer, byte);
-			} else {
-				byte += key[++offset] - SHIFT_BITMAP;
-				isc_buffer_putuint8(name->buffer, byte);
-			}
-			len++;
-		}
-
-		name->length += len;
-		*lenp = len;
-	}
-
-	/* Add a root label for absolute names */
-	if (key[0] == SHIFT_NOBYTE) {
-		name->attributes.absolute = true;
-		isc_buffer_putuint8(name->buffer, 0);
-		name->length++;
-		name->labels++;
-	}
-
-	name->ndata = isc_buffer_base(name->buffer);
-}
-
 /***********************************************************************
  *
  *  trie properties