X Tutup
Skip to content

Commit 69bedd0

Browse files
poetteringpoettering
authored
Merge pull request systemd#19009 from poettering/one-more-cname-fix
resolved: more CNAME redirect fixes
2 parents 1a2c2e1 + b1eea70 commit 69bedd0

File tree

7 files changed

+116
-43
lines changed

7 files changed

+116
-43
lines changed

‎src/resolve/resolved-dns-answer.c‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -879,9 +879,8 @@ void dns_answer_dump(DnsAnswer *answer, FILE *f) {
879879
}
880880

881881
fputs(t, f);
882-
883-
if (item->ifindex != 0 || item->rrsig || item->flags != 0)
884-
fputs("\t;", f);
882+
fputs("\t;", f);
883+
fprintf(f, " ttl=%" PRIu32, item->rr->ttl);
885884

886885
if (item->ifindex != 0)
887886
fprintf(f, " ifindex=%i", item->ifindex);
@@ -963,3 +962,22 @@ void dns_answer_randomize(DnsAnswer *a) {
963962
SWAP_TWO(a->items[i], a->items[k]);
964963
}
965964
}
965+
966+
uint32_t dns_answer_min_ttl(DnsAnswer *a) {
967+
uint32_t ttl = UINT32_MAX;
968+
DnsResourceRecord *rr;
969+
970+
/* Return the smallest TTL of all RRs in this answer */
971+
972+
DNS_ANSWER_FOREACH(rr, a) {
973+
/* Don't consider OPT (where the TTL field is used for other purposes than an actual TTL) */
974+
975+
if (dns_type_is_pseudo(rr->key->type) ||
976+
dns_class_is_pseudo(rr->key->class))
977+
continue;
978+
979+
ttl = MIN(ttl, rr->ttl);
980+
}
981+
982+
return ttl;
983+
}

‎src/resolve/resolved-dns-answer.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ void dns_answer_dump(DnsAnswer *answer, FILE *f);
8787

8888
void dns_answer_randomize(DnsAnswer *a);
8989

90+
uint32_t dns_answer_min_ttl(DnsAnswer *a);
91+
9092
DEFINE_TRIVIAL_CLEANUP_FUNC(DnsAnswer*, dns_answer_unref);
9193

9294
#define _DNS_ANSWER_FOREACH(q, kk, a) \

‎src/resolve/resolved-dns-cache.c‎

Lines changed: 67 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -312,19 +312,23 @@ static DnsCacheItem* dns_cache_get(DnsCache *c, DnsResourceRecord *rr) {
312312
return NULL;
313313
}
314314

