resolve: slightly optimize dns_answer_add()

Previously, dns_answer_add() was O(n^2).
With this change dns_packet_extract() becomes ~15 times faster for some
extremal case.

Before:
```
$ time ./fuzz-dns-packet ~/downloads/clusterfuzz-testcase-minimized-fuzz-dns-packet-5631106733047808
/home/watanabe/downloads/clusterfuzz-testcase-minimized-fuzz-dns-packet-5631106733047808... ok

real    0m15.453s
user    0m15.430s
sys     0m0.007s
```

After:
```
$ time ./fuzz-dns-packet ~/downloads/clusterfuzz-testcase-minimized-fuzz-dns-packet-5631106733047808
/home/watanabe/downloads/clusterfuzz-testcase-minimized-fuzz-dns-packet-5631106733047808... ok

real    0m0.831s
user    0m0.824s
sys     0m0.006s
```

Hopefully fixes oss-fuzz#19227.
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=19227

diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c
index 5b762a82e819624f256c7c7062087b2a296cf039..f5e50fcd844284e64ec1556a4854115ca96e1763 100644 (file)
--- a/src/resolve/resolved-dns-answer.c
+++ b/src/resolve/resolved-dns-answer.c
@@ -8,18 +8,53 @@
 #include "resolved-dns-dnssec.h"
 #include "string-util.h"
 
+static void dns_answer_item_hash_func(const DnsAnswerItem *a, struct siphash *state) {
+        assert(a);
+        assert(state);
+
+        siphash24_compress(&a->ifindex, sizeof(a->ifindex), state);
+
+        dns_resource_record_hash_func(a->rr, state);
+}
+
+static int dns_answer_item_compare_func(const DnsAnswerItem *a, const DnsAnswerItem *b) {
+        int r;
+
+        assert(a);
+        assert(b);
+
+        r = CMP(a->ifindex, b->ifindex);
+        if (r != 0)
+                return r;
+
+        return dns_resource_record_compare_func(a->rr, b->rr);
+}
+
+DEFINE_PRIVATE_HASH_OPS(dns_answer_item_hash_ops, DnsAnswerItem, dns_answer_item_hash_func, dns_answer_item_compare_func);
+
 DnsAnswer *dns_answer_new(size_t n) {
+        _cleanup_set_free_ Set *s = NULL;
         DnsAnswer *a;
 
         if (n > UINT16_MAX) /* We can only place 64K RRs in an answer at max */
                 n = UINT16_MAX;
 
+        s = set_new(&dns_answer_item_hash_ops);
+        if (!s)
+                return NULL;
+
+        /* Higher multipliers give slightly higher efficiency through hash collisions, but the gains
+         * quickly drop off after 2. */
+        if (set_reserve(s, n * 2) < 0)
+                return NULL;
+
         a = malloc0(offsetof(DnsAnswer, items) + sizeof(DnsAnswerItem) * n);
         if (!a)
                 return NULL;
 
         a->n_ref = 1;
         a->n_allocated = n;
+        a->set_items = TAKE_PTR(s);
 
         return a;
 }
