diff options
author | Lennart Poettering <lennart@poettering.net> | 2021-03-25 11:43:52 +0100 |
---|---|---|
committer | Lennart Poettering <lennart@poettering.net> | 2021-03-25 13:12:19 +0100 |
commit | 1db8e6d1db0880de240e5598e28d24d708479434 (patch) | |
tree | e25b4f31bbb7c1f50de54ac3b87b7126655a0160 | |
parent | d451f0e84b1909ca0d24a1e3c135d5ffa2d8613c (diff) | |
download | systemd-1db8e6d1db0880de240e5598e28d24d708479434.tar.gz |
resolved: split dns_query_process_cname() into two separate functions
This does some refactoring: the dns_query_process_cname() function
becomes two: dns_query_process_cname_one() and
dns_query_process_cname_many(). The former will process exactly one
CNAME chain element, the latter will follow a chain for as long as
possible within the current packet.
dns_query_process_cname_many() is mostly identical to the old
dns_query_process_cname(), and all existing code is moved over to using
that.
This is mostly preparation for the next commit, where we make direct use
of dns_query_process_cname_one().
This also renames the DNS_QUERY_RESTARTED return value to
DNS_QUERY_CNAME. That's because in the dns_query_process_cname_many()
case as before if we return this we restarted the query in case we
reached the end of the chain without a conclusive answer, as before. But
in dns_query_process_cname_one() we'll only go one step anyway, and
leave restarting if needed to the caller. Hence DNS_QUERY_RESTARTED is a
bit of a misnomer in that case.
This also gets rid of the weird tail recursion in
dns_query_process_cname() and replaces it with an explicit loop in
dns_query_process_cname_many(). The old recursion wasn't a security
issue since we put a limit on the number of CNAMEs we follow anyway, but
it's still icky to scale stack use by that.
-rw-r--r-- | src/resolve/resolved-bus.c | 20 | ||||
-rw-r--r-- | src/resolve/resolved-dns-query.c | 82 | ||||
-rw-r--r-- | src/resolve/resolved-dns-query.h | 5 | ||||
-rw-r--r-- | src/resolve/resolved-dns-stub.c | 4 | ||||
-rw-r--r-- | src/resolve/resolved-varlink.c | 8 |
5 files changed, 91 insertions, 28 deletions
diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 032ed0256b..c3624669ce 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -195,14 +195,14 @@ static void bus_method_resolve_hostname_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -486,14 +486,14 @@ static void bus_method_resolve_address_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -660,14 +660,14 @@ static void bus_method_resolve_record_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; r = sd_bus_message_new_method_return(q->bus_request, &reply); @@ -1107,8 +1107,8 @@ static void resolve_service_hostname_complete(DnsQuery *q) { return; } - r = dns_query_process_cname(q); - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + r = dns_query_process_cname_many(q); + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; /* This auxiliary lookup is finished or failed, let's see if all are finished now. */ @@ -1180,14 +1180,14 @@ static void bus_method_resolve_service_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = sd_bus_reply_method_errorf(q->bus_request, BUS_ERROR_CNAME_LOOP, "CNAME loop detected, or CNAME resolving disabled on '%s'", dns_query_string(q)); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 8bc0607983..3a908a6b54 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -1036,7 +1036,7 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname) return 0; } -int dns_query_process_cname(DnsQuery *q) { +int dns_query_process_cname_one(DnsQuery *q) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *cname = NULL; DnsQuestion *question; DnsResourceRecord *rr; @@ -1046,6 +1046,22 @@ int dns_query_process_cname(DnsQuery *q) { assert(q); + /* Processes a CNAME redirect if there's one. Returns one of three values: + * + * CNAME_QUERY_MATCH → direct RR match, caller should just use the RRs in this answer (and not + * bother with any CNAME/DNAME stuff) + * + * CNAME_QUERY_NOMATCH → no match at all, neither direct nor CNAME/DNAME, caller might decide to + * restart query or take things as NODATA reply. + * + * CNAME_QUERY_CNAME → no direct RR match, but a CNAME/DNAME match that we now followed for one step. + * + * The function might also return a failure, in particular -ELOOP if we encountered too many + * CNAMEs/DNAMEs in a chain or if following CNAMEs/DNAMEs was turned off. + * + * Note that this function doesn't actually restart the query. The caller can decide to do that in + * case of CNAME_QUERY_CNAME, though. */ + if (!IN_SET(q->state, DNS_TRANSACTION_SUCCESS, DNS_TRANSACTION_NULL)) return DNS_QUERY_NOMATCH; @@ -1112,17 +1128,63 @@ int dns_query_process_cname(DnsQuery *q) { if (r < 0) return r; - /* Let's see if the answer can already answer the new redirected question */ - r = dns_query_process_cname(q); - if (r != DNS_QUERY_NOMATCH) - return r; + return DNS_QUERY_CNAME; /* Tell caller that we did a single CNAME/DNAME redirection step */ +} - /* OK, it cannot, let's begin with the new query */ - r = dns_query_go(q); - if (r < 0) - return r; +int dns_query_process_cname_many(DnsQuery *q) { + int r; - return DNS_QUERY_RESTARTED; /* We restarted the query for a new cname */ + assert(q); + + /* Follows CNAMEs through the current packet: as long as the current packet can fulfill our + * redirected CNAME queries we keep going, and restart the query once the current packet isn't good + * enough anymore. It's a wrapper around dns_query_process_cname_one() and returns the same values, + * but with extended semantics. Specifically: + * + * DNS_QUERY_MATCH → as above + * + * DNS_QUERY_CNAME → we ran into a CNAME/DNAME redirect that we could not answer from the current + * message, and thus restarted the query to resolve it. + * + * DNS_QUERY_NOMATCH → we reached the end of CNAME/DNAME chain, and there are no direct matches nor a + * CNAME/DNAME match. i.e. this is a NODATA case. + * + * Note that this function will restart the query for the caller if needed, and that's the case + * DNS_QUERY_CNAME is returned. + */ + + r = dns_query_process_cname_one(q); + if (r != DNS_QUERY_CNAME) + return r; /* The first redirect is special: if it doesn't answer the question that's no + * reason to restart the query, we just accept this as a NODATA answer. */ + + for (;;) { + r = dns_query_process_cname_one(q); + if (r < 0 || r == DNS_QUERY_MATCH) + return r; + if (r == DNS_QUERY_NOMATCH) { + /* OK, so we followed one or more CNAME/DNAME RR but the existing packet can't answer + * this. Let's restart the query hence, with the new question. Why the different + * handling than the first chain element? Because if the server answers a direct + * question with an empty answer then this is a NODATA response. But if it responds + * with a CNAME chain that ultimately is incomplete (i.e. a non-empty but truncated + * CNAME chain) then we better follow up ourselves and ask for the rest of the + * chain. This is particular relevant since our cache will store CNAME/DNAME + * redirects that we learnt about for lookups of certain DNS types, but later on we + * can reuse this data even for other DNS types, but in that case need to follow up + * with the final lookup of the chain ourselves with the RR type we ourselves are + * interested in. */ + r = dns_query_go(q); + if (r < 0) + return r; + + return DNS_QUERY_CNAME; + } + + /* So we found a CNAME that the existing packet already answers, again via a CNAME, let's + * continue going then. */ + assert(r == DNS_QUERY_CNAME); + } } DnsQuestion* dns_query_question_for_protocol(DnsQuery *q, DnsProtocol protocol) { diff --git a/src/resolve/resolved-dns-query.h b/src/resolve/resolved-dns-query.h index 5d96cc06f8..39bf341d68 100644 --- a/src/resolve/resolved-dns-query.h +++ b/src/resolve/resolved-dns-query.h @@ -112,7 +112,7 @@ struct DnsQuery { enum { DNS_QUERY_MATCH, DNS_QUERY_NOMATCH, - DNS_QUERY_RESTARTED, + DNS_QUERY_CNAME, }; DnsQueryCandidate* dns_query_candidate_ref(DnsQueryCandidate*); @@ -129,7 +129,8 @@ int dns_query_make_auxiliary(DnsQuery *q, DnsQuery *auxiliary_for); int dns_query_go(DnsQuery *q); void dns_query_ready(DnsQuery *q); -int dns_query_process_cname(DnsQuery *q); +int dns_query_process_cname_one(DnsQuery *q); +int dns_query_process_cname_many(DnsQuery *q); void dns_query_complete(DnsQuery *q, DnsTransactionState state); diff --git a/src/resolve/resolved-dns-stub.c b/src/resolve/resolved-dns-stub.c index 7cbbdc725c..cb5d075849 100644 --- a/src/resolve/resolved-dns-stub.c +++ b/src/resolve/resolved-dns-stub.c @@ -772,7 +772,7 @@ static void dns_stub_query_complete(DnsQuery *q) { switch (q->state) { case DNS_TRANSACTION_SUCCESS: - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { /* CNAME loop, let's send what we already have */ log_debug_errno(r, "Detected CNAME loop, returning what we already have."); (void) dns_stub_send_reply(q, q->answer_rcode); @@ -782,7 +782,7 @@ static void dns_stub_query_complete(DnsQuery *q) { log_debug_errno(r, "Failed to process CNAME: %m"); break; } - if (r == DNS_QUERY_RESTARTED) + if (r == DNS_QUERY_CNAME) return; _fallthrough_; diff --git a/src/resolve/resolved-varlink.c b/src/resolve/resolved-varlink.c index 77e75b8a8d..27d8c8967e 100644 --- a/src/resolve/resolved-varlink.c +++ b/src/resolve/resolved-varlink.c @@ -158,14 +158,14 @@ static void vl_method_resolve_hostname_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = varlink_error(q->varlink_request, "io.systemd.Resolve.CNAMELoop", NULL); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); @@ -395,14 +395,14 @@ static void vl_method_resolve_address_complete(DnsQuery *q) { goto finish; } - r = dns_query_process_cname(q); + r = dns_query_process_cname_many(q); if (r == -ELOOP) { r = varlink_error(q->varlink_request, "io.systemd.Resolve.CNAMELoop", NULL); goto finish; } if (r < 0) goto finish; - if (r == DNS_QUERY_RESTARTED) /* This was a cname, and the query was restarted. */ + if (r == DNS_QUERY_CNAME) /* This was a cname, and the query was restarted. */ return; question = dns_query_question_for_protocol(q, q->answer_protocol); |