315-
static usec_t calculate_until(DnsResourceRecord *rr, uint32_t nsec_ttl, usec_t timestamp, bool use_soa_minimum) {
315+
static usec_t calculate_until(
316+
DnsResourceRecord *rr,
317+
uint32_t min_ttl,
318+
uint32_t nsec_ttl,
319+
usec_t timestamp,
320+
bool use_soa_minimum) {
321+
316322
uint32_t ttl;
317323
usec_t u;
318324

319325
assert(rr);
320326

321-
ttl = MIN(rr->ttl, nsec_ttl);
327+
ttl = MIN(min_ttl, nsec_ttl);
322328
if (rr->key->type == DNS_TYPE_SOA && use_soa_minimum) {
323-
/* If this is a SOA RR, and it is requested, clamp to
324-
* the SOA's minimum field. This is used when we do
325-
* negative caching, to determine the TTL for the
326-
* negative caching entry. See RFC 2308, Section
327-
* 5. */
329+
/* If this is a SOA RR, and it is requested, clamp to the SOA's minimum field. This is used
330+
* when we do negative caching, to determine the TTL for the negative caching entry. See RFC
331+
* 2308, Section 5. */
328332

329333
if (ttl > rr->soa.minimum)
330334
ttl = rr->soa.minimum;
@@ -337,8 +341,7 @@ static usec_t calculate_until(DnsResourceRecord *rr, uint32_t nsec_ttl, usec_t t
337341
if (rr->expiry != USEC_INFINITY) {
338342
usec_t left;
339343

340-
/* Make use of the DNSSEC RRSIG expiry info, if we
341-
* have it */
344+
/* Make use of the DNSSEC RRSIG expiry info, if we have it */
342345

343346
left = LESS_BY(rr->expiry, now(CLOCK_REALTIME));
344347
if (u > left)
@@ -354,6 +357,7 @@ static void dns_cache_item_update_positive(
354357
DnsResourceRecord *rr,
355358
DnsAnswer *answer,
356359
DnsPacket *full_packet,
360+
uint32_t min_ttl,
357361
uint64_t query_flags,
358362
bool shared_owner,
359363
DnssecResult dnssec_result,
@@ -390,7 +394,7 @@ static void dns_cache_item_update_positive(
390394
dns_packet_unref(i->full_packet);
391395
i->full_packet = full_packet;
392396

393-
i->until = calculate_until(rr, UINT32_MAX, timestamp, false);
397+
i->until = calculate_until(rr, min_ttl, UINT32_MAX, timestamp, false);
394398
i->query_flags = query_flags & CACHEABLE_QUERY_FLAGS;
395399
i->shared_owner = shared_owner;
396400
i->dnssec_result = dnssec_result;
@@ -417,9 +421,10 @@ static int dns_cache_put_positive(
417421
const union in_addr_union *owner_address) {
418422

419423
_cleanup_(dns_cache_item_freep) DnsCacheItem *i = NULL;
420-
DnsCacheItem *existing;
421424
char key_str[DNS_RESOURCE_KEY_STRING_MAX];
422-
int r, k;
425+
DnsCacheItem *existing;
426+
uint32_t min_ttl;
427+
int r;
423428

424429
assert(c);
425430
assert(rr);
@@ -431,11 +436,18 @@ static int dns_cache_put_positive(
431436
if (dns_type_is_pseudo(rr->key->type))
432437
return 0;
433438

439+
/* Determine the minimal TTL of all RRs in the answer plus the one by the main RR we are supposed to
440+
* cache. Since we cache whole answers to questions we should never return answers where only some
441+
* RRs are still valid, hence find the lowest here */
442+
min_ttl = dns_answer_min_ttl(answer);
443+
if (rr)
444+
min_ttl = MIN(min_ttl, rr->ttl);
445+
434446
/* New TTL is 0? Delete this specific entry... */
435-
if (rr->ttl <= 0) {
436-
k = dns_cache_remove_by_rr(c, rr);
447+
if (min_ttl <= 0) {
448+
r = dns_cache_remove_by_rr(c, rr);
437449
log_debug("%s: %s",
438-
k > 0 ? "Removed zero TTL entry from cache" : "Not caching zero TTL cache entry",
450+
r > 0 ? "Removed zero TTL entry from cache" : "Not caching zero TTL cache entry",
439451
dns_resource_key_to_string(rr->key, key_str, sizeof key_str));
440452
return 0;
441453
}
@@ -449,6 +461,7 @@ static int dns_cache_put_positive(
449461
rr,
450462
answer,
451463
full_packet,
464+
min_ttl,
452465
query_flags,
453466
shared_owner,
454467
dnssec_result,
@@ -476,7 +489,7 @@ static int dns_cache_put_positive(
476489
.rr = dns_resource_record_ref(rr),
477490
.answer = dns_answer_ref(answer),
478491
.full_packet = dns_packet_ref(full_packet),
479-
.until = calculate_until(rr, UINT32_MAX, timestamp, false),
492+
.until = calculate_until(rr, min_ttl, UINT32_MAX, timestamp, false),
480493
.query_flags = query_flags & CACHEABLE_QUERY_FLAGS,
481494
.shared_owner = shared_owner,
482495
.dnssec_result = dnssec_result,
@@ -578,9 +591,12 @@ static int dns_cache_put_negative(
578591
.full_packet = dns_packet_ref(full_packet),
579592
};
580593

594+
/* Determine how long to cache this entry. In case we have some RRs in the answer use the lowest TTL
595+
* of any of them. Typically that's the SOA's TTL, which is OK, but could possibly be lower because
596+
* of some other RR. Let's better take the lowest option here than a needlessly high one */
581597
i->until =
582598
i->type == DNS_CACHE_RCODE ? timestamp + CACHE_TTL_STRANGE_RCODE_USEC :
583-
calculate_until(soa, nsec_ttl, timestamp, true);
599+
calculate_until(soa, dns_answer_min_ttl(answer), nsec_ttl, timestamp, true);
584600

585601
if (i->type == DNS_CACHE_NXDOMAIN) {
586602
/* NXDOMAIN entries should apply equally to all types, so we use ANY as
@@ -696,7 +712,7 @@ int dns_cache_put(
696712
* short time.) */
697713

698714
if (IN_SET(rcode, DNS_RCODE_SUCCESS, DNS_RCODE_NXDOMAIN)) {
699-
if (dns_answer_size(answer) <= 0) {
715+
if (dns_answer_isempty(answer)) {
700716
if (key) {
701717
char key_str[DNS_RESOURCE_KEY_STRING_MAX];
702718

@@ -785,9 +801,8 @@ int dns_cache_put(
785801
if (r > 0)
786802
return 0;
787803

788-
/* But not if it has a matching CNAME/DNAME (the negative
789-
* caching will be done on the canonical name, not on the
790-
* alias) */
804+
/* But not if it has a matching CNAME/DNAME (the negative caching will be done on the canonical name,
805+
* not on the alias) */
791806
r = dns_answer_find_cname_or_dname(answer, key, NULL, NULL);
792807
if (r < 0)
793808
goto fail;
@@ -803,8 +818,7 @@ int dns_cache_put(
803818
if (r == 0 && !weird_rcode)
804819
return 0;
805820
if (r > 0) {
806-
/* Refuse using the SOA data if it is unsigned, but the key is
807-
* signed */
821+
/* Refuse using the SOA data if it is unsigned, but the key is signed */
808822
if (FLAGS_SET(query_flags, SD_RESOLVED_AUTHENTICATED) &&
809823
(flags & DNS_ANSWER_AUTHENTICATED) == 0)
810824
return 0;
@@ -813,7 +827,7 @@ int dns_cache_put(
813827
if (cache_mode == DNS_CACHE_MODE_NO_NEGATIVE) {
814828
char key_str[DNS_RESOURCE_KEY_STRING_MAX];
815829
log_debug("Not caching negative entry for: %s, cache mode set to no-negative",
816-
dns_resource_key_to_string(key, key_str, sizeof key_str));
830+
dns_resource_key_to_string(key, key_str, sizeof key_str));
817831
return 0;
818832
}
819833

@@ -923,17 +937,26 @@ static int answer_add_clamp_ttl(
923937
assert(rr);
924938

925939
if (FLAGS_SET(query_flags, SD_RESOLVED_CLAMP_TTL)) {
940+
uint32_t left_ttl;
941+
942+
/* Let's determine how much time is left for this cache entry. Note that we round down, but
943+
* clamp this to be 1s at minimum, since we usually want records to remain cached better too
944+
* short a time than too long a time, but otoh don't want to return 0 ever, since that has
945+
* special semantics in various contexts — in particular in mDNS */
946+
947+
left_ttl = MAX(1U, LESS_BY(until, current) / USEC_PER_SEC);
948+
926949
patched = dns_resource_record_ref(rr);
927950

928-
r = dns_resource_record_clamp_ttl(&patched, LESS_BY(until, current) / USEC_PER_SEC);
951+
r = dns_resource_record_clamp_ttl(&patched, left_ttl);
929952
if (r < 0)
930953
return r;
931954

932955
rr = patched;
933956

934957
if (rrsig) {
935958
patched_rrsig = dns_resource_record_ref(rrsig);
936-
r = dns_resource_record_clamp_ttl(&patched_rrsig, LESS_BY(until, current) / USEC_PER_SEC);
959+
r = dns_resource_record_clamp_ttl(&patched_rrsig, left_ttl);
937960
if (r < 0)
938961
return r;
939962

@@ -1051,21 +1074,30 @@ int dns_cache_lookup(
10511074
DnsAnswerItem *item;
10521075

10531076
DNS_ANSWER_FOREACH_ITEM(item, j->answer) {
1054-
r = answer_add_clamp_ttl(&answer, item->rr, item->ifindex, item->flags, item->rrsig, query_flags, j->until, current);
1077+
r = answer_add_clamp_ttl(
1078+
&answer,
1079+
item->rr,
1080+
item->ifindex,
1081+
item->flags,
1082+
item->rrsig,
1083+
query_flags,
1084+
j->until,
1085+
current);
10551086
if (r < 0)
10561087
return r;
10571088
}
10581089
}
10591090

10601091
} else if (j->rr) {
1061-
r = answer_add_clamp_ttl(&answer,
1062-
j->rr,
1063-
j->ifindex,
1064-
FLAGS_SET(j->query_flags, SD_RESOLVED_AUTHENTICATED) ? DNS_ANSWER_AUTHENTICATED : 0,
1065-
NULL,
1066-
query_flags,
1067-
j->until,
1068-
current);
1092+
r = answer_add_clamp_ttl(
1093+
&answer,
1094+
j->rr,
1095+
j->ifindex,
1096+
FLAGS_SET(j->query_flags, SD_RESOLVED_AUTHENTICATED) ? DNS_ANSWER_AUTHENTICATED : 0,
1097+
NULL,
1098+
query_flags,
1099+
j->until,
1100+
current);
10691101
if (r < 0)
10701102
return r;
10711103
}

‎src/resolve/resolved-dns-query.c‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1019,7 +1019,9 @@ static int dns_query_cname_redirect(DnsQuery *q, const DnsResourceRecord *cname)
10191019
q->question_utf8 = TAKE_PTR(nq_utf8);
10201020

10211021
dns_query_unref_candidates(q);
1022-
dns_query_reset_answer(q);
1022+
1023+
/* Note that we do *not* reset the answer here, because the answer we previously got might already
1024+
* include everything we need, let's check that first */
10231025

10241026
q->state = DNS_TRANSACTION_NULL;
10251027

@@ -1069,8 +1071,7 @@ int dns_query_process_cname(DnsQuery *q) {
10691071
if (r < 0)
10701072
return r;
10711073

1072-
/* Let's see if the answer can already answer the new
1073-
* redirected question */
1074+
/* Let's see if the answer can already answer the new redirected question */
10741075
r = dns_query_process_cname(q);
10751076
if (r != DNS_QUERY_NOMATCH)
10761077
return r;

‎src/resolve/resolved-dns-question.c‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,3 +445,21 @@ int dns_question_new_service(
445445

446446
return 0;
447447
}
448+
449+
/*
450+
* This function is not used in the code base, but is useful when debugging. Do not delete.
451+
*/
452+
void dns_question_dump(DnsQuestion *question, FILE *f) {
453+
DnsResourceKey *k;
454+
455+
if (!f)
456+
f = stdout;
457+
458+
DNS_QUESTION_FOREACH(k, question) {
459+
char buf[DNS_RESOURCE_KEY_STRING_MAX];
460+
461+
fputc('\t', f);
462+
fputs(dns_resource_key_to_string(k, buf, sizeof(buf)), f);
463+
fputc('\n', f);
464+
}
465+
}

‎src/resolve/resolved-dns-question.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ int dns_question_is_equal(DnsQuestion *a, DnsQuestion *b);
3333

3434
int dns_question_cname_redirect(DnsQuestion *q, const DnsResourceRecord *cname, DnsQuestion **ret);
3535

36+
void dns_question_dump(DnsQuestion *q, FILE *f);
37+
3638
const char *dns_question_first_name(DnsQuestion *q);
3739

3840
static inline size_t dns_question_size(DnsQuestion *q) {

‎src/resolve/resolved-dns-stub.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,7 @@ static int dns_stub_collect_answer_by_section(
275275
dns_type_is_dnssec(item->rr->key->type))
276276
continue;
277277

278-
if (((item->flags ^ section) & (DNS_ANSWER_SECTION_ANSWER|DNS_ANSWER_SECTION_AUTHORITY|DNS_ANSWER_SECTION_ADDITIONAL)) != 0)
278+
if (((item->flags ^ section) & DNS_ANSWER_MASK_SECTIONS) != 0)
279279
continue;
280280

281281
r = reply_add_with_rrsig(
@@ -761,7 +761,7 @@ static void dns_stub_query_complete(DnsQuery *q) {
761761
* and keep adding all RRs in the CNAME chain. */
762762
r = dns_stub_assign_sections(
763763
q,
764-
q->request_packet->question,
764+
dns_query_question_for_protocol(q, DNS_PROTOCOL_DNS),
765765
dns_stub_reply_with_edns0_do(q));
766766
if (r < 0) {
767767
log_debug_errno(r, "Failed to assign sections: %m");

0 commit comments

Comments
 (0)
X Tutup