@@ -30,6 +65,8 @@ static void dns_answer_flush(DnsAnswer *a) {
         if (!a)
                 return;
 
+        a->set_items = set_free(a->set_items);
+
         DNS_ANSWER_FOREACH(rr, a)
                 dns_resource_record_unref(rr);
 
@@ -46,6 +83,8 @@ static DnsAnswer *dns_answer_free(DnsAnswer *a) {
 DEFINE_TRIVIAL_REF_UNREF_FUNC(DnsAnswer, dns_answer, dns_answer_free);
 
 static int dns_answer_add_raw(DnsAnswer *a, DnsResourceRecord *rr, int ifindex, DnsAnswerFlags flags) {
+        int r;
+
         assert(rr);
 
         if (!a)
@@ -54,12 +93,21 @@ static int dns_answer_add_raw(DnsAnswer *a, DnsResourceRecord *rr, int ifindex,
         if (a->n_rrs >= a->n_allocated)
                 return -ENOSPC;
 
-        a->items[a->n_rrs++] = (DnsAnswerItem) {
-                .rr = dns_resource_record_ref(rr),
+        a->items[a->n_rrs] = (DnsAnswerItem) {
+                .rr = rr,
                 .ifindex = ifindex,
                 .flags = flags,
         };
 
+        r = set_put(a->set_items, &a->items[a->n_rrs]);
+        if (r < 0)
+                return r;
+        if (r == 0)
+                return -EEXIST;
+
+        dns_resource_record_ref(rr);
+        a->n_rrs++;
+
         return 1;
 }
 
@@ -78,8 +126,7 @@ static int dns_answer_add_raw_all(DnsAnswer *a, DnsAnswer *source) {
 }
 
 int dns_answer_add(DnsAnswer *a, DnsResourceRecord *rr, int ifindex, DnsAnswerFlags flags) {
-        size_t i;
-        int r;
+        DnsAnswerItem tmp, *exist;
 
         assert(rr);
 
@@ -88,36 +135,26 @@ int dns_answer_add(DnsAnswer *a, DnsResourceRecord *rr, int ifindex, DnsAnswerFl
         if (a->n_ref > 1)
                 return -EBUSY;
 
-        for (i = 0; i < a->n_rrs; i++) {
-                if (a->items[i].ifindex != ifindex)
-                        continue;
-
-                r = dns_resource_key_equal(a->items[i].rr->key, rr->key);
-                if (r < 0)
-                        return r;
-                if (r == 0)
-                        continue;
+        tmp = (DnsAnswerItem) {
+                .rr = rr,
+                .ifindex = ifindex,
+        };
 
+        exist = set_get(a->set_items, &tmp);
+        if (exist) {
                 /* 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))
+                if ((rr->ttl == 0) != (exist->rr->ttl == 0))
                         return -EINVAL;
 
-                r = dns_resource_record_payload_equal(a->items[i].rr, rr);
-                if (r < 0)
-                        return r;
-                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;
+                /* Entry already exists, keep the entry with the higher TTL. */
+                if (rr->ttl > exist->rr->ttl) {
+                        dns_resource_record_unref(exist->rr);
+                        exist->rr = dns_resource_record_ref(rr);
                 }
 
-                a->items[i].flags |= flags;
+                exist->flags |= flags;
                 return 0;
         }
 

diff --git a/src/resolve/resolved-dns-answer.h b/src/resolve/resolved-dns-answer.h
index fd94c516de9a7b1012617190416842d7b3c7f261..d73525cedd211b47b7975e5d2fe295b34bfbdbb6 100644 (file)
--- a/src/resolve/resolved-dns-answer.h
+++ b/src/resolve/resolved-dns-answer.h
@@ -6,6 +6,7 @@ typedef struct DnsAnswerItem DnsAnswerItem;
 
 #include "macro.h"
 #include "resolved-dns-rr.h"
+#include "set.h"
 
 /* A simple array of resource records. We keep track of the
  * originating ifindex for each RR where that makes sense, so that we
@@ -30,6 +31,7 @@ struct DnsAnswerItem {
 
 struct DnsAnswer {
         unsigned n_ref;
+        Set *set_items; /* Used by dns_answer_add() for optimization. */
         size_t n_rrs, n_allocated;
         DnsAnswerItem items[0];
 };

diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c
index c848da3ccb986a23feca4d355da5f914fe1458c1..19e075479f75ba58cfc8d337924f0098c5414b68 100644 (file)
--- a/src/resolve/resolved-dns-rr.c
+++ b/src/resolve/resolved-dns-rr.c
@@ -1479,7 +1479,7 @@ void dns_resource_record_hash_func(const DnsResourceRecord *rr, struct siphash *
         }
 }
 
-static int dns_resource_record_compare_func(const DnsResourceRecord *x, const DnsResourceRecord *y) {
+int dns_resource_record_compare_func(const DnsResourceRecord *x, const DnsResourceRecord *y) {
         int r;
 
         r = dns_resource_key_compare_func(x->key, y->key);

diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h
index 59b3a70179c5c787cbf4de1febc5eed7d4eab390..aa17d6d391b66a796f301d8776ed29d65894eb65 100644 (file)
--- a/src/resolve/resolved-dns-rr.h
+++ b/src/resolve/resolved-dns-rr.h
@@ -330,6 +330,7 @@ DnsTxtItem *dns_txt_item_copy(DnsTxtItem *i);
 int dns_txt_item_new_empty(DnsTxtItem **ret);
 
 void dns_resource_record_hash_func(const DnsResourceRecord *i, struct siphash *state);
+int dns_resource_record_compare_func(const DnsResourceRecord *x, const DnsResourceRecord *y);
 
 extern const struct hash_ops dns_resource_key_hash_ops;
 extern const struct hash_ops dns_resource_record_hash_ops;

diff --git a/test/fuzz/fuzz-dns-packet/oss-fuzz-19227 b/test/fuzz/fuzz-dns-packet/oss-fuzz-19227
new file mode 100644 (file)
index 0000000..62828e7
Binary files /dev/null and b/test/fuzz/fuzz-dns-packet/oss-fuzz-19227 differ