From 61596a9aac5f0d4cef3845b04d61f2dad4aa0814 Mon Sep 17 00:00:00 2001

From: Lennart Poettering <lennart@poettering.net>

Date: Mon, 22 Feb 2016 20:39:45 +0100

Subject: [PATCH] resolved: fix notification iteration logic when transactions

 are completed

When a transaction is complete, and we notify its owners, make sure we deal

correctly with the requesters removing themselves from the list of owners while

we continue iterating.

This was previously already dealt with with transactions that require other

transactions for DNSSEC purposes, fix this for other possibly transaction

owners too now.

Since iterating through "Set" objects is not safe regarding removal of entries

from it, rework the logic to use two Sets, and move each entry we notified from

one set to the other set before we dispatch the notification. This move operation

requires no additional memory, and enables us to ensure that we don't notify

any object twice.

Fixes: #2676

(cherry picked from commit 35aa04e9edf422beac3493afa555d29575b3046c)

---

 src/basic/macro.h                      |  6 ++++

 src/basic/set.h                        |  3 ++

 src/resolve/resolved-dns-query.c       |  5 +++

 src/resolve/resolved-dns-transaction.c | 62 ++++++++++++++++------------------

 src/resolve/resolved-dns-transaction.h |  6 ++--

 src/resolve/resolved-dns-zone.c        |  5 +++

 6 files changed, 52 insertions(+), 35 deletions(-)

diff --git a/src/basic/macro.h b/src/basic/macro.h

index 2695d0edb7..ab5cc97e17 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 a378b2b7f7..2a02544eb6 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 d48fdd1281..4f5cbab702 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);

@@ -128,8 +141,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;

         assert(t);

@@ -329,39 +345,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);

@@ -1619,6 +1613,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 4617194711..fd0237d166 100644

--- a/src/resolve/resolved-dns-transaction.h

+++ b/src/resolve/resolved-dns-transaction.h

@@ -119,17 +119,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 f52383cfd1..be535cff14 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;