resolved: explicitly avoid cyclic transaction dependencies

We already try hard not to create cyclic transaction dependencies, where
a transaction requires another one for DNSSEC validation purposes, which
in turn (possibly indirectly) pulls in the original transaction again,
thus resulting in a cyclic dependency and ultimately a deadlock since
each transaction waits for another one forever.

So far we wanted to avoid such cyclic dependencies by only going "up the
tree" when requesting auxiliary RRs and only going from one RR type to
another, but never back. However this turned out to be insufficient.
Consider a domain that publishes one or more DNSKEY but which has no DS
for it. A request for the domain's DNSKEY triggers a request for the
domain's DS, which will then fail, but return an NSEC, signed by the
DNSKEY. To validate that we'd request the DNSKEY again. Thus a DNSKEY
request results in a DS request which results in the original DNSKEY
request again. If the original lookup had been a DS lookup we'd end up
in the same cyclic dependency, hence we cannot statically break one of
them, since both requests are of course fully valid. Hence, do full
cyclic dependency checking: each time we are about to add a dependency
to a transaction, check if the transaction is already a dependency of
the dependency (recursively down the tree).

1 files changed, 37 insertions, 7 deletions

diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index c0626626f2..ce320e9a97 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -1233,6 +1233,28 @@ int dns_transaction_go(DnsTransaction *t) {
         return 1;
 }
 
+static int dns_transaction_find_cyclic(DnsTransaction *t, DnsTransaction *aux) {
+        DnsTransaction *n;
+        Iterator i;
+        int r;
+
+        assert(t);
+        assert(aux);
+
+        /* Try to find cyclic dependencies between transaction objects */
+
+        if (t == aux)
+                return 1;
+
+        SET_FOREACH(n, aux->notify_transactions, i) {
+                r = dns_transaction_find_cyclic(t, n);
+                if (r != 0)
+                        return r;
+        }
+
+        return r;
+}
+
 static int dns_transaction_add_dnssec_transaction(DnsTransaction *t, DnsResourceKey *key, DnsTransaction **ret) {
         DnsTransaction *aux;
         int r;
@@ -1251,6 +1273,18 @@ static int dns_transaction_add_dnssec_transaction(DnsTransaction *t, DnsResource
                         *ret = aux;
                         return 0;
                 }
+
+                r = dns_transaction_find_cyclic(t, aux);
+                if (r < 0)
+                        return r;
+                if (r > 0) {
+                        log_debug("Detected potential cyclic dependency, refusing to add transaction %" PRIu16 " (%s) as dependency for %" PRIu16 " (%s).",
+                                  aux->id,
+                                  strna(dns_transaction_key_string(aux)),
+                                  t->id,
+                                  strna(dns_transaction_key_string(t)));
+                        return -ELOOP;
+                }
         }
 
         r = set_ensure_allocated(&t->dnssec_transactions, NULL);
@@ -1287,12 +1321,6 @@ static int dns_transaction_request_dnssec_rr(DnsTransaction *t, DnsResourceKey *
         assert(t);
         assert(key);
 
-        r = dns_resource_key_equal(t->key, key);
-        if (r < 0)
-                return r;
-        if (r > 0) /* Don't go in circles */
-                return 0;
-
         /* Try to get the data from the trust anchor */
         r = dns_trust_anchor_lookup_positive(&t->scope->manager->trust_anchor, key, &a);
         if (r < 0)
@@ -1307,6 +1335,8 @@ static int dns_transaction_request_dnssec_rr(DnsTransaction *t, DnsResourceKey *
 
         /* This didn't work, ask for it via the network/cache then. */
         r = dns_transaction_add_dnssec_transaction(t, key, &aux);
+        if (r == -ELOOP) /* This would result in a cyclic dependency */
+                return 0;
         if (r < 0)
                 return r;
 
@@ -1316,7 +1346,7 @@ static int dns_transaction_request_dnssec_rr(DnsTransaction *t, DnsResourceKey *
                         return r;
         }
 
-        return 0;
+        return 1;
 }
 
 static int dns_transaction_has_positive_answer(DnsTransaction *t, DnsAnswerFlags *flags) {