Fix delegation database NOEXACT lookup for top-level names

dns__deleg_lookup() with DNS_DBFIND_NOEXACT is supposed to return
the deepest proper ancestor of the lookup name.  It called
getparentnode() to step up from an exact match, but getparentnode()
only iterated while the chain length was >= 2.  When the chain
contained a single entry (the exact match itself with no ancestor
stored in the trie), the loop did not execute and left the caller
looking at the exact match.  The subsequent isactive() check then
returned success and the function reported the exact match as the
"deepest ancestor", violating NOEXACT semantics.

This was observable as the resolver picking the child-side
delegation for an at-parent type (e.g. a DS query for a TLD), then
sending the query to the child's own nameservers and recovering via
the "chase DS servers" path.

Have getparentnode() set '*node' to NULL when it cannot find an
active proper ancestor, and make dns__deleg_lookup() NULL-check
before returning, matching the canonical NOEXACT implementation in
dns_zt_find().  Update the deleg unit test to expect NOTFOUND for
the top-level-no-parent case.

diff --git a/lib/dns/deleg.c b/lib/dns/deleg.c
index dc7f5a42dacb83fcc43a8559c21c02c89f127ffc..28fb777ef889392745fcdf9f10ef37577dd24084 100644 (file)
--- a/lib/dns/deleg.c
+++ b/lib/dns/deleg.c
@@ -267,14 +267,22 @@ getparentnode(dns_qpchain_t *chain, delegdb_node_t **node, dns_ttl_t now) {
        size_t len = dns_qpchain_length(chain);
 
        while (len >= 2) {
-               *node = NULL;
-               dns_qpchain_node(chain, len - 2, (void **)node, NULL);
+               delegdb_node_t *parent = NULL;
+               dns_qpchain_node(chain, len - 2, (void **)&parent, NULL);
 
-               if (isactive(*node, now)) {
-                       break;
+               if (isactive(parent, now)) {
+                       *node = parent;
+                       return;
                }
                len--;
        }
+
+       /*
+        * No active proper ancestor was found in the chain.  Signal
+        * "no parent" so the caller does not mistake the original
+        * matched node for an ancestor.
+        */
+       *node = NULL;
 }
 
 /*
@@ -309,27 +317,36 @@ dns__deleg_lookup(dns_delegdb_t *delegdb, dns_qpread_t *qpr,
                dns_name_copy(&node->zonecut, deepestzonecut);
        }
 
-       if (result == ISC_R_SUCCESS && (noexact || !isactive(node, now))) {
-               getparentnode(&chain, &node, now);
-       } else if (result == DNS_R_PARTIALMATCH && !isactive(node, now)) {
+       /*
+        * Walk up the chain when:
+        *  - we have an exact match but the caller asked for NOEXACT
+        *    (i.e. the caller wants the deepest *proper* ancestor), or
+        *  - the matched node is no longer active and we need to fall
+        *    back to the closest still-active ancestor (this applies
+        *    equally to exact and partial matches).
+        *
+        * getparentnode() sets 'node' to NULL when no active ancestor
+        * exists in the chain, so we must NULL-check before dereferencing
+        * 'node' below.
+        */
+       if ((result == ISC_R_SUCCESS && noexact) || !isactive(node, now)) {
                getparentnode(&chain, &node, now);
        }
 
-       result = isactive(node, now) ? ISC_R_SUCCESS : ISC_R_NOTFOUND;
-       if (result == ISC_R_SUCCESS) {
+       if (node != NULL && isactive(node, now)) {
                dns_name_copy(&node->zonecut, zonecut);
                INSIST(node->delegset);
                dns_delegset_attach(node->delegset, delegsetp);
                ISC_SIEVE_MARK(node, visited);
-       } else {
-               /*
-                * FIXME: if we lookup something that has expired, we need
-                * either the "deadnodes" (see qpcache) mechanism here - or call
-                * something like isc_async_run(delete_me, node).
-                */
+               return ISC_R_SUCCESS;
        }
 
-       return result;
+       /*
+        * FIXME: if we lookup something that has expired, we need
+        * either the "deadnodes" (see qpcache) mechanism here - or call
+        * something like isc_async_run(delete_me, node).
+        */
+       return ISC_R_NOTFOUND;
 }
 
 isc_result_t

diff --git a/tests/dns/deleg_test.c b/tests/dns/deleg_test.c
index 904eaeef00c86101c3a600671042a1981c4e7dbc..e27b7b1eb74f819c1461a0491e1f0c2fd563009e 100644 (file)
--- a/tests/dns/deleg_test.c
+++ b/tests/dns/deleg_test.c
@@ -124,13 +124,16 @@ lookupdb(dns_delegdb_t *db, const char *namestr, isc_stdtime_t now,
                   *expectedzc = dns_fixedname_initname(&fexpectedzc),
                   *zonecut = dns_fixedname_initname(&fzonecut);
 
-       dns_name_fromstring(expectedzc, expectedzcstr, NULL, 0, NULL);
+       if (expectedzcstr != NULL) {
+               dns_name_fromstring(expectedzc, expectedzcstr, NULL, 0, NULL);
+       }
        dns_name_fromstring(name, namestr, NULL, 0, NULL);
        result = dns_delegdb_lookup(db, name, now, options, zonecut, NULL,
                                    delegsetp);
 
        if (result == ISC_R_SUCCESS) {
                assert_non_null(*delegsetp);
+               assert_non_null(expectedzcstr);
                assert_true(dns_name_equal(zonecut, expectedzc));
        } else {
                assert_null(*delegsetp);
@@ -411,15 +414,22 @@ noexacttests(ISC_ATTR_UNUSED void *arg) {
                const char *name;
                const char *expected;
                const char *noexactexpected;
+               isc_result_t noexactresult;
                dns_ttl_t ttl;
        } zonecuts[] = {
-               { "stuff.", "stuff.", "stuff.", 30 },
-               { "foo.stuff.", "foo.stuff.", "stuff.", 30 },
-               { "expired.foo.stuff.", "foo.stuff.", "foo.stuff.", 1 },
+               /*
+                * "stuff." has no proper ancestor in the trie, so a
+                * NOEXACT lookup must return NOTFOUND rather than the
+                * exact match itself.
+                */
+               { "stuff.", "stuff.", NULL, ISC_R_NOTFOUND, 30 },
+               { "foo.stuff.", "foo.stuff.", "stuff.", ISC_R_SUCCESS, 30 },
+               { "expired.foo.stuff.", "foo.stuff.", "foo.stuff.",
+                 ISC_R_SUCCESS, 1 },
                { "bar.expired.foo.stuff.", "bar.expired.foo.stuff.",
-                 "foo.stuff.", 30 },
+                 "foo.stuff.", ISC_R_SUCCESS, 30 },
                { "baz.bar.expired.foo.stuff.", "baz.bar.expired.foo.stuff.",
-                 "bar.expired.foo.stuff.", 30 }
+                 "bar.expired.foo.stuff.", ISC_R_SUCCESS, 30 }
        };
 
        for (size_t i = 0; i < ARRAY_SIZE(zonecuts); i++) {
@@ -440,8 +450,10 @@ noexacttests(ISC_ATTR_UNUSED void *arg) {
                result = lookupdb(db, zonecuts[i].name, now + 1,
                                  DNS_DBFIND_NOEXACT,
                                  zonecuts[i].noexactexpected, &delegset);
-               assert_int_equal(result, ISC_R_SUCCESS);
-               dns_delegset_detach(&delegset);
+               assert_int_equal(result, zonecuts[i].noexactresult);
+               if (result == ISC_R_SUCCESS) {
+                       dns_delegset_detach(&delegset);
+               }
        }
 
        result = lookupdb(db, "gee.expired.foo.stuff.", now + 1, 0,