resolved: rework dns server lifecycle logic

Previously, there was a chance of memory corruption, because when
switching to the next DNS server we didn't care whether they linked list
of DNS servers was still valid.

Clean up lifecycle of the dns server logic:

- When a DnsServer object is still in the linked list of DnsServers for
  a link or the manager, indicate so with a "linked" boolean field, and
  never follow the linked list if that boolean is not set.

- When picking a DnsServer to use for a link ot manager, always
  explicitly take a reference.

This also rearranges some logic, to make the tracking of dns servers by
link and globally more alike.

diff --git a/src/resolve/resolved-conf.c b/src/resolve/resolved-conf.c
index 8b49d46a5968089ac7cf367e3c81b330b4f5ae2e..fe88006fb73ef001f2b43ae244bb237552fbb924 100644 (file)
--- a/src/resolve/resolved-conf.c
+++ b/src/resolve/resolved-conf.c
@@ -29,8 +29,8 @@
 
 int manager_add_dns_server_by_string(Manager *m, DnsServerType type, const char *word) {
         union in_addr_union address;
-        DnsServer *first, *s;
         int family, r;
+        DnsServer *s;
 
         assert(m);
         assert(word);
@@ -39,13 +39,8 @@ int manager_add_dns_server_by_string(Manager *m, DnsServerType type, const char
         if (r < 0)
                 return r;
 
-        first = type == DNS_SERVER_FALLBACK ? m->fallback_dns_servers : m->dns_servers;
-
         /* Filter out duplicates */
-        LIST_FOREACH(servers, s, first)
-                if (s->family == family && in_addr_equal(family, &s->address, &address))
-                        break;
-
+        s = manager_find_dns_server(m, type, family, &address);
         if (s) {
                 /*
                  * Drop the marker. This is used to find the servers

diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c
index 207ec4c6039b57b866e5a6d140475a38d559ee97..371594c710f576695b24286f4c4472c7709bbe7c 100644 (file)
--- a/src/resolve/resolved-dns-server.c
+++ b/src/resolve/resolved-dns-server.c
@@ -67,6 +67,7 @@ int dns_server_new(
                 assert_not_reached("Unknown server type");
 
         s->manager = m;
+        s->linked = true;
 
         /* A new DNS server that isn't fallback is added and the one
          * we used so far was a fallback one? Then let's try to pick
@@ -87,39 +88,61 @@ DnsServer* dns_server_ref(DnsServer *s)  {
                 return NULL;
 
         assert(s->n_ref > 0);
-
         s->n_ref ++;
 
         return s;
 }
 
-static DnsServer* dns_server_free(DnsServer *s)  {
+DnsServer* dns_server_unref(DnsServer *s)  {
         if (!s)
                 return NULL;
 
-        if (s->link && s->link->current_dns_server == s)
-                link_set_dns_server(s->link, NULL);
+        assert(s->n_ref > 0);
+        s->n_ref --;
 
-        if (s->manager && s->manager->current_dns_server == s)
-                manager_set_dns_server(s->manager, NULL);
+        if (s->n_ref > 0)
+                return NULL;
 
         free(s);
-
         return NULL;
 }
 
-DnsServer* dns_server_unref(DnsServer *s)  {
-        if (!s)
-                return NULL;
+void dns_server_unlink(DnsServer *s) {
+        assert(s);
+        assert(s->manager);
 
-        assert(s->n_ref > 0);
+        /* This removes the specified server from the linked list of
+         * servers, but any server might still stay around if it has
+         * refs, for example from an ongoing transaction. */
 
-        if (s->n_ref == 1)
-                dns_server_free(s);
-        else
-                s->n_ref --;
+        if (!s->linked)
+                return;
 
-        return NULL;
+        switch (s->type) {
+
+        case DNS_SERVER_LINK:
+                assert(s->link);
+                LIST_REMOVE(servers, s->link->dns_servers, s);
+                break;
+
+        case DNS_SERVER_SYSTEM:
+                LIST_REMOVE(servers, s->manager->dns_servers, s);
+                break;
+
+        case DNS_SERVER_FALLBACK:
+                LIST_REMOVE(servers, s->manager->fallback_dns_servers, s);
+                break;
+        }
+
+        s->linked = false;
+
+        if (s->link && s->link->current_dns_server == s)
+                link_set_dns_server(s->link, NULL);
+
+        if (s->manager->current_dns_server == s)
+                manager_set_dns_server(s->manager, NULL);
+
+        dns_server_unref(s);
 }
 
 void dns_server_packet_received(DnsServer *s, usec_t rtt) {
@@ -166,34 +189,48 @@ const struct hash_ops dns_server_hash_ops = {
         .compare = dns_server_compare_func
 };
 
-void manager_flush_dns_servers(Manager *m, DnsServerType type) {
-        DnsServer **first, *s;
+DnsServer *manager_get_first_dns_server(Manager *m, DnsServerType t) {
+        assert(m);
+
+        switch (t) {
+
+        case DNS_SERVER_SYSTEM:
+                return m->dns_servers;
+
+        case DNS_SERVER_FALLBACK:
+                return m->fallback_dns_servers;
 
+        default:
+                return NULL;
+        }
+}
+
+void manager_flush_dns_servers(Manager *m, DnsServerType type) {
         assert(m);
 
-        first = type == DNS_SERVER_FALLBACK ? &m->fallback_dns_servers : &m->dns_servers;
+        for (;;) {
+                DnsServer *first;
 
-        while (*first) {
-                s = *first;
+                first = manager_get_first_dns_server(m, type);
+                if (!first)
+                        break;
 
-                LIST_REMOVE(servers, *first, s);
-                dns_server_unref(s);
+                dns_server_unlink(first);
         }
 }
 
 void manager_flush_marked_dns_servers(Manager *m, DnsServerType type) {
-        DnsServer **first, *s, *next;
+        DnsServer *first, *s, *next;
 
         assert(m);
 
-        first = type == DNS_SERVER_FALLBACK ? &m->fallback_dns_servers : &m->dns_servers;
+        first = manager_get_first_dns_server(m, type);
 
-        LIST_FOREACH_SAFE(servers, s, next, *first) {
+        LIST_FOREACH_SAFE(servers, s, next, first) {
                 if (!s->marked)
                         continue;
 
-                LIST_REMOVE(servers, *first, s);
-                dns_server_unref(s);
+                dns_server_unlink(s);
         }
 }
 
@@ -202,23 +239,20 @@ void manager_mark_dns_servers(Manager *m, DnsServerType type) {
 
         assert(m);
 
-        first = type == DNS_SERVER_FALLBACK ? m->fallback_dns_servers : m->dns_servers;
-
+        first = manager_get_first_dns_server(m, type);
         LIST_FOREACH(servers, s, first)
                 s->marked = true;
 }
 
-DnsServer* manager_find_dns_server(Manager *m, int family, const union in_addr_union *in_addr) {
-        DnsServer *s;
+DnsServer* manager_find_dns_server(Manager *m, DnsServerType type, int family, const union in_addr_union *in_addr) {
+        DnsServer *first, *s;
 
         assert(m);
         assert(in_addr);
 
-        LIST_FOREACH(servers, s, m->dns_servers)
-                if (s->family == family && in_addr_equal(family, &s->address, in_addr) > 0)
-                        return s;
+        first = manager_get_first_dns_server(m, type);
 
-        LIST_FOREACH(servers, s, m->fallback_dns_servers)
+        LIST_FOREACH(servers, s, first)
                 if (s->family == family && in_addr_equal(family, &s->address, in_addr) > 0)
                         return s;
 
@@ -238,7 +272,8 @@ DnsServer *manager_set_dns_server(Manager *m, DnsServer *s) {
                 log_info("Switching to system DNS server %s.", strna(ip));
         }
 
-        m->current_dns_server = s;
+        dns_server_unref(m->current_dns_server);
+        m->current_dns_server = dns_server_ref(s);
 
         if (m->unicast_scope)
                 dns_cache_flush(&m->unicast_scope->cache);
@@ -286,8 +321,9 @@ void manager_next_dns_server(Manager *m) {
         if (!m->current_dns_server)
                 return;
 
-        /* Change to the next one */
-        if (m->current_dns_server->servers_next) {
+        /* Change to the next one, but make sure to follow the linked
+         * list only if the server is still linked. */
+        if (m->current_dns_server->linked && m->current_dns_server->servers_next) {
                 manager_set_dns_server(m, m->current_dns_server->servers_next);
                 return;
         }

diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h
index 49b550c1ffa555d6527b35d931bb3f4602903885..19916794ab33a0308f6685056d937675415afef9 100644 (file)
--- a/src/resolve/resolved-dns-server.h
+++ b/src/resolve/resolved-dns-server.h
@@ -51,6 +51,8 @@ struct DnsServer {
 
         bool marked:1;
 
+        /* If linked is set, then this server appears in the servers linked list */
+        bool linked:1;
         LIST_FIELDS(DnsServer, servers);
 };
 
@@ -65,18 +67,22 @@ int dns_server_new(
 DnsServer* dns_server_ref(DnsServer *s);
 DnsServer* dns_server_unref(DnsServer *s);
 
+void dns_server_unlink(DnsServer *s);
+
 void dns_server_packet_received(DnsServer *s, usec_t rtt);
 void dns_server_packet_lost(DnsServer *s, usec_t usec);
 
-DnsServer *manager_set_dns_server(Manager *m, DnsServer *s);
-DnsServer *manager_find_dns_server(Manager *m, int family, const union in_addr_union *in_addr);
-DnsServer *manager_get_dns_server(Manager *m);
-void manager_next_dns_server(Manager *m);
+DnsServer *manager_get_first_dns_server(Manager *m, DnsServerType t);
 
 void manager_flush_dns_servers(Manager *m, DnsServerType t);
 void manager_flush_marked_dns_servers(Manager *m, DnsServerType type);
 void manager_mark_dns_servers(Manager *m, DnsServerType type);
 
+DnsServer *manager_set_dns_server(Manager *m, DnsServer *s);
+DnsServer *manager_find_dns_server(Manager *m, DnsServerType t, int family, const union in_addr_union *in_addr);
+DnsServer *manager_get_dns_server(Manager *m);
+void manager_next_dns_server(Manager *m);
+
 DEFINE_TRIVIAL_CLEANUP_FUNC(DnsServer*, dns_server_unref);
 
 extern const struct hash_ops dns_server_hash_ops;

diff --git a/src/resolve/resolved-link.c b/src/resolve/resolved-link.c
index 28926410753ffae5925bd94e7731808cab5ef640..ef3db773fd463e61dfd84a77d1db92727e4df42c 100644 (file)
--- a/src/resolve/resolved-link.c
+++ b/src/resolve/resolved-link.c
@@ -65,19 +65,14 @@ Link *link_free(Link *l) {
         if (!l)
                 return NULL;
 
+        link_flush_dns_servers(l);
+
         while (l->addresses)
                 link_address_free(l->addresses);
 
         if (l->manager)
                 hashmap_remove(l->manager->links, INT_TO_PTR(l->ifindex));
 
-        while (l->dns_servers) {
-                DnsServer *s = l->dns_servers;
-
-                LIST_REMOVE(servers, l->dns_servers, s);
-                dns_server_unref(s);
-        }
-
         dns_scope_free(l->unicast_scope);
         dns_scope_free(l->llmnr_ipv4_scope);
         dns_scope_free(l->llmnr_ipv6_scope);
@@ -158,7 +153,6 @@ int link_update_rtnl(Link *l, sd_netlink_message *m) {
 static int link_update_dns_servers(Link *l) {
         _cleanup_strv_free_ char **nameservers = NULL;
         char **nameserver;
-        DnsServer *s, *nx;
         int r;
 
         assert(l);
@@ -167,11 +161,11 @@ static int link_update_dns_servers(Link *l) {
         if (r < 0)
                 goto clear;
 
-        LIST_FOREACH(servers, s, l->dns_servers)
-                s->marked = true;
+        link_mark_dns_servers(l);
 
         STRV_FOREACH(nameserver, nameservers) {
                 union in_addr_union a;
+                DnsServer *s;
                 int family;
 
                 r = in_addr_from_string_auto(*nameserver, &family, &a);
@@ -188,22 +182,11 @@ static int link_update_dns_servers(Link *l) {
                 }
         }
 
-        LIST_FOREACH_SAFE(servers, s, nx, l->dns_servers)
-                if (s->marked) {
-                        LIST_REMOVE(servers, l->dns_servers, s);
-                        dns_server_unref(s);
-                }
-
+        link_flush_marked_dns_servers(l);
         return 0;
 
 clear:
-        while (l->dns_servers) {
-                s = l->dns_servers;
-
-                LIST_REMOVE(servers, l->dns_servers, s);
-                dns_server_unref(s);
-        }
-
+        link_flush_dns_servers(l);
         return r;
 }
 
@@ -303,10 +286,40 @@ LinkAddress *link_find_address(Link *l, int family, const union in_addr_union *i
         return NULL;
 }
 
+void link_flush_dns_servers(Link *l) {
+        assert(l);
+
+        while (l->dns_servers)
+                dns_server_unlink(l->dns_servers);
+}
+
+void link_flush_marked_dns_servers(Link *l) {
+        DnsServer *s, *next;
+
+        assert(l);
+
+        LIST_FOREACH_SAFE(servers, s, next, l->dns_servers) {
+                if (!s->marked)
+                        continue;
+
+                dns_server_unlink(s);
+        }
+}
+
+void link_mark_dns_servers(Link *l) {
+        DnsServer *s;
+
+        assert(l);
+
+        LIST_FOREACH(servers, s, l->dns_servers)
+                s->marked = true;
+}
+
 DnsServer* link_find_dns_server(Link *l, int family, const union in_addr_union *in_addr) {
         DnsServer *s;
 
         assert(l);
+        assert(in_addr);
 
         LIST_FOREACH(servers, s, l->dns_servers)
                 if (s->family == family && in_addr_equal(family, &s->address, in_addr))
@@ -327,7 +340,8 @@ DnsServer* link_set_dns_server(Link *l, DnsServer *s) {
                 log_info("Switching to DNS server %s for interface %s.", strna(ip), l->name);
         }
 
-        l->current_dns_server = s;
+        dns_server_unref(l->current_dns_server);
+        l->current_dns_server = dns_server_ref(s);
 
         if (l->unicast_scope)
                 dns_cache_flush(&l->unicast_scope->cache);
@@ -350,7 +364,9 @@ void link_next_dns_server(Link *l) {
         if (!l->current_dns_server)
                 return;
 
-        if (l->current_dns_server->servers_next) {
+        /* Change to the next one, but make sure to follow the linked
+         * list only if this server is actually still linked. */
+        if (l->current_dns_server->linked && l->current_dns_server->servers_next) {
                 link_set_dns_server(l, l->current_dns_server->servers_next);
                 return;
         }

diff --git a/src/resolve/resolved-link.h b/src/resolve/resolved-link.h
index e3ab27c2497329b3dc757c73a50b6e49bb229c37..76e96e733a843d00fd19b0eb550fb0ea27f7c904 100644 (file)
--- a/src/resolve/resolved-link.h
+++ b/src/resolve/resolved-link.h
@@ -75,6 +75,10 @@ bool link_relevant(Link *l, int family);
 LinkAddress* link_find_address(Link *l, int family, const union in_addr_union *in_addr);
 void link_add_rrs(Link *l, bool force_remove);
 
+void link_flush_dns_servers(Link *l);
+void link_flush_marked_dns_servers(Link *l);
+void link_mark_dns_servers(Link *l);
+
 DnsServer* link_set_dns_server(Link *l, DnsServer *s);
 DnsServer* link_find_dns_server(Link *l, int family, const union in_addr_union *in_addr);
 DnsServer* link_get_dns_server(Link *l);

diff --git a/src/resolve/resolved-manager.c b/src/resolve/resolved-manager.c
index f991dd72fc2ae42b510a8eaf20b99a185c2a059d..5ebf1926be66c64677132a9997e2837c62b11027 100644 (file)
--- a/src/resolve/resolved-manager.c
+++ b/src/resolve/resolved-manager.c
@@ -536,15 +536,15 @@ Manager *manager_free(Manager *m) {
         if (!m)
                 return NULL;
 
+        manager_flush_dns_servers(m, DNS_SERVER_SYSTEM);
+        manager_flush_dns_servers(m, DNS_SERVER_FALLBACK);
+
         while ((l = hashmap_first(m->links)))
                link_free(l);
 
         while (m->dns_queries)
                 dns_query_free(m->dns_queries);
 
-        manager_flush_dns_servers(m, DNS_SERVER_SYSTEM);
-        manager_flush_dns_servers(m, DNS_SERVER_FALLBACK);
-
         dns_scope_free(m->unicast_scope);
 
         hashmap_free(m->links);

diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c
index 5a6e3812df9ae8e98d8cbd5944048c77288bbce6..98c1720f0f817f9edbeef83789feb40da2a273d0 100644 (file)
--- a/src/resolve/resolved-resolv-conf.c
+++ b/src/resolve/resolved-resolv-conf.c
@@ -36,7 +36,6 @@ int manager_read_resolv_conf(Manager *m) {
         _cleanup_fclose_ FILE *f = NULL;
         struct stat st, own;
         char line[LINE_MAX];
-        DnsServer *s;
         usec_t t;
         int r;
 
@@ -53,7 +52,7 @@ int manager_read_resolv_conf(Manager *m) {
                 if (errno == ENOENT)
                         r = 0;
                 else
-                        r = log_warning_errno(errno, "Failed to open /etc/resolv.conf: %m");
+                        r = log_warning_errno(errno, "Failed to stat /etc/resolv.conf: %m");
                 goto clear;
         }
 
@@ -89,7 +88,6 @@ int manager_read_resolv_conf(Manager *m) {
         manager_mark_dns_servers(m, DNS_SERVER_SYSTEM);
 
         FOREACH_LINE(line, f, r = -errno; goto clear) {
-                _cleanup_strv_free_ char **d = NULL;
                 const char *a;
                 char *l;
 
@@ -124,13 +122,7 @@ int manager_read_resolv_conf(Manager *m) {
         return 0;
 
 clear:
-        while (m->dns_servers) {
-                s = m->dns_servers;
-
-                LIST_REMOVE(servers, m->dns_servers, s);
-                dns_server_unref(s);
-        }
-
+        manager_flush_dns_servers(m, DNS_SERVER_SYSTEM);
         return r;
 }