diff --git a/fuzz/dns_qpkey_name.c b/fuzz/dns_qpkey_name.c index ed9dd16b3c..cf818d5bc7 100644 --- a/fuzz/dns_qpkey_name.c +++ b/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)); diff --git a/lib/dns/forward.c b/lib/dns/forward.c index 20fdc5d4fb..1cc61115a8 100644 --- a/lib/dns/forward.c +++ b/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; diff --git a/lib/dns/include/dns/qp.h b/lib/dns/include/dns/qp.h index 2801c49040..f855f12908 100644 --- a/lib/dns/include/dns/qp.h +++ b/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 diff --git a/lib/dns/keytable.c b/lib/dns/keytable.c index 10614431b3..0b243b3c55 100644 --- a/lib/dns/keytable.c +++ b/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) { diff --git a/lib/dns/nametree.c b/lib/dns/nametree.c index 08140371c0..0f906c58de 100644 --- a/lib/dns/nametree.c +++ b/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; diff --git a/lib/dns/nta.c b/lib/dns/nta.c index 6df1a9f5ab..bcaf8eee57 100644 --- a/lib/dns/nta.c +++ b/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) { diff --git a/lib/dns/qp.c b/lib/dns/qp.c index e416bf8085..b3fe7926bb 100644 --- a/lib/dns/qp.c +++ b/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, - uint32_t *ival_r) { + dns_qpfind_t options, dns_name_t *foundname, + 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); } } diff --git a/lib/dns/zt.c b/lib/dns/zt.c index 7f2cedadd3..a65d527b42 100644 --- a/lib/dns/zt.c +++ b/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); diff --git a/tests/bench/qplookups.c b/tests/bench/qplookups.c index f7473ba07f..71564d6cab 100644 --- a/tests/bench/qplookups.c +++ b/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(); diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index fb95350f1f..80d076a4a3 100644 --- a/tests/dns/qp_test.c +++ b/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; - dns_test_namefromstring(testcases[i].namestr, &fn1); - in = dns_fixedname_name(&fn1); + in = dns_fixedname_initname(&fn1); + if (testcases[i].len != 0) { + dns_test_namefromstring(testcases[i].namestr, &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_name_t *name = dns_fixedname_name(&fixed); + dns_fixedname_t fn1, fn2; + 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, - check[i].options, isc_result_totext(check[i].result), + fprintf(stderr, "%s (flags %u) %s (expected %s) " + "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,21 +393,41 @@ 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[]){ - { "bar", 0, DNS_R_PARTIALMATCH, "" }, - { "bar.", 0, DNS_R_PARTIALMATCH, "" }, - { NULL, 0, 0, NULL }, - }); + 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 }, + }); dns_qp_destroy(&qp); } diff --git a/tests/include/tests/qp.h b/tests/include/tests/qp.h index 8e165001aa..d0d7357315 100644 --- a/tests/include/tests/qp.h +++ b/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 */ diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index 392297746a..f2b914472f 100644 --- a/tests/libtest/qp.c +++ b/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