From 35ac3b76641926f60c7f0602fc24eaeae24e4fc8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 07:52:38 +0200 Subject: [PATCH 01/10] network: introduce reference counter for Network object --- src/network/networkd-manager.c | 2 +- src/network/networkd-network.c | 14 +++++++++----- src/network/networkd-network.h | 8 +++++--- src/network/test-networkd-conf.c | 3 ++- 4 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 8c461a89e0..7d55142c41 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1458,7 +1458,7 @@ void manager_free(Manager *m) { m->duids_requesting_uuid = set_free(m->duids_requesting_uuid); while ((network = m->networks)) - network_free(network); + network_unref(network); hashmap_free(m->networks_by_name); m->netdevs = hashmap_free_with_destructor(m->netdevs, netdev_unref); diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index a85d5ede28..0ac9f7bea3 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -298,7 +298,7 @@ int network_verify(Network *network) { int network_load_one(Manager *manager, const char *filename) { _cleanup_free_ char *fname = NULL, *name = NULL; - _cleanup_(network_freep) Network *network = NULL; + _cleanup_(network_unrefp) Network *network = NULL; _cleanup_fclose_ FILE *file = NULL; const char *dropin_dirname; char *d; @@ -344,6 +344,8 @@ int network_load_one(Manager *manager, const char *filename) { .filename = TAKE_PTR(fname), .name = TAKE_PTR(name), + .n_ref = 1, + .required_for_online = true, .required_operstate_for_online = LINK_OPERSTATE_DEGRADED, .dhcp = ADDRESS_FAMILY_NO, @@ -473,7 +475,7 @@ int network_load(Manager *manager) { assert(manager); while ((network = manager->networks)) - network_free(network); + network_unref(network); r = conf_files_list_strv(&files, ".network", NULL, 0, NETWORK_DIRS); if (r < 0) @@ -488,7 +490,7 @@ int network_load(Manager *manager) { return 0; } -void network_free(Network *network) { +static Network *network_free(Network *network) { IPv6ProxyNDPAddress *ipv6_proxy_ndp_address; RoutingPolicyRule *rule; FdbEntry *fdb_entry; @@ -499,7 +501,7 @@ void network_free(Network *network) { Route *route; if (!network) - return; + return NULL; free(network->filename); @@ -586,9 +588,11 @@ void network_free(Network *network) { set_free_free(network->dnssec_negative_trust_anchors); - free(network); + return mfree(network); } +DEFINE_TRIVIAL_REF_UNREF_FUNC(Network, network, network_free); + int network_get_by_name(Manager *manager, const char *name, Network **ret) { Network *network; diff --git a/src/network/networkd-network.h b/src/network/networkd-network.h index 852144da3c..782dd491a8 100644 --- a/src/network/networkd-network.h +++ b/src/network/networkd-network.h @@ -92,6 +92,8 @@ struct Network { char *filename; char *name; + unsigned n_ref; + Set *match_mac; char **match_path; char **match_driver; @@ -278,9 +280,9 @@ struct Network { LIST_FIELDS(Network, networks); }; -void network_free(Network *network); - -DEFINE_TRIVIAL_CLEANUP_FUNC(Network*, network_free); +Network *network_ref(Network *network); +Network *network_unref(Network *network); +DEFINE_TRIVIAL_CLEANUP_FUNC(Network*, network_unref); int network_load(Manager *manager); int network_load_one(Manager *manager, const char *filename); diff --git a/src/network/test-networkd-conf.c b/src/network/test-networkd-conf.c index 05fc01d048..dfb41f801b 100644 --- a/src/network/test-networkd-conf.c +++ b/src/network/test-networkd-conf.c @@ -169,9 +169,10 @@ static void test_config_parse_hwaddr(void) { } static void test_config_parse_address_one(const char *rvalue, int family, unsigned n_addresses, const union in_addr_union *u, unsigned char prefixlen) { - _cleanup_(network_freep) Network *network = NULL; + _cleanup_(network_unrefp) Network *network = NULL; assert_se(network = new0(Network, 1)); + network->n_ref = 1; assert_se(network->filename = strdup("hogehoge.network")); assert_se(config_parse_ifnames("network", "filename", 1, "section", 1, "Name", 0, "*", &network->match_name, network) == 0); assert_se(config_parse_address("network", "filename", 1, "section", 1, "Address", 0, rvalue, network, network) == 0); From c9c908a60dab4c043be11e65962247ee2c7ac59a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 08:05:11 +0200 Subject: [PATCH 02/10] network: make Link objects take references of Network objects --- src/network/networkd-link.c | 2 ++ src/network/networkd-network.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 533193ac93..a224f2fe22 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -690,6 +690,8 @@ static Link *link_free(Link *link) { set_free_with_destructor(link->slaves, link_unref); + network_unref(link->network); + return mfree(link); } diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 0ac9f7bea3..c0ef2c6edf 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -358,7 +358,7 @@ int network_load_one(Manager *manager, const char *filename) { /* To enable/disable RFC7844 Anonymity Profiles */ .dhcp_anonymize = false, .dhcp_route_metric = DHCP_ROUTE_METRIC, - /* NOTE: this var might be overwrite by network_apply_anonymize_if_set */ + /* NOTE: this var might be overwritten by network_apply_anonymize_if_set */ .dhcp_client_identifier = DHCP_CLIENT_ID_DUID, .dhcp_route_table = RT_TABLE_MAIN, .dhcp_route_table_set = false, @@ -660,7 +660,7 @@ int network_apply(Network *network, Link *link) { assert(network); assert(link); - link->network = network; + link->network = network_ref(network); if (network->n_dns > 0 || !strv_isempty(network->ntp) || From d4df6326740e1fabb356299875d154de89589211 Mon Sep 17 00:00:00 2001 From: Susant Sahani Date: Thu, 2 May 2019 15:22:03 +0530 Subject: [PATCH 03/10] networkd: manager do not unef netlink and gennetlink early Because of this the fd is getting closed and we getting errors like ``` ^Ceno1: Could not send rtnetlink message: Bad file descriptor enp7s0f0: Could not send rtnetlink message: Bad file descriptor enp7s0f0: Cannot delete unreachable route for DHCPv6 delegated subnet 2a0a:...:fc::/62: Bad file descriptor Assertion '*_head == _item' failed at ../systemd/src/network/networkd-route.c:126, function route_free(). Aborting. Aborted ``` Closes one of https://github.com/systemd/systemd/issues/12452 --- src/network/networkd-manager.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 7d55142c41..047296cbf0 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1427,7 +1427,6 @@ int manager_new(Manager **ret) { void manager_free(Manager *m) { AddressPool *pool; - Network *network; Link *link; if (!m) @@ -1435,10 +1434,6 @@ void manager_free(Manager *m) { free(m->state_file); - sd_netlink_unref(m->rtnl); - sd_netlink_unref(m->genl); - sd_resolve_unref(m->resolve); - while ((link = hashmap_first(m->dhcp6_prefixes))) manager_dhcp6_prefix_remove_all(m, link); hashmap_free(m->dhcp6_prefixes); @@ -1457,9 +1452,7 @@ void manager_free(Manager *m) { m->links = hashmap_free_with_destructor(m->links, link_unref); m->duids_requesting_uuid = set_free(m->duids_requesting_uuid); - while ((network = m->networks)) - network_unref(network); - hashmap_free(m->networks_by_name); + m->networks_by_name = hashmap_free_with_destructor(m->networks_by_name, network_unref); m->netdevs = hashmap_free_with_destructor(m->netdevs, netdev_unref); @@ -1472,6 +1465,10 @@ void manager_free(Manager *m) { m->rules_foreign = set_free_with_destructor(m->rules_foreign, routing_policy_rule_free); set_free_with_destructor(m->rules_saved, routing_policy_rule_free); + sd_netlink_unref(m->rtnl); + sd_netlink_unref(m->genl); + sd_resolve_unref(m->resolve); + sd_event_unref(m->event); sd_device_monitor_unref(m->device_monitor); From 715d398e612b3b9965771e238d6a49207f9709de Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 13:02:18 +0200 Subject: [PATCH 04/10] network: drop list fields in Network object --- src/network/networkd-address-pool.c | 2 +- src/network/networkd-manager.c | 4 +--- src/network/networkd-manager.h | 3 +-- src/network/networkd-network-bus.c | 3 ++- src/network/networkd-network.c | 27 ++++++++++----------------- src/network/networkd-network.h | 2 -- 6 files changed, 15 insertions(+), 26 deletions(-) diff --git a/src/network/networkd-address-pool.c b/src/network/networkd-address-pool.c index eaf056d118..db6c1456dc 100644 --- a/src/network/networkd-address-pool.c +++ b/src/network/networkd-address-pool.c @@ -106,7 +106,7 @@ static bool address_pool_prefix_is_taken( } /* And don't clash with configured but un-assigned addresses either */ - LIST_FOREACH(networks, n, p->manager->networks) { + ORDERED_HASHMAP_FOREACH(n, p->manager->networks, i) { Address *a; LIST_FOREACH(addresses, a, n->static_addresses) { diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 047296cbf0..98d7259782 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1402,8 +1402,6 @@ int manager_new(Manager **ret) { if (r < 0) return r; - LIST_HEAD_INIT(m->networks); - r = sd_resolve_default(&m->resolve); if (r < 0) return r; @@ -1452,7 +1450,7 @@ void manager_free(Manager *m) { m->links = hashmap_free_with_destructor(m->links, link_unref); m->duids_requesting_uuid = set_free(m->duids_requesting_uuid); - m->networks_by_name = hashmap_free_with_destructor(m->networks_by_name, network_unref); + m->networks = ordered_hashmap_free_with_destructor(m->networks, network_unref); m->netdevs = hashmap_free_with_destructor(m->netdevs, netdev_unref); diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index 35ab6bedb1..1084a3a60b 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -37,9 +37,8 @@ struct Manager { Hashmap *links; Hashmap *netdevs; - Hashmap *networks_by_name; + OrderedHashmap *networks; Hashmap *dhcp6_prefixes; - LIST_HEAD(Network, networks); LIST_HEAD(AddressPool, address_pools); usec_t network_dirs_ts_usec; diff --git a/src/network/networkd-network-bus.c b/src/network/networkd-network-bus.c index 07e453cc80..e3ba148ce1 100644 --- a/src/network/networkd-network-bus.c +++ b/src/network/networkd-network-bus.c @@ -87,6 +87,7 @@ int network_node_enumerator(sd_bus *bus, const char *path, void *userdata, char _cleanup_strv_free_ char **l = NULL; Manager *m = userdata; Network *network; + Iterator i; int r; assert(bus); @@ -94,7 +95,7 @@ int network_node_enumerator(sd_bus *bus, const char *path, void *userdata, char assert(m); assert(nodes); - LIST_FOREACH(networks, network, m->networks) { + ORDERED_HASHMAP_FOREACH(network, m->networks, i) { char *p; p = network_bus_path(network); diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index c0ef2c6edf..0b8cf5a43b 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -344,6 +344,7 @@ int network_load_one(Manager *manager, const char *filename) { .filename = TAKE_PTR(fname), .name = TAKE_PTR(name), + .manager = manager, .n_ref = 1, .required_for_online = true, @@ -448,14 +449,11 @@ int network_load_one(Manager *manager, const char *filename) { if (r < 0) log_warning_errno(r, "%s: Failed to add IPv4LL route, ignoring: %m", network->filename); - LIST_PREPEND(networks, manager->networks, network); - network->manager = manager; - - r = hashmap_ensure_allocated(&manager->networks_by_name, &string_hash_ops); + r = ordered_hashmap_ensure_allocated(&manager->networks, &string_hash_ops); if (r < 0) return r; - r = hashmap_put(manager->networks_by_name, network->name, network); + r = ordered_hashmap_put(manager->networks, network->name, network); if (r < 0) return r; @@ -467,21 +465,19 @@ int network_load_one(Manager *manager, const char *filename) { } int network_load(Manager *manager) { - Network *network; _cleanup_strv_free_ char **files = NULL; char **f; int r; assert(manager); - while ((network = manager->networks)) - network_unref(network); + ordered_hashmap_clear_with_destructor(manager->networks, network_unref); r = conf_files_list_strv(&files, ".network", NULL, 0, NETWORK_DIRS); if (r < 0) return log_error_errno(r, "Failed to enumerate network files: %m"); - STRV_FOREACH_BACKWARDS(f, files) { + STRV_FOREACH(f, files) { r = network_load_one(manager, *f); if (r < 0) return r; @@ -570,11 +566,8 @@ static Network *network_free(Network *network) { hashmap_free(network->rules_by_section); if (network->manager) { - if (network->manager->networks) - LIST_REMOVE(networks, network->manager->networks, network); - - if (network->manager->networks_by_name && network->name) - hashmap_remove(network->manager->networks_by_name, network->name); + if (network->manager->networks && network->name) + ordered_hashmap_remove(network->manager->networks, network->name); if (network->manager->duids_requesting_uuid) set_remove(network->manager->duids_requesting_uuid, &network->duid); @@ -600,7 +593,7 @@ int network_get_by_name(Manager *manager, const char *name, Network **ret) { assert(name); assert(ret); - network = hashmap_get(manager->networks_by_name, name); + network = ordered_hashmap_get(manager->networks, name); if (!network) return -ENOENT; @@ -614,6 +607,7 @@ int network_get(Manager *manager, sd_device *device, Network **ret) { const char *path = NULL, *driver = NULL, *devtype = NULL; Network *network; + Iterator i; assert(manager); assert(ret); @@ -626,7 +620,7 @@ int network_get(Manager *manager, sd_device *device, (void) sd_device_get_devtype(device, &devtype); } - LIST_FOREACH(networks, network, manager->networks) { + ORDERED_HASHMAP_FOREACH(network, manager->networks, i) if (net_match_config(network->match_mac, network->match_path, network->match_driver, network->match_type, network->match_name, @@ -649,7 +643,6 @@ int network_get(Manager *manager, sd_device *device, *ret = network; return 0; } - } *ret = NULL; diff --git a/src/network/networkd-network.h b/src/network/networkd-network.h index 782dd491a8..dfaf43c206 100644 --- a/src/network/networkd-network.h +++ b/src/network/networkd-network.h @@ -276,8 +276,6 @@ struct Network { char **ntp; char **bind_carrier; - - LIST_FIELDS(Network, networks); }; Network *network_ref(Network *network); From f535e35417ffc789457badf605f60215c731f530 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 15:33:17 +0200 Subject: [PATCH 05/10] network: simplify link_free() --- src/network/networkd-link.c | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index a224f2fe22..0a679e4ba6 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -624,30 +624,15 @@ static int link_new(Manager *manager, sd_netlink_message *message, Link **ret) { } static Link *link_free(Link *link) { - Link *carrier; Address *address; - Route *route; - Iterator i; assert(link); - while ((route = set_first(link->routes))) - route_free(route); + link->routes = set_free_with_destructor(link->routes, route_free); + link->routes_foreign = set_free_with_destructor(link->routes_foreign, route_free); - while ((route = set_first(link->routes_foreign))) - route_free(route); - - link->routes = set_free(link->routes); - link->routes_foreign = set_free(link->routes_foreign); - - while ((address = set_first(link->addresses))) - address_free(address); - - while ((address = set_first(link->addresses_foreign))) - address_free(address); - - link->addresses = set_free(link->addresses); - link->addresses_foreign = set_free(link->addresses_foreign); + link->addresses = set_free_with_destructor(link->addresses, address_free); + link->addresses_foreign = set_free_with_destructor(link->addresses_foreign, address_free); while ((address = link->pool_addresses)) { LIST_REMOVE(addresses, link->pool_addresses, address); @@ -680,12 +665,7 @@ static Link *link_free(Link *link) { sd_device_unref(link->sd_device); - HASHMAP_FOREACH (carrier, link->bound_to_links, i) - hashmap_remove(link->bound_to_links, INT_TO_PTR(carrier->ifindex)); hashmap_free(link->bound_to_links); - - HASHMAP_FOREACH (carrier, link->bound_by_links, i) - hashmap_remove(link->bound_by_links, INT_TO_PTR(carrier->ifindex)); hashmap_free(link->bound_by_links); set_free_with_destructor(link->slaves, link_unref); From ca7c792b83f91f6b55bd16ee364a1d751b94cd5d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 15:59:36 +0200 Subject: [PATCH 06/10] network: fix memleak and double free Fixes the third issue in #12452. --- src/network/networkd-dhcp6.c | 79 ++++++++++++------------------------ 1 file changed, 27 insertions(+), 52 deletions(-) diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index ba795decfe..b921eed8d7 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -126,47 +126,33 @@ int dhcp6_lease_pd_prefix_lost(sd_dhcp6_client *client, Link* link) { &lifetime_preferred, &lifetime_valid) >= 0) { _cleanup_free_ char *buf = NULL; - _cleanup_free_ Route *route = NULL; + Route *route; - if (pd_prefix_len > 64) + if (pd_prefix_len >= 64) continue; (void) in_addr_to_string(AF_INET6, &pd_prefix, &buf); - if (pd_prefix_len < 64) { - r = route_new(&route); - if (r < 0) { - log_link_warning_errno(link, r, "Cannot create unreachable route to delete for DHCPv6 delegated subnet %s/%u: %m", - strnull(buf), - pd_prefix_len); - continue; - } - - r = route_add(link, AF_INET6, &pd_prefix, pd_prefix_len, 0, 0, 0, &route); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to add unreachable route to delete for DHCPv6 delegated subnet %s/%u: %m", - strnull(buf), - pd_prefix_len); - continue; - } - - route_update(route, NULL, 0, NULL, NULL, 0, 0, RTN_UNREACHABLE); - - r = route_remove(route, link, dhcp6_route_remove_handler); - if (r < 0) { - (void) in_addr_to_string(AF_INET6, - &pd_prefix, &buf); - - log_link_warning_errno(link, r, "Cannot delete unreachable route for DHCPv6 delegated subnet %s/%u: %m", - strnull(buf), - pd_prefix_len); - - continue; - } - - log_link_debug(link, "Removing unreachable route %s/%u", - strnull(buf), pd_prefix_len); + r = route_add(link, AF_INET6, &pd_prefix, pd_prefix_len, 0, 0, 0, &route); + if (r < 0) { + log_link_warning_errno(link, r, "Failed to add unreachable route to delete for DHCPv6 delegated subnet %s/%u: %m", + strnull(buf), + pd_prefix_len); + continue; } + + route_update(route, NULL, 0, NULL, NULL, 0, 0, RTN_UNREACHABLE); + + r = route_remove(route, link, dhcp6_route_remove_handler); + if (r < 0) { + log_link_warning_errno(link, r, "Cannot delete unreachable route for DHCPv6 delegated subnet %s/%u: %m", + strnull(buf), + pd_prefix_len); + continue; + } + + log_link_debug(link, "Removing unreachable route %s/%u", + strnull(buf), pd_prefix_len); } return 0; @@ -266,7 +252,6 @@ static int dhcp6_lease_pd_prefix_acquired(sd_dhcp6_client *client, Link *link) { union in_addr_union pd_prefix; uint8_t pd_prefix_len; uint32_t lifetime_preferred, lifetime_valid; - _cleanup_free_ char *buf = NULL; Iterator i = ITERATOR_FIRST; r = sd_dhcp6_client_get_lease(client, &lease); @@ -279,32 +264,23 @@ static int dhcp6_lease_pd_prefix_acquired(sd_dhcp6_client *client, Link *link) { &lifetime_preferred, &lifetime_valid) >= 0) { + _cleanup_free_ char *buf = NULL; + + (void) in_addr_to_string(AF_INET6, &pd_prefix, &buf); + if (pd_prefix_len > 64) { - (void) in_addr_to_string(AF_INET6, &pd_prefix, &buf); log_link_debug(link, "PD Prefix length > 64, ignoring prefix %s/%u", strnull(buf), pd_prefix_len); continue; } - if (pd_prefix_len < 48) { - (void) in_addr_to_string(AF_INET6, &pd_prefix, &buf); + if (pd_prefix_len < 48) log_link_warning(link, "PD Prefix length < 48, looks unusual %s/%u", strnull(buf), pd_prefix_len); - } if (pd_prefix_len < 64) { - _cleanup_(route_freep) Route *route = NULL; uint32_t table; - - (void) in_addr_to_string(AF_INET6, &pd_prefix, &buf); - - r = route_new(&route); - if (r < 0) { - log_link_warning_errno(link, r, "Cannot create unreachable route for DHCPv6 delegated subnet %s/%u: %m", - strnull(buf), - pd_prefix_len); - continue; - } + Route *route; table = link_get_dhcp_route_table(link); @@ -328,7 +304,6 @@ static int dhcp6_lease_pd_prefix_acquired(sd_dhcp6_client *client, Link *link) { log_link_debug(link, "Configuring unreachable route for %s/%u", strnull(buf), pd_prefix_len); - } else log_link_debug(link, "Not adding a blocking route since distributed prefix is /64"); From 2c448c8a174a4dc299c68941f742365d8fb773f0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 19:43:45 +0200 Subject: [PATCH 07/10] network: fix use-after-free The function sd_radv_add_prefix() in dhcp6_pd_prefix_assign() may return -EEXIST, and in that case the sd_radv_prefix object allocated in dhcp6_pd_prefix_assign() will be freed when the function returns. Hence, the key value in Manager::dhcp6_prefixes hashmap is lost. --- src/network/networkd-dhcp6.c | 4 ++-- src/network/networkd-link.c | 2 -- src/network/networkd-link.h | 1 + src/network/networkd-manager.c | 42 +++++++++++++++++++++++----------- 4 files changed, 32 insertions(+), 17 deletions(-) diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index b921eed8d7..7db59d3aca 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -90,7 +90,7 @@ static int dhcp6_pd_prefix_assign(Link *link, struct in6_addr *prefix, if (r < 0 && r != -EEXIST) return r; - r = manager_dhcp6_prefix_add(link->manager, &p->opt.in6_addr, link); + r = manager_dhcp6_prefix_add(link->manager, prefix, link); if (r < 0) return r; @@ -203,7 +203,7 @@ static int dhcp6_pd_prefix_distribute(Link *dhcp6_link, Iterator *i, continue; assigned_link = manager_dhcp6_prefix_get(manager, &prefix.in6); - if (assigned_link != NULL && assigned_link != link) + if (assigned_link && assigned_link != link) continue; (void) in_addr_to_string(AF_INET6, &prefix, &assigned_buf); diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 0a679e4ba6..c61ad8c52a 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -524,8 +524,6 @@ static int link_update_flags(Link *link, sd_netlink_message *m) { return 0; } -DEFINE_TRIVIAL_CLEANUP_FUNC(Link*, link_unref); - static int link_new(Manager *manager, sd_netlink_message *message, Link **ret) { _cleanup_(link_unrefp) Link *link = NULL; uint16_t type; diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index 1366a29924..930dd25f92 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -132,6 +132,7 @@ int get_product_uuid_handler(sd_bus_message *m, void *userdata, sd_bus_error *re Link *link_unref(Link *link); Link *link_ref(Link *link); +DEFINE_TRIVIAL_CLEANUP_FUNC(Link*, link_unref); DEFINE_TRIVIAL_DESTRUCTOR(link_netlink_destroy_callback, Link, link_unref); int link_get(Manager *m, int ifindex, Link **ret); diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 98d7259782..13e3d7f200 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -1279,7 +1279,9 @@ static int dhcp6_prefixes_compare_func(const struct in6_addr *a, const struct in DEFINE_PRIVATE_HASH_OPS(dhcp6_prefixes_hash_ops, struct in6_addr, dhcp6_prefixes_hash_func, dhcp6_prefixes_compare_func); int manager_dhcp6_prefix_add(Manager *m, struct in6_addr *addr, Link *link) { + _cleanup_free_ struct in6_addr *a = NULL; _cleanup_free_ char *buf = NULL; + Link *assigned_link; Route *route; int r; @@ -1298,11 +1300,27 @@ int manager_dhcp6_prefix_add(Manager *m, struct in6_addr *addr, Link *link) { (void) in_addr_to_string(AF_INET6, (union in_addr_union *) addr, &buf); log_link_debug(link, "Adding prefix route %s/64", strnull(buf)); + assigned_link = hashmap_get(m->dhcp6_prefixes, addr); + if (assigned_link) { + assert(assigned_link == link); + return 0; + } + + a = newdup(struct in6_addr, addr, 1); + if (!a) + return -ENOMEM; + r = hashmap_ensure_allocated(&m->dhcp6_prefixes, &dhcp6_prefixes_hash_ops); if (r < 0) return r; - return hashmap_put(m->dhcp6_prefixes, addr, link); + r = hashmap_put(m->dhcp6_prefixes, a, link); + if (r < 0) + return r; + + TAKE_PTR(a); + link_ref(link); + return 0; } static int dhcp6_route_remove_handler(sd_netlink *nl, sd_netlink_message *m, Link *link) { @@ -1318,21 +1336,21 @@ static int dhcp6_route_remove_handler(sd_netlink *nl, sd_netlink_message *m, Lin } static int manager_dhcp6_prefix_remove(Manager *m, struct in6_addr *addr) { + _cleanup_free_ struct in6_addr *a = NULL; + _cleanup_(link_unrefp) Link *l = NULL; _cleanup_free_ char *buf = NULL; Route *route; - Link *l; int r; assert_return(m, -EINVAL); assert_return(addr, -EINVAL); - l = hashmap_remove(m->dhcp6_prefixes, addr); + l = hashmap_remove2(m->dhcp6_prefixes, addr, (void **) &a); if (!l) return -EINVAL; (void) sd_radv_remove_prefix(l->radv, addr, 64); - r = route_get(l, AF_INET6, (union in_addr_union *) addr, 64, - 0, 0, 0, &route); + r = route_get(l, AF_INET6, (union in_addr_union *) addr, 64, 0, 0, 0, &route); if (r < 0) return r; @@ -1354,12 +1372,9 @@ int manager_dhcp6_prefix_remove_all(Manager *m, Link *link) { assert_return(m, -EINVAL); assert_return(link, -EINVAL); - HASHMAP_FOREACH_KEY(l, addr, m->dhcp6_prefixes, i) { - if (l != link) - continue; - - (void) manager_dhcp6_prefix_remove(m, addr); - } + HASHMAP_FOREACH_KEY(l, addr, m->dhcp6_prefixes, i) + if (l == link) + (void) manager_dhcp6_prefix_remove(m, addr); return 0; } @@ -1424,6 +1439,7 @@ int manager_new(Manager **ret) { } void manager_free(Manager *m) { + struct in6_addr *a; AddressPool *pool; Link *link; @@ -1432,8 +1448,8 @@ void manager_free(Manager *m) { free(m->state_file); - while ((link = hashmap_first(m->dhcp6_prefixes))) - manager_dhcp6_prefix_remove_all(m, link); + while ((a = hashmap_first_key(m->dhcp6_prefixes))) + (void) manager_dhcp6_prefix_remove(m, a); hashmap_free(m->dhcp6_prefixes); while ((link = hashmap_steal_first(m->links))) { From 62bbbedf7393d993bd3d8730d49003fae5aa5a82 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 18:08:23 +0200 Subject: [PATCH 08/10] sd-radv: fix memleak Fixes one memleak found in #12452. --- src/libsystemd-network/sd-radv.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libsystemd-network/sd-radv.c b/src/libsystemd-network/sd-radv.c index 08433adb25..185b55e1c5 100644 --- a/src/libsystemd-network/sd-radv.c +++ b/src/libsystemd-network/sd-radv.c @@ -598,6 +598,7 @@ _public_ sd_radv_prefix *sd_radv_remove_prefix(sd_radv *ra, LIST_REMOVE(prefix, ra->prefixes, cur); ra->n_prefixes--; + sd_radv_prefix_unref(cur); break; } From ce2ea782c287d4055b24b95bd02d1326b0482b3b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 20:03:44 +0200 Subject: [PATCH 09/10] network: fix conditional jump depends on uninitialised value(s) When address is in IPv4, the remaining buffer in in_addr_union may not be initialized. Fixes the following valgrind warning: ``` ==13169== Conditional jump or move depends on uninitialised value(s) ==13169== at 0x137FF6: UnknownInlinedFun (networkd-ndisc.c:77) ==13169== by 0x137FF6: UnknownInlinedFun (networkd-ndisc.c:580) ==13169== by 0x137FF6: ndisc_handler.lto_priv.83 (networkd-ndisc.c:597) ==13169== by 0x11BE23: UnknownInlinedFun (sd-ndisc.c:201) ==13169== by 0x11BE23: ndisc_recv.lto_priv.174 (sd-ndisc.c:254) ==13169== by 0x4AA18CF: source_dispatch (sd-event.c:2821) ==13169== by 0x4AA1BC2: sd_event_dispatch (sd-event.c:3234) ==13169== by 0x4AA1D88: sd_event_run (sd-event.c:3291) ==13169== by 0x4AA1FAB: sd_event_loop (sd-event.c:3313) ==13169== by 0x117401: UnknownInlinedFun (networkd.c:113) ==13169== by 0x117401: main (networkd.c:120) ==13169== Uninitialised value was created by a stack allocation ==13169== at 0x1753C8: manager_rtnl_process_address (networkd-manager.c:479) ``` --- src/network/networkd-ndisc.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index eb470a4d48..3b54652805 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -39,7 +39,7 @@ static int ndisc_netlink_message_handler(sd_netlink *rtnl, sd_netlink_message *m static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { _cleanup_(route_freep) Route *route = NULL; - struct in6_addr gateway; + union in_addr_union gateway; uint16_t lifetime; unsigned preference; uint32_t mtu; @@ -58,12 +58,14 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { if (lifetime == 0) /* not a default router */ return 0; - r = sd_ndisc_router_get_address(rt, &gateway); + r = sd_ndisc_router_get_address(rt, &gateway.in6); if (r < 0) return log_link_warning_errno(link, r, "Failed to get gateway address from RA: %m"); - SET_FOREACH(address, link->addresses, i) - if (!memcmp(&gateway, &address->in_addr.in6, sizeof(address->in_addr.in6))) { + SET_FOREACH(address, link->addresses, i) { + if (address->family != AF_INET6) + continue; + if (in_addr_equal(AF_INET6, &gateway, &address->in_addr)) { char buffer[INET6_ADDRSTRLEN]; log_link_debug(link, "No NDisc route added, gateway %s matches local address", @@ -72,9 +74,12 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { buffer, sizeof(buffer))); return 0; } + } - SET_FOREACH(address, link->addresses_foreign, i) - if (!memcmp(&gateway, &address->in_addr.in6, sizeof(address->in_addr.in6))) { + SET_FOREACH(address, link->addresses_foreign, i) { + if (address->family != AF_INET6) + continue; + if (in_addr_equal(AF_INET6, &gateway, &address->in_addr)) { char buffer[INET6_ADDRSTRLEN]; log_link_debug(link, "No NDisc route added, gateway %s matches local address", @@ -83,6 +88,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { buffer, sizeof(buffer))); return 0; } + } r = sd_ndisc_router_get_preference(rt, &preference); if (r < 0) @@ -107,7 +113,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { route->priority = link->network->dhcp_route_metric; route->protocol = RTPROT_RA; route->pref = preference; - route->gw.in6 = gateway; + route->gw = gateway; route->lifetime = time_now + lifetime * USEC_PER_SEC; route->mtu = mtu; From 67a4683364e802e5b5566ca1a144228e87024739 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 4 May 2019 20:14:08 +0200 Subject: [PATCH 10/10] network: use IN_ADDR_NULL and ETHER_ADDR_NULL The change in manager_rtnl_process_address() may not be necessary, but for safety, let's initialize the value. --- src/network/networkctl.c | 6 +++--- src/network/networkd-address.c | 2 +- src/network/networkd-manager.c | 6 +++--- src/network/test-network.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/network/networkctl.c b/src/network/networkctl.c index 3881aaa42b..eb3521b794 100644 --- a/src/network/networkctl.c +++ b/src/network/networkctl.c @@ -320,8 +320,8 @@ static int get_gateway_description( return r; for (m = reply; m; m = sd_netlink_message_next(m)) { - union in_addr_union gw = {}; - struct ether_addr mac = {}; + union in_addr_union gw = IN_ADDR_NULL; + struct ether_addr mac = ETHER_ADDR_NULL; uint16_t type; int ifi, fam; @@ -514,7 +514,7 @@ static int dump_address_labels(sd_netlink *rtnl) { for (m = reply; m; m = sd_netlink_message_next(m)) { _cleanup_free_ char *pretty = NULL; - union in_addr_union prefix = {}; + union in_addr_union prefix = IN_ADDR_NULL; uint8_t prefixlen; uint32_t label; diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 42d61cc0e5..600bad474d 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -490,7 +490,7 @@ int address_remove( } static int address_acquire(Link *link, Address *original, Address **ret) { - union in_addr_union in_addr = {}; + union in_addr_union in_addr = IN_ADDR_NULL; struct in_addr broadcast = {}; _cleanup_(address_freep) Address *na = NULL; int r; diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 13e3d7f200..a842d131a5 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -266,7 +266,7 @@ int manager_rtnl_process_route(sd_netlink *rtnl, sd_netlink_message *message, vo unsigned char protocol, scope, tos, table, rt_type; int family; unsigned char dst_prefixlen, src_prefixlen; - union in_addr_union dst = {}, gw = {}, src = {}, prefsrc = {}; + union in_addr_union dst = IN_ADDR_NULL, gw = IN_ADDR_NULL, src = IN_ADDR_NULL, prefsrc = IN_ADDR_NULL; Route *route = NULL; int r; @@ -484,7 +484,7 @@ int manager_rtnl_process_address(sd_netlink *rtnl, sd_netlink_message *message, int family; unsigned char prefixlen; unsigned char scope; - union in_addr_union in_addr; + union in_addr_union in_addr = IN_ADDR_NULL; struct ifa_cacheinfo cinfo; Address *address = NULL; char buf[INET6_ADDRSTRLEN], valid_buf[FORMAT_TIMESPAN_MAX]; @@ -728,7 +728,7 @@ static int manager_rtnl_process_link(sd_netlink *rtnl, sd_netlink_message *messa int manager_rtnl_process_rule(sd_netlink *rtnl, sd_netlink_message *message, void *userdata) { uint8_t tos = 0, to_prefixlen = 0, from_prefixlen = 0, protocol = 0; struct fib_rule_port_range sport = {}, dport = {}; - union in_addr_union to = {}, from = {}; + union in_addr_union to = IN_ADDR_NULL, from = IN_ADDR_NULL; RoutingPolicyRule *rule = NULL; uint32_t fwmark = 0, table = 0; const char *iif = NULL, *oif = NULL; diff --git a/src/network/test-network.c b/src/network/test-network.c index 6e169e0fca..21ee97e84e 100644 --- a/src/network/test-network.c +++ b/src/network/test-network.c @@ -121,7 +121,7 @@ static int test_load_config(Manager *manager) { static void test_network_get(Manager *manager, sd_device *loopback) { Network *network; - const struct ether_addr mac = {}; + const struct ether_addr mac = ETHER_ADDR_NULL; /* let's assume that the test machine does not have a .network file that applies to the loopback device... */