diff --git a/CHANGES b/CHANGES index 8ca7f9e5fd..5a4b6a0eed 100644 --- a/CHANGES +++ b/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] diff --git a/lib/dns/qp.c b/lib/dns/qp.c index 0329f424cb..e8ff8f66a7 100644 --- a/lib/dns/qp.c +++ b/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); } diff --git a/tests/dns/qp_test.c b/tests/dns/qp_test.c index efd1470360..9cfab6509b 100644 --- a/tests/dns/qp_test.c +++ b/tests/dns/qp_test.c @@ -38,6 +38,8 @@ #include #include +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 diff --git a/tests/include/tests/qp.h b/tests/include/tests/qp.h index d0d7357315..34a84491d8 100644 --- a/tests/include/tests/qp.h +++ b/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); diff --git a/tests/libtest/qp.c b/tests/libtest/qp.c index d6a1d3055f..fb65962bbf 100644 --- a/tests/libtest/qp.c +++ b/tests/libtest/qp.c @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -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) ? "." : ""); +} + /**********************************************************************/