Merge pull request #2702 from poettering/resolved-iterate-fix

resolved iteration fix

7 files changed, 59 insertions, 42 deletions

diff --git a/src/basic/hashmap.c b/src/basic/hashmap.c
index 6f1a049d47..85b8d812b3 100644
--- a/src/basic/hashmap.c
+++ b/src/basic/hashmap.c
@@ -176,7 +176,7 @@ enum HashmapType {
 };
 
 struct _packed_ indirect_storage {
-        char    *storage;                  /* where buckets and DIBs are stored */
+        void *storage;                     /* where buckets and DIBs are stored */
         uint8_t  hash_key[HASH_KEY_SIZE];  /* hash key; changes during resize */
 
         unsigned n_entries;                /* number of stored entries */
@@ -193,7 +193,7 @@ struct direct_storage {
         /* This gives us 39 bytes on 64bit, or 35 bytes on 32bit.
          * That's room for 4 set_entries + 4 DIB bytes + 3 unused bytes on 64bit,
          *              or 7 set_entries + 7 DIB bytes + 0 unused bytes on 32bit. */
-        char storage[sizeof(struct indirect_storage)];
+        uint8_t storage[sizeof(struct indirect_storage)];
 };
 
 #define DIRECT_BUCKETS(entry_t) \
@@ -302,7 +302,7 @@ static void n_entries_dec(HashmapBase *h) {
                 h->n_direct_entries--;
 }
 
-static char *storage_ptr(HashmapBase *h) {
+static void *storage_ptr(HashmapBase *h) {
         return h->has_indirect ? h->indirect.storage
                                : h->direct.storage;
 }
@@ -347,7 +347,7 @@ static void get_hash_key(uint8_t hash_key[HASH_KEY_SIZE], bool reuse_is_ok) {
 
 static struct hashmap_base_entry *bucket_at(HashmapBase *h, unsigned idx) {
         return (struct hashmap_base_entry*)
-                (storage_ptr(h) + idx * hashmap_type_info[h->type].entry_size);
+                ((uint8_t*) storage_ptr(h) + idx * hashmap_type_info[h->type].entry_size);
 }
 
 static struct plain_hashmap_entry *plain_bucket_at(Hashmap *h, unsigned idx) {
@@ -381,7 +381,7 @@ static struct hashmap_base_entry *bucket_at_virtual(HashmapBase *h, struct swap_
 
 static dib_raw_t *dib_raw_ptr(HashmapBase *h) {
         return (dib_raw_t*)
-                (storage_ptr(h) + hashmap_type_info[h->type].entry_size * n_buckets(h));
+                ((uint8_t*) storage_ptr(h) + hashmap_type_info[h->type].entry_size * n_buckets(h));
 }
 
 static unsigned bucket_distance(HashmapBase *h, unsigned idx, unsigned from) {
@@ -1028,7 +1028,7 @@ static int hashmap_base_put_boldly(HashmapBase *h, unsigned idx,
  */
 static int resize_buckets(HashmapBase *h, unsigned entries_add) {
         struct swap_entries swap;
-        char *new_storage;
+        void *new_storage;
         dib_raw_t *old_dibs, *new_dibs;
         const struct hashmap_type_info *hi;
         unsigned idx, optimal_idx;
@@ -1095,7 +1095,7 @@ static int resize_buckets(HashmapBase *h, unsigned entries_add) {
         h->indirect.n_buckets = (1U << new_shift) /
                                 (hi->entry_size + sizeof(dib_raw_t));
 
-        old_dibs = (dib_raw_t*)(new_storage + hi->entry_size * old_n_buckets);
+        old_dibs = (dib_raw_t*)((uint8_t*) new_storage + hi->entry_size * old_n_buckets);
         new_dibs = dib_raw_ptr(h);
 
         /*
diff --git a/src/basic/macro.h b/src/basic/macro.h
index ddf0968d1b..c34441d75d 100644
--- a/src/basic/macro.h
+++ b/src/basic/macro.h
@@ -361,6 +361,12 @@ static inline unsigned long ALIGN_POWER2(unsigned long u) {
                 _found;                         \
         })
 
+#define SWAP_TWO(x, y) do {                        \
+                typeof(x) _t = (x);                \
+                (x) = (y);                         \
+                (y) = (_t);                        \
+        } while (false)
+
 /* Define C11 thread_local attribute even on older gcc compiler
  * version */
 #ifndef thread_local
diff --git a/src/basic/set.h b/src/basic/set.h
index 2bff5062da..e0d9dd001c 100644
--- a/src/basic/set.h
+++ b/src/basic/set.h
@@ -126,6 +126,9 @@ int set_put_strdupv(Set *s, char **l);
 #define SET_FOREACH(e, s, i) \
         for ((i) = ITERATOR_FIRST; set_iterate((s), &(i), (void**)&(e)); )
 
+#define SET_FOREACH_MOVE(e, d, s)                                       \
+        for (; ({ e = set_first(s); assert_se(!e || set_move_one(d, s, e) >= 0); e; }); )
+
 DEFINE_TRIVIAL_CLEANUP_FUNC(Set*, set_free);
 DEFINE_TRIVIAL_CLEANUP_FUNC(Set*, set_free_free);
 
diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c
index a46674f6fe..706f8c14ed 100644
--- a/src/resolve/resolved-dns-query.c
+++ b/src/resolve/resolved-dns-query.c
@@ -62,6 +62,7 @@ static void dns_query_candidate_stop(DnsQueryCandidate *c) {
 
         while ((t = set_steal_first(c->transactions))) {
                 set_remove(t->notify_query_candidates, c);
+                set_remove(t->notify_query_candidates_done, c);
                 dns_transaction_gc(t);
         }
 }
@@ -139,6 +140,10 @@ static int dns_query_candidate_add_transaction(DnsQueryCandidate *c, DnsResource
         if (r < 0)
                 goto gc;
 
+        r = set_ensure_allocated(&t->notify_query_candidates_done, NULL);
+        if (r < 0)
+                goto gc;
+
         r = set_put(t->notify_query_candidates, c);
         if (r < 0)
                 goto gc;
diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index 57f9455131..a5129c201e 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -52,6 +52,7 @@ static void dns_transaction_flush_dnssec_transactions(DnsTransaction *t) {
 
         while ((z = set_steal_first(t->dnssec_transactions))) {
                 set_remove(z->notify_transactions, t);
+                set_remove(z->notify_transactions_done, t);
                 dns_transaction_gc(z);
         }
 }
@@ -100,14 +101,26 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) {
                 set_remove(c->transactions, t);
         set_free(t->notify_query_candidates);
 
+        while ((c = set_steal_first(t->notify_query_candidates_done)))
+                set_remove(c->transactions, t);
+        set_free(t->notify_query_candidates_done);
+
         while ((i = set_steal_first(t->notify_zone_items)))
                 i->probe_transaction = NULL;
         set_free(t->notify_zone_items);
 
+        while ((i = set_steal_first(t->notify_zone_items_done)))
+                i->probe_transaction = NULL;
+        set_free(t->notify_zone_items_done);
+
         while ((z = set_steal_first(t->notify_transactions)))
                 set_remove(z->dnssec_transactions, t);
         set_free(t->notify_transactions);
 
+        while ((z = set_steal_first(t->notify_transactions_done)))
+                set_remove(z->dnssec_transactions, t);
+        set_free(t->notify_transactions_done);
+
         dns_transaction_flush_dnssec_transactions(t);
         set_free(t->dnssec_transactions);
 
@@ -127,8 +140,11 @@ bool dns_transaction_gc(DnsTransaction *t) {
                 return true;
 
         if (set_isempty(t->notify_query_candidates) &&
+            set_isempty(t->notify_query_candidates_done) &&
             set_isempty(t->notify_zone_items) &&
-            set_isempty(t->notify_transactions)) {
+            set_isempty(t->notify_zone_items_done) &&
+            set_isempty(t->notify_transactions) &&
+            set_isempty(t->notify_transactions_done)) {
                 dns_transaction_free(t);
                 return false;
         }
@@ -266,6 +282,7 @@ static void dns_transaction_tentative(DnsTransaction *t, DnsPacket *p) {
         log_debug("We have the lexicographically larger IP address and thus lost in the conflict.");
 
         t->block_gc++;
+
         while ((z = set_first(t->notify_zone_items))) {
                 /* First, make sure the zone item drops the reference
                  * to us */
@@ -284,7 +301,6 @@ void dns_transaction_complete(DnsTransaction *t, DnsTransactionState state) {
         DnsQueryCandidate *c;
         DnsZoneItem *z;
         DnsTransaction *d;
-        Iterator i;
         const char *st;
         char key_str[DNS_RESOURCE_KEY_STRING_MAX];
 
@@ -333,39 +349,17 @@ void dns_transaction_complete(DnsTransaction *t, DnsTransactionState state) {
          * transaction isn't freed while we are still looking at it */
         t->block_gc++;
 
-        SET_FOREACH(c, t->notify_query_candidates, i)
+        SET_FOREACH_MOVE(c, t->notify_query_candidates_done, t->notify_query_candidates)
                 dns_query_candidate_notify(c);
-        SET_FOREACH(z, t->notify_zone_items, i)
-                dns_zone_item_notify(z);
+        SWAP_TWO(t->notify_query_candidates, t->notify_query_candidates_done);
 
-        if (!set_isempty(t->notify_transactions)) {
-                DnsTransaction **nt;
-                unsigned j, n = 0;
-
-                /* We need to be careful when notifying other
-                 * transactions, as that might destroy other
-                 * transactions in our list. Hence, in order to be
-                 * able to safely iterate through the list of
-                 * transactions, take a GC lock on all of them
-                 * first. Then, in a second loop, notify them, but
-                 * first unlock that specific transaction. */
-
-                nt = newa(DnsTransaction*, set_size(t->notify_transactions));
-                SET_FOREACH(d, t->notify_transactions, i) {
-                        nt[n++] = d;
-                        d->block_gc++;
-                }
-
-                assert(n == set_size(t->notify_transactions));
+        SET_FOREACH_MOVE(z, t->notify_zone_items_done, t->notify_zone_items)
+                dns_zone_item_notify(z);
+        SWAP_TWO(t->notify_zone_items, t->notify_zone_items_done);
 
-                for (j = 0; j < n; j++) {
-                        if (set_contains(t->notify_transactions, nt[j]))
-                                dns_transaction_notify(nt[j], t);
-
-                        nt[j]->block_gc--;
-                        dns_transaction_gc(nt[j]);
-                }
-        }
+        SET_FOREACH_MOVE(d, t->notify_transactions_done, t->notify_transactions)
+                dns_transaction_notify(d, t);
+        SWAP_TWO(t->notify_transactions, t->notify_transactions_done);
 
         t->block_gc--;
         dns_transaction_gc(t);
@@ -1626,6 +1620,10 @@ static int dns_transaction_add_dnssec_transaction(DnsTransaction *t, DnsResource
         if (r < 0)
                 goto gc;
 
+        r = set_ensure_allocated(&aux->notify_transactions_done, NULL);
+        if (r < 0)
+                goto gc;
+
         r = set_put(t->dnssec_transactions, aux);
         if (r < 0)
                 goto gc;
diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h
index 491c62d772..eaece91533 100644
--- a/src/resolve/resolved-dns-transaction.h
+++ b/src/resolve/resolved-dns-transaction.h
@@ -118,17 +118,17 @@ struct DnsTransaction {
         /* Query candidates this transaction is referenced by and that
          * shall be notified about this specific transaction
          * completing. */
-        Set *notify_query_candidates;
+        Set *notify_query_candidates, *notify_query_candidates_done;
 
         /* Zone items this transaction is referenced by and that shall
          * be notified about completion. */
-        Set *notify_zone_items;
+        Set *notify_zone_items, *notify_zone_items_done;
 
         /* Other transactions that this transactions is referenced by
          * and that shall be notified about completion. This is used
          * when transactions want to validate their RRsets, but need
          * another DNSKEY or DS RR to do so. */
-        Set *notify_transactions;
+        Set *notify_transactions, *notify_transactions_done;
 
         /* The opposite direction: the transactions this transaction
          * created in order to request DNSKEY or DS RRs. */
diff --git a/src/resolve/resolved-dns-zone.c b/src/resolve/resolved-dns-zone.c
index 03813da6a2..850eed8cb8 100644
--- a/src/resolve/resolved-dns-zone.c
+++ b/src/resolve/resolved-dns-zone.c
@@ -38,6 +38,7 @@ void dns_zone_item_probe_stop(DnsZoneItem *i) {
         i->probe_transaction = NULL;
 
         set_remove(t->notify_zone_items, i);
+        set_remove(t->notify_zone_items_done, i);
         dns_transaction_gc(t);
 }
 
@@ -186,6 +187,10 @@ static int dns_zone_item_probe_start(DnsZoneItem *i)  {
         if (r < 0)
                 goto gc;
 
+        r = set_ensure_allocated(&t->notify_zone_items_done, NULL);
+        if (r < 0)
+                goto gc;
+
         r = set_put(t->notify_zone_items, i);
         if (r < 0)
                 goto gc;