From: Yu Watanabe Date: Wed, 8 Nov 2023 11:55:50 +0000 (+0900) Subject: resolve/mdns: split out mdns_make_dummy_packet() X-Git-Tag: v255-rc2~74^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7645e5a8133be15c471abf172af601b83a7f9cf4;p=thirdparty%2Fsystemd.git resolve/mdns: split out mdns_make_dummy_packet() Then, this fixes the following issues: - if dns_packet_append_zone() for other transaction is failed with EMSGSIZE, the previously added key was not removed, - if dns_transaction_prepare() for other transaction returns 0, then we restated the loop without dropping previously appended keys, which might not be necessary any more. --- diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index c5c0a2312fd..696fce532a4 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1786,21 +1786,20 @@ static int dns_packet_append_zone(DnsPacket *p, DnsTransaction *t, DnsResourceKe return dns_packet_append_answer(p, answer, nscount); } -static int dns_transaction_make_packet_mdns(DnsTransaction *t) { +static int mdns_make_dummy_packet(DnsTransaction *t, DnsPacket **ret_packet, Set **ret_keys) { _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; _cleanup_set_free_ Set *keys = NULL; - unsigned qdcount, ancount = 0 /* avoid false maybe-uninitialized warning */, nscount; bool add_known_answers = false; + unsigned qdcount; usec_t ts; int r; assert(t); + assert(t->scope); assert(t->scope->protocol == DNS_PROTOCOL_MDNS); + assert(ret_packet); + assert(ret_keys); - /* Discard any previously prepared packet, so we can start over and coalesce again */ - t->sent = dns_packet_unref(t->sent); - - /* First, create a dummy packet to calculate packet size. */ r = dns_packet_new_query(&p, t->scope->protocol, 0, false); if (r < 0) return r; @@ -1823,120 +1822,139 @@ static int dns_transaction_make_packet_mdns(DnsTransaction *t) { if (r < 0) return r; - /* - * For mDNS, we want to coalesce as many open queries in pending transactions into one single - * query packet on the wire as possible. To achieve that, we iterate through all pending transactions - * in our current scope, and see whether their timing constraints allow them to be sent. - */ - assert_se(sd_event_now(t->scope->manager->event, CLOCK_BOOTTIME, &ts) >= 0); - for (bool restart = true; restart;) { - restart = false; - LIST_FOREACH(transactions_by_scope, other, t->scope->transactions) { - size_t saved_packet_size; - bool append = false; + LIST_FOREACH(transactions_by_scope, other, t->scope->transactions) { - /* Skip ourselves */ - if (other == t) - continue; - - if (other->state != DNS_TRANSACTION_PENDING) - continue; - - if (other->next_attempt_after > ts) - continue; + /* Skip ourselves */ + if (other == t) + continue; - if (!set_contains(keys, dns_transaction_key(other))) { - r = dns_packet_append_key(p, dns_transaction_key(other), 0, &saved_packet_size); - /* If we can't stuff more questions into the packet, just give up. - * One of the 'other' transactions will fire later and take care of the rest. */ - if (r == -EMSGSIZE) - break; - if (r < 0) - return r; + if (other->state != DNS_TRANSACTION_PENDING) + continue; - r = dns_packet_append_zone(p, t, dns_transaction_key(other), NULL); - if (r == -EMSGSIZE) - break; - if (r < 0) - return r; + if (other->next_attempt_after > ts) + continue; - append = true; - } + if (!set_contains(keys, dns_transaction_key(other))) { + size_t saved_packet_size; - r = dns_transaction_prepare(other, ts); + r = dns_packet_append_key(p, dns_transaction_key(other), 0, &saved_packet_size); + /* If we can't stuff more questions into the packet, just give up. + * One of the 'other' transactions will fire later and take care of the rest. */ + if (r == -EMSGSIZE) + break; if (r < 0) return r; - if (r == 0) { - if (append) - dns_packet_truncate(p, saved_packet_size); - /* In this case, not only this transaction, but multiple transactions may be - * freed. Hence, we need to restart the loop. */ - restart = true; + r = dns_packet_append_zone(p, t, dns_transaction_key(other), NULL); + if (r == -EMSGSIZE) { + dns_packet_truncate(p, saved_packet_size); break; } + if (r < 0) + return r; - usec_t timeout = transaction_get_resend_timeout(other); - r = dns_transaction_setup_timeout(other, timeout, usec_add(ts, timeout)); + r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(other)); if (r < 0) return r; + } - if (dns_key_is_shared(dns_transaction_key(other))) - add_known_answers = true; + r = dns_transaction_prepare(other, ts); + if (r < 0) + return r; + if (r == 0) + /* In this case, not only this transaction, but multiple transactions may be + * freed. Hence, we need to restart the loop. */ + return -EAGAIN; - if (append) { - r = set_ensure_put(&keys, &dns_resource_key_hash_ops, dns_transaction_key(other)); - if (r < 0) - return r; - } + usec_t timeout = transaction_get_resend_timeout(other); + r = dns_transaction_setup_timeout(other, timeout, usec_add(ts, timeout)); + if (r < 0) + return r; - qdcount++; - if (qdcount >= UINT16_MAX) - break; - } + if (dns_key_is_shared(dns_transaction_key(other))) + add_known_answers = true; + + qdcount++; + if (qdcount >= UINT16_MAX) + break; } - /* Append known answer section if we're asking for any shared record */ + DNS_PACKET_HEADER(p)->qdcount = htobe16(qdcount); + + /* Append known answers section if we're asking for any shared record */ if (add_known_answers) { r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, 0); if (r < 0) return r; + } + + *ret_packet = TAKE_PTR(p); + *ret_keys = TAKE_PTR(keys); + return add_known_answers; +} + +static int dns_transaction_make_packet_mdns(DnsTransaction *t) { + _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL, *dummy = NULL; + _cleanup_set_free_ Set *keys = NULL; + bool add_known_answers; + DnsResourceKey *k; + unsigned c; + int r; + + assert(t); + assert(t->scope->protocol == DNS_PROTOCOL_MDNS); + + /* Discard any previously prepared packet, so we can start over and coalesce again */ + t->sent = dns_packet_unref(t->sent); + + /* First, create a dummy packet to calculate the number of known answers to be appended in the first packet. */ + for (;;) { + r = mdns_make_dummy_packet(t, &dummy, &keys); + if (r == -EAGAIN) + continue; + if (r < 0) + return r; - ancount = be16toh(DNS_PACKET_HEADER(p)->ancount); + add_known_answers = r; + break; } /* Then, create actual packet. */ - p = dns_packet_unref(p); r = dns_packet_new_query(&p, t->scope->protocol, 0, false); if (r < 0) return r; /* Questions */ - DnsResourceKey *k; + c = 0; SET_FOREACH(k, keys) { r = dns_packet_append_key(p, k, 0, NULL); if (r < 0) return r; + c++; } - DNS_PACKET_HEADER(p)->qdcount = htobe16(qdcount); + DNS_PACKET_HEADER(p)->qdcount = htobe16(c); /* Known answers */ if (add_known_answers) { - r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, ancount); + usec_t ts; + + assert_se(sd_event_now(t->scope->manager->event, CLOCK_BOOTTIME, &ts) >= 0); + + r = dns_cache_export_shared_to_packet(&t->scope->cache, p, ts, be16toh(DNS_PACKET_HEADER(dummy)->ancount)); if (r < 0) return r; } /* Authorities */ - nscount = 0; + c = 0; SET_FOREACH(k, keys) { - r = dns_packet_append_zone(p, t, k, &nscount); + r = dns_packet_append_zone(p, t, k, &c); if (r < 0) return r; } - DNS_PACKET_HEADER(p)->nscount = htobe16(nscount); + DNS_PACKET_HEADER(p)->nscount = htobe16(c); t->sent = TAKE_PTR(p); return 0;