Merge pull request #11841 from keszybz/dns-packet-speedup

DNS packet speedup

7 files changed, 77 insertions, 46 deletions

diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c
index d7252d3dac..44dc6552b4 100644
--- a/src/resolve/resolved-dns-answer.c
+++ b/src/resolve/resolved-dns-answer.c
@@ -87,40 +87,33 @@ int dns_answer_add(DnsAnswer *a, DnsResourceRecord *rr, int ifindex, DnsAnswerFl
                 if (a->items[i].ifindex != ifindex)
                         continue;
 
-                r = dns_resource_record_equal(a->items[i].rr, rr);
+                r = dns_resource_key_equal(a->items[i].rr->key, rr->key);
                 if (r < 0)
                         return r;
-                if (r > 0) {
-                        /* Don't mix contradicting TTLs (see below) */
-                        if ((rr->ttl == 0) != (a->items[i].rr->ttl == 0))
-                                return -EINVAL;
-
-                        /* Entry already exists, keep the entry with
-                         * the higher RR. */
-                        if (rr->ttl > a->items[i].rr->ttl) {
-                                dns_resource_record_ref(rr);
-                                dns_resource_record_unref(a->items[i].rr);
-                                a->items[i].rr = rr;
-                        }
+                if (r == 0)
+                        continue;
 
-                        a->items[i].flags |= flags;
-                        return 0;
-                }
+                /* There's already an RR of the same RRset in place! Let's see if the TTLs more or less
+                 * match. We don't really care if they match precisely, but we do care whether one is 0 and
+                 * the other is not. See RFC 2181, Section 5.2. */
+                if ((rr->ttl == 0) != (a->items[i].rr->ttl == 0))
+                        return -EINVAL;
 
-                r = dns_resource_key_equal(a->items[i].rr->key, rr->key);
+                r = dns_resource_record_payload_equal(a->items[i].rr, rr);
                 if (r < 0)
                         return r;
-                if (r > 0) {
-                        /* There's already an RR of the same RRset in
-                         * place! Let's see if the TTLs more or less
-                         * match. We don't really care if they match
-                         * precisely, but we do care whether one is 0
-                         * and the other is not. See RFC 2181, Section
-                         * 5.2. */
-
-                        if ((rr->ttl == 0) != (a->items[i].rr->ttl == 0))
-                                return -EINVAL;
+                if (r == 0)
+                        continue;
+
+                /* Entry already exists, keep the entry with the higher RR. */
+                if (rr->ttl > a->items[i].rr->ttl) {
+                        dns_resource_record_ref(rr);
+                        dns_resource_record_unref(a->items[i].rr);
+                        a->items[i].rr = rr;
                 }
+
+                a->items[i].flags |= flags;
+                return 0;
         }
 
         return dns_answer_add_raw(a, rr, ifindex, flags);
diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c
index 572271be95..278546da84 100644
--- a/src/resolve/resolved-dns-packet.c
+++ b/src/resolve/resolved-dns-packet.c
@@ -7,6 +7,7 @@
 #include "alloc-util.h"
 #include "dns-domain.h"
 #include "resolved-dns-packet.h"
+#include "set.h"
 #include "string-table.h"
 #include "strv.h"
 #include "unaligned.h"
@@ -2133,6 +2134,17 @@ static int dns_packet_extract_question(DnsPacket *p, DnsQuestion **ret_question)
                 if (!question)
                         return -ENOMEM;
 
+                _cleanup_set_free_ Set *keys = NULL; /* references to keys are kept by Question */
+
+                keys = set_new(&dns_resource_key_hash_ops);
+                if (!keys)
+                        return log_oom();
+
+                r = set_reserve(keys, n * 2); /* Higher multipliers give slightly higher efficiency through
+                                               * hash collisions, but the gains quickly drop of after 2. */
+                if (r < 0)
+                        return r;
+
                 for (i = 0; i < n; i++) {
                         _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
                         bool cache_flush;
@@ -2147,7 +2159,14 @@ static int dns_packet_extract_question(DnsPacket *p, DnsQuestion **ret_question)
                         if (!dns_type_is_valid_query(key->type))
                                 return -EBADMSG;
 
-                        r = dns_question_add(question, key);
+                        r = set_put(keys, key);
+                        if (r < 0)
+                                return r;
+                        if (r == 0)
+                                /* Already in the Question, let's skip */
+                                continue;
+
+                        r = dns_question_add_raw(question, key);
                         if (r < 0)
                                 return r;
                 }
@@ -2239,7 +2258,7 @@ static int dns_packet_extract_answer(DnsPacket *p, DnsAnswer **ret_answer) {
                                          * be contained in questions, never in replies. Crappy
                                          * Belkin routers copy the OPT data for example, hence let's
                                          * detect this so that we downgrade early. */
-                                        log_debug("OPT RR contained RFC6975 data, ignoring.");
+                                        log_debug("OPT RR contains RFC6975 data, ignoring.");
                                         bad_opt = true;
                                         continue;
                                 }
diff --git a/src/resolve/resolved-dns-question.c b/src/resolve/resolved-dns-question.c
index 1ed9171564..60cd34bcfc 100644
--- a/src/resolve/resolved-dns-question.c
+++ b/src/resolve/resolved-dns-question.c
@@ -32,8 +32,20 @@ static DnsQuestion *dns_question_free(DnsQuestion *q) {
 
 DEFINE_TRIVIAL_REF_UNREF_FUNC(DnsQuestion, dns_question, dns_question_free);
 
+int dns_question_add_raw(DnsQuestion *q, DnsResourceKey *key) {
+        /* Insert without checking for duplicates. */
+
+        assert(key);
+        assert(q);
+
+        if (q->n_keys >= q->n_allocated)
+                return -ENOSPC;
+
+        q->keys[q->n_keys++] = dns_resource_key_ref(key);
+        return 0;
+}
+
 int dns_question_add(DnsQuestion *q, DnsResourceKey *key) {
-        size_t i;
         int r;
 
         assert(key);
@@ -41,7 +53,7 @@ int dns_question_add(DnsQuestion *q, DnsResourceKey *key) {
         if (!q)
                 return -ENOSPC;
 
-        for (i = 0; i < q->n_keys; i++) {
+        for (size_t i = 0; i < q->n_keys; i++) {
                 r = dns_resource_key_equal(q->keys[i], key);
                 if (r < 0)
                         return r;
@@ -49,11 +61,7 @@ int dns_question_add(DnsQuestion *q, DnsResourceKey *key) {
                         return 0;
         }
 
-        if (q->n_keys >= q->n_allocated)
-                return -ENOSPC;
-
-        q->keys[q->n_keys++] = dns_resource_key_ref(key);
-        return 0;
+        return dns_question_add_raw(q, key);
 }
 
 int dns_question_matches_rr(DnsQuestion *q, DnsResourceRecord *rr, const char *search_domain) {
diff --git a/src/resolve/resolved-dns-question.h b/src/resolve/resolved-dns-question.h
index f513bf0328..0803f49b8b 100644
--- a/src/resolve/resolved-dns-question.h
+++ b/src/resolve/resolved-dns-question.h
@@ -22,6 +22,7 @@ int dns_question_new_address(DnsQuestion **ret, int family, const char *name, bo
 int dns_question_new_reverse(DnsQuestion **ret, int family, const union in_addr_union *a);
 int dns_question_new_service(DnsQuestion **ret, const char *service, const char *type, const char *domain, bool with_txt, bool convert_idna);
 
+int dns_question_add_raw(DnsQuestion *q, DnsResourceKey *key);
 int dns_question_add(DnsQuestion *q, DnsResourceKey *key);
 
 int dns_question_matches_rr(DnsQuestion *q, DnsResourceRecord *rr, const char *search_domain);
diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c
index 477d5170aa..4cbb9723e2 100644
--- a/src/resolve/resolved-dns-rr.c
+++ b/src/resolve/resolved-dns-rr.c
@@ -557,18 +557,10 @@ int dns_resource_record_new_address(DnsResourceRecord **ret, int family, const u
         ((a).field ## _size == (b).field ## _size &&  \
          memcmp((a).field, (b).field, (a).field ## _size) == 0)
 
-int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b) {
+int dns_resource_record_payload_equal(const DnsResourceRecord *a, const DnsResourceRecord *b) {
         int r;
 
-        assert(a);
-        assert(b);
-
-        if (a == b)
-                return 1;
-
-        r = dns_resource_key_equal(a->key, b->key);
-        if (r <= 0)
-                return r;
+        /* Check if a and b are the same, but don't look at their keys */
 
         if (a->unparseable != b->unparseable)
                 return 0;
@@ -692,6 +684,22 @@ int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecor
         }
 }
 
+int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b) {
+        int r;
+
+        assert(a);
+        assert(b);
+
+        if (a == b)
+                return 1;
+
+        r = dns_resource_key_equal(a->key, b->key);
+        if (r <= 0)
+                return r;
+
+        return dns_resource_record_payload_equal(a, b);
+}
+
 static char* format_location(uint32_t latitude, uint32_t longitude, uint32_t altitude,
                              uint8_t size, uint8_t horiz_pre, uint8_t vert_pre) {
         char *s;
diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h
index e02fa2d53b..3d27836651 100644
--- a/src/resolve/resolved-dns-rr.h
+++ b/src/resolve/resolved-dns-rr.h
@@ -308,6 +308,8 @@ DnsResourceRecord* dns_resource_record_unref(DnsResourceRecord *rr);
 int dns_resource_record_new_reverse(DnsResourceRecord **ret, int family, const union in_addr_union *address, const char *name);
 int dns_resource_record_new_address(DnsResourceRecord **ret, int family, const union in_addr_union *address, const char *name);
 int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b);
+int dns_resource_record_payload_equal(const DnsResourceRecord *a, const DnsResourceRecord *b);
+
 const char* dns_resource_record_to_string(DnsResourceRecord *rr);
 DnsResourceRecord *dns_resource_record_copy(DnsResourceRecord *rr);
 DEFINE_TRIVIAL_CLEANUP_FUNC(DnsResourceRecord*, dns_resource_record_unref);
diff --git a/test/fuzz/fuzz-dns-packet/oss-fuzz-13422 b/test/fuzz/fuzz-dns-packet/oss-fuzz-13422
new file mode 100644
index 0000000000..439aec54e3
--- /dev/null
+++ b/test/fuzz/fuzz-dns-packet/oss-fuzz-13422