From f7b2ebc87adc1a4afae8c89a7ebd94e7e03374ce Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 09:10:49 +0100 Subject: [PATCH 1/7] shared: fix typecheck in NM_PTRARRAY_LEN() Previously, NM_PTRARRAY_LEN() would not work if the pointer type is an opaque type, which is common. For example: NMConnection *const*connections = ...; --- shared/nm-utils/nm-macros-internal.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index f0d7374eb4..2e3940fd76 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -271,7 +271,8 @@ NM_G_ERROR_MSG (GError *error) gsize _n = 0; \ \ if (_array) { \ - _nm_unused typeof (*(_array[0])) *_array_check = _array[0]; \ + _nm_unused gconstpointer _type_check_is_pointer = _array[0]; \ + \ while (_array[_n]) \ _n++; \ } \ From af97b9a41e422524e69adf751caadef53104c07b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 08:12:10 +0100 Subject: [PATCH 2/7] device: minor cleanup of nm_device_complete_connection() and add code comment Regarding the cleanup: remove the success variable and instead error out early. --- src/devices/adsl/nm-device-adsl.c | 2 -- src/devices/nm-device.c | 35 ++++++++++++++++++++----------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/devices/adsl/nm-device-adsl.c b/src/devices/adsl/nm-device-adsl.c index 8675cbf8bc..ed7b868d24 100644 --- a/src/devices/adsl/nm-device-adsl.c +++ b/src/devices/adsl/nm-device-adsl.c @@ -135,8 +135,6 @@ complete_connection (NMDevice *device, _("ADSL connection"), NULL, FALSE); /* No IPv6 yet by default */ - - return TRUE; } diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 6146304e7f..feb2a1b2b0 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -4767,6 +4767,16 @@ nm_device_generate_connection (NMDevice *self, return g_steal_pointer (&connection); } +/** + * nm_device_complete_connection: + * + * Complete the connection. This is solely used for AddAndActivate where the user + * may pass in an incomplete connection and a device, and the device tries to + * make sense of it and complete it for activation. Otherwise, this is not + * used. + * + * Returns: success or failure. + */ gboolean nm_device_complete_connection (NMDevice *self, NMConnection *connection, @@ -4774,27 +4784,28 @@ nm_device_complete_connection (NMDevice *self, const GSList *existing_connections, GError **error) { - gboolean success = FALSE; + NMDeviceClass *klass; - g_return_val_if_fail (self != NULL, FALSE); - g_return_val_if_fail (connection != NULL, FALSE); + g_return_val_if_fail (NM_IS_DEVICE (self), FALSE); + g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); - if (!NM_DEVICE_GET_CLASS (self)->complete_connection) { + klass = NM_DEVICE_GET_CLASS (self); + + if (!klass->complete_connection) { g_set_error (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_INVALID_CONNECTION, "Device class %s had no complete_connection method", G_OBJECT_TYPE_NAME (self)); return FALSE; } - success = NM_DEVICE_GET_CLASS (self)->complete_connection (self, - connection, - specific_object, - existing_connections, - error); - if (success) - success = nm_connection_verify (connection, error); + if (!klass->complete_connection (self, + connection, + specific_object, + existing_connections, + error)) + return FALSE; - return success; + return nm_connection_verify (connection, error); } gboolean From 14adbc692a8840dab0f87851fdbfaacef7bb1a36 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 08:19:47 +0100 Subject: [PATCH 3/7] ofono: fix crash during complete-connection for Ofono modem nm_modem_complete_connection() cannot just return FALSE in case the modem doesn't overwrite complete_connection(). It must set the error variable. This leads to a crash when calling AddAndActivate for Ofono type modem. It does not affect the ModemManager implementation NMModemBroadband, because that one implements the method. --- src/devices/wwan/nm-modem.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/devices/wwan/nm-modem.c b/src/devices/wwan/nm-modem.c index 85f6eb039e..7dcbec1b41 100644 --- a/src/devices/wwan/nm-modem.c +++ b/src/devices/wwan/nm-modem.c @@ -1094,9 +1094,17 @@ nm_modem_complete_connection (NMModem *self, const GSList *existing_connections, GError **error) { - if (NM_MODEM_GET_CLASS (self)->complete_connection) - return NM_MODEM_GET_CLASS (self)->complete_connection (self, connection, existing_connections, error); - return FALSE; + NMModemClass *klass; + + klass = NM_MODEM_GET_CLASS (self); + if (!klass->complete_connection) { + g_set_error (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_INVALID_CONNECTION, + "Modem class %s had no complete_connection method", + G_OBJECT_TYPE_NAME (self)); + return FALSE; + } + + return klass->complete_connection (self, connection, existing_connections, error); } /*****************************************************************************/ From 1d5a76140d5f546d60c9c4fa9df9596978ff0f7b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 08:29:02 +0100 Subject: [PATCH 4/7] core: cleanup of nm_utils_complete_generic() Also, endlessly try searching for a unused name. Why limit it arbitrarily to 10000 or 500? Just try, eventually we will find a match. --- src/NetworkManagerUtils.c | 56 +++++++++++++++++++-------------------- src/NetworkManagerUtils.h | 2 +- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 8c3f82ae41..35b077a7a1 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -67,32 +67,34 @@ nm_utils_get_shared_wifi_permission (NMConnection *connection) /*****************************************************************************/ static char * -get_new_connection_name (const GSList *existing, +get_new_connection_name (const GSList *existing_connections, const char *preferred, const char *fallback_prefix) { GSList *names = NULL; const GSList *iter; - char *cname = NULL; - int i = 0; - gboolean preferred_found = FALSE; + guint i; g_assert (fallback_prefix); - for (iter = existing; iter; iter = g_slist_next (iter)) { + for (iter = existing_connections; iter; iter = g_slist_next (iter)) { NMConnection *candidate = NM_CONNECTION (iter->data); const char *id; id = nm_connection_get_id (candidate); - g_assert (id); + nm_assert (id); + names = g_slist_append (names, (gpointer) id); - if (preferred && !preferred_found && (strcmp (preferred, id) == 0)) - preferred_found = TRUE; + if ( preferred + && nm_streq (preferred, id)) { + /* the preferred name is already taken. Forget about it. */ + preferred = NULL; + } } /* Return the preferred name if it was unique */ - if (preferred && !preferred_found) { + if (preferred) { g_slist_free (names); return g_strdup (preferred); } @@ -100,52 +102,50 @@ get_new_connection_name (const GSList *existing, /* Otherwise find the next available unique connection name using the given * connection name template. */ - while (!cname && (i++ < 10000)) { - char *temp; + for (i = 1; TRUE; i++) { gboolean found = FALSE; + char *temp; /* Translators: the first %s is a prefix for the connection id, such * as "Wired Connection" or "VPN Connection". The %d is a number * that is combined with the first argument to create a unique * connection id. */ - temp = g_strdup_printf (C_("connection id fallback", "%s %d"), + temp = g_strdup_printf (C_("connection id fallback", "%s %u"), fallback_prefix, i); for (iter = names; iter; iter = g_slist_next (iter)) { - if (!strcmp (iter->data, temp)) { + if (nm_streq (iter->data, temp)) { found = TRUE; break; } } - if (!found) - cname = temp; - else - g_free (temp); + if (!found) { + g_slist_free (names); + return temp; + } + g_free (temp); } - - g_slist_free (names); - return cname; } static char * get_new_connection_ifname (NMPlatform *platform, - const GSList *existing, + const GSList *existing_connections, const char *prefix) { - int i; + guint i; char *name; const GSList *iter; gboolean found; - for (i = 0; i < 500; i++) { + for (i = 0; TRUE; i++) { name = g_strdup_printf ("%s%d", prefix, i); if (nm_platform_link_get_by_ifname (platform, name)) goto next; - for (iter = existing, found = FALSE; iter; iter = g_slist_next (iter)) { + for (iter = existing_connections, found = FALSE; iter; iter = g_slist_next (iter)) { NMConnection *candidate = iter->data; - if (g_strcmp0 (nm_connection_get_interface_name (candidate), name) == 0) { + if (nm_streq0 (nm_connection_get_interface_name (candidate), name)) { found = TRUE; break; } @@ -250,7 +250,7 @@ void nm_utils_complete_generic (NMPlatform *platform, NMConnection *connection, const char *ctype, - const GSList *existing, + const GSList *existing_connections, const char *preferred_id, const char *fallback_id_prefix, const char *ifname_prefix, @@ -277,14 +277,14 @@ nm_utils_complete_generic (NMPlatform *platform, /* Add a connection ID if absent */ if (!nm_setting_connection_get_id (s_con)) { - id = get_new_connection_name (existing, preferred_id, fallback_id_prefix); + id = get_new_connection_name (existing_connections, preferred_id, fallback_id_prefix); g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_ID, id, NULL); g_free (id); } /* Add an interface name, if requested */ if (ifname_prefix && !nm_setting_connection_get_interface_name (s_con)) { - ifname = get_new_connection_ifname (platform, existing, ifname_prefix); + ifname = get_new_connection_ifname (platform, existing_connections, ifname_prefix); g_object_set (G_OBJECT (s_con), NM_SETTING_CONNECTION_INTERFACE_NAME, ifname, NULL); g_free (ifname); } diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index 8cd9a19334..1947a316cf 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -31,7 +31,7 @@ const char *nm_utils_get_shared_wifi_permission (NMConnection *connection); void nm_utils_complete_generic (NMPlatform *platform, NMConnection *connection, const char *ctype, - const GSList *existing, + const GSList *existing_connections, const char *preferred_id, const char *fallback_id_prefix, const char *ifname_prefix, From f063ab41e96b1b62719837a51f71ac1d44c44b10 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 08:53:32 +0100 Subject: [PATCH 5/7] core: don't sort connection list for nm_utils_complete_generic() We create the all_connections list in impl_manager_add_and_activate_connection(). With this list we either call nm_utils_complete_generic() directly, or pass it to nm_device_complete_connection(). The latter also ends up only calling nm_utils_complete_generic() with this argument. nm_utils_complete_generic() doesn't care about the order in which the connections are returned. Which also becomes apparent, because we first sorted the list, and then reverted the order with g_slist_prepend(). Do not sort the list, hence we also don't need to clone it. --- src/nm-manager.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 0b34ff44ab..c621c76e1a 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4555,12 +4555,10 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, goto error; { - gs_free NMSettingsConnection **connections = NULL; + NMSettingsConnection *const*connections; guint i, len; - connections = nm_settings_get_connections_clone (priv->settings, &len, - NULL, NULL, - nm_settings_connection_cmp_autoconnect_priority_p_with_data, NULL); + connections = nm_settings_get_connections (priv->settings, &len); all_connections = NULL; for (i = len; i > 0; ) { i--; From e17cd1d742c47122b159040ce23fbf684b21c486 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 08:57:42 +0100 Subject: [PATCH 6/7] core: avoid clone of all-connections list for nm_utils_complete_generic() NMSettings exposes a cached list of all connection. We don't need to clone it. Note that this is not save against concurrent modification, meaning, add/remove of connections in NMSettings will invalidate the list. However, it wasn't save against that previously either, because altough we cloned the container (GSList), we didn't take an additional reference to the elements. This is purely a performance optimization, we don't need to clone the list. Also, since the original list is of type "NMConnection *const*", use that type insistently, instead of dependent API requiring GSList. IMO, GSList is anyway not a very nice API for many use cases because it requires an additional slice allocation for each element. It's slower, and often less convenient to use. --- src/NetworkManagerUtils.c | 83 ++++++++++++-------------- src/NetworkManagerUtils.h | 2 +- src/devices/adsl/nm-device-adsl.c | 2 +- src/devices/bluetooth/nm-device-bt.c | 2 +- src/devices/nm-device-bond.c | 2 +- src/devices/nm-device-bridge.c | 2 +- src/devices/nm-device-dummy.c | 2 +- src/devices/nm-device-ethernet.c | 2 +- src/devices/nm-device-infiniband.c | 2 +- src/devices/nm-device-ip-tunnel.c | 2 +- src/devices/nm-device-macvlan.c | 2 +- src/devices/nm-device-tun.c | 2 +- src/devices/nm-device-vlan.c | 2 +- src/devices/nm-device-vxlan.c | 2 +- src/devices/nm-device.c | 2 +- src/devices/nm-device.h | 4 +- src/devices/team/nm-device-team.c | 2 +- src/devices/wifi/nm-device-iwd.c | 2 +- src/devices/wifi/nm-device-olpc-mesh.c | 2 +- src/devices/wifi/nm-device-wifi.c | 2 +- src/devices/wwan/nm-device-modem.c | 2 +- src/devices/wwan/nm-modem-broadband.c | 2 +- src/devices/wwan/nm-modem.c | 2 +- src/devices/wwan/nm-modem.h | 4 +- src/nm-manager.c | 19 +----- 25 files changed, 66 insertions(+), 86 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index 35b077a7a1..12d46697ae 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -67,43 +67,47 @@ nm_utils_get_shared_wifi_permission (NMConnection *connection) /*****************************************************************************/ static char * -get_new_connection_name (const GSList *existing_connections, +get_new_connection_name (NMConnection *const*existing_connections, const char *preferred, const char *fallback_prefix) { - GSList *names = NULL; - const GSList *iter; - guint i; + gs_free const char **existing_names = NULL; + guint i, existing_len = 0; g_assert (fallback_prefix); - for (iter = existing_connections; iter; iter = g_slist_next (iter)) { - NMConnection *candidate = NM_CONNECTION (iter->data); - const char *id; + if (existing_connections) { + existing_len = NM_PTRARRAY_LEN (existing_connections); + existing_names = g_new (const char *, existing_len); + for (i = 0; i < existing_len; i++) { + NMConnection *candidate; + const char *id; - id = nm_connection_get_id (candidate); - nm_assert (id); + candidate = existing_connections[i]; + nm_assert (NM_IS_CONNECTION (candidate)); - names = g_slist_append (names, (gpointer) id); + id = nm_connection_get_id (candidate); + nm_assert (id); - if ( preferred - && nm_streq (preferred, id)) { - /* the preferred name is already taken. Forget about it. */ - preferred = NULL; + existing_names[i] = id; + + if ( preferred + && nm_streq (preferred, id)) { + /* the preferred name is already taken. Forget about it. */ + preferred = NULL; + } } + nm_assert (!existing_connections[i]); } /* Return the preferred name if it was unique */ - if (preferred) { - g_slist_free (names); + if (preferred) return g_strdup (preferred); - } /* Otherwise find the next available unique connection name using the given * connection name template. */ for (i = 1; TRUE; i++) { - gboolean found = FALSE; char *temp; /* Translators: the first %s is a prefix for the connection id, such @@ -112,53 +116,44 @@ get_new_connection_name (const GSList *existing_connections, * connection id. */ temp = g_strdup_printf (C_("connection id fallback", "%s %u"), fallback_prefix, i); - for (iter = names; iter; iter = g_slist_next (iter)) { - if (nm_streq (iter->data, temp)) { - found = TRUE; - break; - } - } - if (!found) { - g_slist_free (names); + + if (nm_utils_strv_find_first ((char **) existing_names, + existing_len, + temp) < 0) return temp; - } + g_free (temp); } } static char * get_new_connection_ifname (NMPlatform *platform, - const GSList *existing_connections, + NMConnection *const*existing_connections, const char *prefix) { - guint i; - char *name; - const GSList *iter; - gboolean found; + guint i, j; for (i = 0; TRUE; i++) { + char *name; + name = g_strdup_printf ("%s%d", prefix, i); if (nm_platform_link_get_by_ifname (platform, name)) goto next; - for (iter = existing_connections, found = FALSE; iter; iter = g_slist_next (iter)) { - NMConnection *candidate = iter->data; - - if (nm_streq0 (nm_connection_get_interface_name (candidate), name)) { - found = TRUE; - break; + if (existing_connections) { + for (j = 0; existing_connections[j]; j++) { + if (nm_streq0 (nm_connection_get_interface_name (existing_connections[j]), + name)) + goto next; } } - if (!found) - return name; + return name; - next: +next: g_free (name); } - - return NULL; } const char * @@ -250,7 +245,7 @@ void nm_utils_complete_generic (NMPlatform *platform, NMConnection *connection, const char *ctype, - const GSList *existing_connections, + NMConnection *const*existing_connections, const char *preferred_id, const char *fallback_id_prefix, const char *ifname_prefix, diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index 1947a316cf..13bdb67e85 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -31,7 +31,7 @@ const char *nm_utils_get_shared_wifi_permission (NMConnection *connection); void nm_utils_complete_generic (NMPlatform *platform, NMConnection *connection, const char *ctype, - const GSList *existing_connections, + NMConnection *const*existing_connections, const char *preferred_id, const char *fallback_id_prefix, const char *ifname_prefix, diff --git a/src/devices/adsl/nm-device-adsl.c b/src/devices/adsl/nm-device-adsl.c index ed7b868d24..9133137682 100644 --- a/src/devices/adsl/nm-device-adsl.c +++ b/src/devices/adsl/nm-device-adsl.c @@ -114,7 +114,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingAdsl *s_adsl; diff --git a/src/devices/bluetooth/nm-device-bt.c b/src/devices/bluetooth/nm-device-bt.c index 0b794cbaa5..1d237d9277 100644 --- a/src/devices/bluetooth/nm-device-bt.c +++ b/src/devices/bluetooth/nm-device-bt.c @@ -215,7 +215,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMDeviceBtPrivate *priv = NM_DEVICE_BT_GET_PRIVATE ((NMDeviceBt *) device); diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 94131bf9f4..2dd9494a8b 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -76,7 +76,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingBond *s_bond; diff --git a/src/devices/nm-device-bridge.c b/src/devices/nm-device-bridge.c index 22d2d6bb24..c81a025338 100644 --- a/src/devices/nm-device-bridge.c +++ b/src/devices/nm-device-bridge.c @@ -115,7 +115,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingBridge *s_bridge; diff --git a/src/devices/nm-device-dummy.c b/src/devices/nm-device-dummy.c index 07647897ea..f8bc8e7521 100644 --- a/src/devices/nm-device-dummy.c +++ b/src/devices/nm-device-dummy.c @@ -55,7 +55,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingDummy *s_dummy; diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index 930ab2bac7..16fa431463 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -1371,7 +1371,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingWired *s_wired; diff --git a/src/devices/nm-device-infiniband.c b/src/devices/nm-device-infiniband.c index dbba257888..781bbd6921 100644 --- a/src/devices/nm-device-infiniband.c +++ b/src/devices/nm-device-infiniband.c @@ -175,7 +175,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingInfiniband *s_infiniband; diff --git a/src/devices/nm-device-ip-tunnel.c b/src/devices/nm-device-ip-tunnel.c index f0ee79ccb2..59ca9ed557 100644 --- a/src/devices/nm-device-ip-tunnel.c +++ b/src/devices/nm-device-ip-tunnel.c @@ -358,7 +358,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingIPTunnel *s_ip_tunnel; diff --git a/src/devices/nm-device-macvlan.c b/src/devices/nm-device-macvlan.c index cc4b984222..b8e748d660 100644 --- a/src/devices/nm-device-macvlan.c +++ b/src/devices/nm-device-macvlan.c @@ -334,7 +334,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingMacvlan *s_macvlan; diff --git a/src/devices/nm-device-tun.c b/src/devices/nm-device-tun.c index 3573334b3d..123455f16d 100644 --- a/src/devices/nm-device-tun.c +++ b/src/devices/nm-device-tun.c @@ -145,7 +145,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingTun *s_tun; diff --git a/src/devices/nm-device-vlan.c b/src/devices/nm-device-vlan.c index 8737f10b6a..ae6a0f3690 100644 --- a/src/devices/nm-device-vlan.c +++ b/src/devices/nm-device-vlan.c @@ -379,7 +379,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingVlan *s_vlan; diff --git a/src/devices/nm-device-vxlan.c b/src/devices/nm-device-vxlan.c index d16754f9ee..e125222374 100644 --- a/src/devices/nm-device-vxlan.c +++ b/src/devices/nm-device-vxlan.c @@ -314,7 +314,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingVxlan *s_vxlan; diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index feb2a1b2b0..6c23a77286 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -4781,7 +4781,7 @@ gboolean nm_device_complete_connection (NMDevice *self, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMDeviceClass *klass; diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index f990f8331e..4ed852bc65 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -312,7 +312,7 @@ typedef struct { gboolean (* complete_connection) (NMDevice *self, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error); NMActStageReturn (* act_stage1_prepare) (NMDevice *self, @@ -520,7 +520,7 @@ gboolean nm_device_can_auto_connect (NMDevice *self, gboolean nm_device_complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connection, + NMConnection *const*existing_connections, GError **error); gboolean nm_device_check_connection_compatible (NMDevice *device, NMConnection *connection); diff --git a/src/devices/team/nm-device-team.c b/src/devices/team/nm-device-team.c index 2f1dbde39f..f83ba22b5d 100644 --- a/src/devices/team/nm-device-team.c +++ b/src/devices/team/nm-device-team.c @@ -104,7 +104,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingTeam *s_team; diff --git a/src/devices/wifi/nm-device-iwd.c b/src/devices/wifi/nm-device-iwd.c index f3a3a87921..40980b06f0 100644 --- a/src/devices/wifi/nm-device-iwd.c +++ b/src/devices/wifi/nm-device-iwd.c @@ -644,7 +644,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMDeviceIwd *self = NM_DEVICE_IWD (device); diff --git a/src/devices/wifi/nm-device-olpc-mesh.c b/src/devices/wifi/nm-device-olpc-mesh.c index a330de038d..c3d070d98d 100644 --- a/src/devices/wifi/nm-device-olpc-mesh.c +++ b/src/devices/wifi/nm-device-olpc-mesh.c @@ -113,7 +113,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMSettingOlpcMesh *s_mesh; diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 7aae9ea6ca..7c64d594b7 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -733,7 +733,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMDeviceWifi *self = NM_DEVICE_WIFI (device); diff --git a/src/devices/wwan/nm-device-modem.c b/src/devices/wwan/nm-device-modem.c index 0820b82db0..2a3e9ebeb7 100644 --- a/src/devices/wwan/nm-device-modem.c +++ b/src/devices/wwan/nm-device-modem.c @@ -434,7 +434,7 @@ static gboolean complete_connection (NMDevice *device, NMConnection *connection, const char *specific_object, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMDeviceModemPrivate *priv = NM_DEVICE_MODEM_GET_PRIVATE ((NMDeviceModem *) device); diff --git a/src/devices/wwan/nm-modem-broadband.c b/src/devices/wwan/nm-modem-broadband.c index be0c236201..9a3744db3c 100644 --- a/src/devices/wwan/nm-modem-broadband.c +++ b/src/devices/wwan/nm-modem-broadband.c @@ -663,7 +663,7 @@ check_connection_compatible (NMModem *_self, NMConnection *connection) static gboolean complete_connection (NMModem *_self, NMConnection *connection, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMModemBroadband *self = NM_MODEM_BROADBAND (_self); diff --git a/src/devices/wwan/nm-modem.c b/src/devices/wwan/nm-modem.c index 7dcbec1b41..61b7247ec8 100644 --- a/src/devices/wwan/nm-modem.c +++ b/src/devices/wwan/nm-modem.c @@ -1091,7 +1091,7 @@ nm_modem_check_connection_compatible (NMModem *self, NMConnection *connection) gboolean nm_modem_complete_connection (NMModem *self, NMConnection *connection, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error) { NMModemClass *klass; diff --git a/src/devices/wwan/nm-modem.h b/src/devices/wwan/nm-modem.h index a95da25534..3e281c0c92 100644 --- a/src/devices/wwan/nm-modem.h +++ b/src/devices/wwan/nm-modem.h @@ -126,7 +126,7 @@ typedef struct { gboolean (*complete_connection) (NMModem *modem, NMConnection *connection, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error); NMActStageReturn (*act_stage1_prepare) (NMModem *modem, @@ -189,7 +189,7 @@ gboolean nm_modem_check_connection_compatible (NMModem *self, NMConnection *conn gboolean nm_modem_complete_connection (NMModem *self, NMConnection *connection, - const GSList *existing_connections, + NMConnection *const*existing_connections, GError **error); void nm_modem_get_route_parameters (NMModem *self, diff --git a/src/nm-manager.c b/src/nm-manager.c index c621c76e1a..7764dbbee3 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4515,7 +4515,6 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, NMManager *self = NM_MANAGER (obj); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); NMConnection *connection = NULL; - GSList *all_connections = NULL; NMActiveConnection *active = NULL; NMAuthSubject *subject = NULL; GError *error = NULL; @@ -4554,17 +4553,6 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!subject) goto error; - { - NMSettingsConnection *const*connections; - guint i, len; - - connections = nm_settings_get_connections (priv->settings, &len); - all_connections = NULL; - for (i = len; i > 0; ) { - i--; - all_connections = g_slist_prepend (all_connections, connections[i]); - } - } if (vpn) { /* Try to fill the VPN's connection setting and name at least */ if (!nm_connection_get_setting_vpn (connection)) { @@ -4578,7 +4566,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, nm_utils_complete_generic (priv->platform, connection, NM_SETTING_VPN_SETTING_NAME, - all_connections, + (NMConnection *const*) nm_settings_get_connections (priv->settings, NULL), NULL, _("VPN connection"), NULL, @@ -4588,12 +4576,10 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!nm_device_complete_connection (device, connection, specific_object_path, - all_connections, + (NMConnection *const*) nm_settings_get_connections (priv->settings, NULL), &error)) goto error; } - g_slist_free (all_connections); - all_connections = NULL; active = _new_active_connection (self, connection, @@ -4618,7 +4604,6 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, error: nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD_ACTIVATE, NULL, FALSE, NULL, subject, error->message); g_clear_object (&connection); - g_slist_free (all_connections); g_clear_object (&subject); g_clear_object (&active); From c5457fcadb78fb37b4aa4b6307b4b8163acdadcb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 14 Mar 2018 10:16:25 +0100 Subject: [PATCH 7/7] settings: invalidate pointers for debugging use of outdated connections_cached_list connections_cached_list stays only valid until we remove/add connections to NMSettings. Using the list without cloning requires to be aware of that. When clearing the list, invalidate all pointers, in the hope that a following use-after-free will blow up with an assertion. We only do this in elevated assertion mode. It's not to prevent any bugs, it's to better notice it. --- src/settings/nm-settings.c | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 7cb5a2234b..ade117de5d 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -367,6 +367,22 @@ error: g_dbus_method_invocation_take_error (invocation, error); } +static void +_clear_connections_cached_list (NMSettingsConnection ***p_connections_cached_list) +{ +#if NM_MORE_ASSERTS + /* set the pointer to a bogus value. This makes it more apparent + * if somebody has a reference to the cached list and still uses + * it. That is a bug, this code just tries to make it blow up + * more eagerly. */ + if (*p_connections_cached_list) { + NMSettingsConnection **p = *p_connections_cached_list; + memset (p, 0xdeaddead, sizeof (NMSettingsConnection *) * (NM_PTRARRAY_LEN (p) + 1)); + } +#endif + g_clear_pointer (p_connections_cached_list, g_free); +} + /** * nm_settings_get_connections: * @self: the #NMSettings @@ -865,7 +881,7 @@ connection_removed (NMSettingsConnection *connection, gpointer user_data) /* Forget about the connection internally */ g_hash_table_remove (priv->connections, (gpointer) cpath); - g_clear_pointer (&priv->connections_cached_list, g_free); + _clear_connections_cached_list (&priv->connections_cached_list); _emit_connection_removed (self, connection); @@ -998,7 +1014,7 @@ claim_connection (NMSettings *self, NMSettingsConnection *connection) g_hash_table_insert (priv->connections, (gpointer) nm_connection_get_path (NM_CONNECTION (connection)), g_object_ref (connection)); - g_clear_pointer (&priv->connections_cached_list, g_free); + _clear_connections_cached_list (&priv->connections_cached_list); nm_utils_log_connection_diff (NM_CONNECTION (connection), NULL, LOGL_DEBUG, LOGD_CORE, "new connection", "++ "); @@ -1953,7 +1969,7 @@ finalize (GObject *object) NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); g_hash_table_destroy (priv->connections); - g_clear_pointer (&priv->connections_cached_list, g_free); + _clear_connections_cached_list (&priv->connections_cached_list); g_slist_free_full (priv->unmanaged_specs, g_free); g_slist_free_full (priv->unrecognized_specs, g_free);