From 458b422468b30b4738eb245c35c150925c8399e9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 14:23:04 +0200 Subject: [PATCH 01/20] shared: add NM_CAST_STRV_*() helper macros --- shared/nm-utils/nm-macros-internal.h | 35 ++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index 2e3940fd76..63d0128739 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -440,6 +440,41 @@ NM_G_ERROR_MSG (GError *error) #define _NM_ENSURE_TYPE(type, value) (value) #endif +#if _NM_CC_SUPPORT_GENERIC +/* these macros cast (value) to + * - "const char **" (for "MC", mutable-const) + * - "const char *const*" (for "CC", const-const) + * The point is to do this cast, but only accepting pointers + * that are compatible already. + * + * The problem is, if you add a function like g_strdupv(), the input + * argument is not modified (CC), but you want to make it work also + * for "char **". C doesn't allow this form of casting (for good reasons), + * so the function makes a choice like g_strdupv(char**). That means, + * every time you want to call ith with a const argument, you need to + * explicitly cast it. + * + * These macros do the cast, but they only accept a compatible input + * type, otherwise they will fail compilation. + */ +#define NM_CAST_STRV_MC(value) \ + (_Generic ((value), \ + const char * *: (const char * *) (value), \ + char * *: (const char * *) (value), \ + void *: (const char * *) (value))) +#define NM_CAST_STRV_CC(value) \ + (_Generic ((value), \ + const char *const*: (const char *const*) (value), \ + const char * *: (const char *const*) (value), \ + char *const*: (const char *const*) (value), \ + char * *: (const char *const*) (value), \ + const void *: (const char *const*) (value), \ + void *: (const char *const*) (value))) +#else +#define NM_CAST_STRV_MC(value) ((const char * *) (value)) +#define NM_CAST_STRV_CC(value) ((const char *const*) (value)) +#endif + #if _NM_CC_SUPPORT_GENERIC #define NM_PROPAGATE_CONST(test_expr, ptr) \ (_Generic ((test_expr), \ From be9a5ab308f13977f07a33135b3792ccbd1d6e67 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 14:10:22 +0200 Subject: [PATCH 02/20] shared: add nm_utils_strv_sort() helper --- shared/nm-utils/nm-shared-utils.c | 35 +++++++++++++++++++++++++++++++ shared/nm-utils/nm-shared-utils.h | 3 +++ 2 files changed, 38 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index ca5238b42b..582ccfffcf 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -1274,3 +1274,38 @@ fail: NM_SET_OUT (out_ppid, 0); return 0; } + +/*****************************************************************************/ + +/** + * _nm_utils_strv_sort: + * @strv: pointer containing strings that will be sorted + * in-place, %NULL is allowed, unless @len indicates + * that there are more elements. + * @len: the number of elements in strv. If negative, + * strv must be a NULL terminated array and the length + * will be calculated first. If @len is a positive + * number, all first @len elements in @strv must be + * non-NULL, valid strings. + * + * Ascending sort of the array @strv inplace, using plain strcmp() string + * comparison. + */ +void +_nm_utils_strv_sort (const char **strv, gssize len) +{ + gsize l; + + l = len < 0 ? (gsize) NM_PTRARRAY_LEN (strv) : (gsize) len; + + if (l <= 1) + return; + + nm_assert (l <= (gsize) G_MAXINT); + + g_qsort_with_data (strv, + l, + sizeof (const char *), + nm_strcmp_p_with_data, + NULL); +} diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index af897171ed..9f86a15170 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -484,6 +484,9 @@ nm_utils_strv_make_deep_copied_nonnull (const char **strv) return nm_utils_strv_make_deep_copied (strv) ?: g_new0 (char *, 1); } +void _nm_utils_strv_sort (const char **strv, gssize len); +#define nm_utils_strv_sort(strv, len) _nm_utils_strv_sort (NM_CAST_STRV_MC (strv), len) + /*****************************************************************************/ #define NM_UTILS_NS_PER_SECOND ((gint64) 1000000000) From 56a3f3bba93f49611cc5ac1e34acb607143bb3a2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 17:03:55 +0200 Subject: [PATCH 03/20] shared: add nm_utils_dbus_normalize_object_path() helper --- shared/nm-utils/nm-shared-utils.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 9f86a15170..f966d293d1 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -501,6 +501,18 @@ int nm_utils_fd_read_loop_exact (int fd, void *buf, size_t nbytes, bool do_poll) /*****************************************************************************/ +static inline const char * +nm_utils_dbus_normalize_object_path (const char *path) +{ + /* D-Bus does not allow an empty object path. Hence, whenever we mean NULL / no-object + * on D-Bus, it's path is actually "/". + * + * Normalize that away, and return %NULL in that case. */ + if (path && path[0] == '/' && path[1] == '\0') + return NULL; + return path; +} + #define NM_DEFINE_GDBUS_ARG_INFO_FULL(name_, ...) \ ((GDBusArgInfo *) (&((const GDBusArgInfo) { \ .ref_count = -1, \ From 6ec4dfce6983b1f30166dbffd11d79fa3fabcbec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 11:44:58 +0200 Subject: [PATCH 04/20] libnm-core: improve documentation for autoconnect and autoconnect-slaves properties --- clients/common/settings-docs.h.in | 4 ++-- libnm-core/nm-setting-connection.c | 8 +++++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index d10a59f0cd..8279fadd26 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -136,10 +136,10 @@ #define DESCRIBE_DOC_NM_SETTING_CDMA_PASSWORD_FLAGS N_("Flags indicating how to handle the \"password\" property.") #define DESCRIBE_DOC_NM_SETTING_CDMA_USERNAME N_("The username used to authenticate with the network, if required. Many providers do not require a username, or accept any username. But if a username is required, it is specified here.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTH_RETRIES N_("The number of retries for the authentication. Zero means to try indefinitely; -1 means to use a global default. If the global default is not set, the authentication retries for 3 times before failing the connection. Currently this only applies to 802-1x authentication.") -#define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTOCONNECT N_("Whether or not the connection should be automatically connected by NetworkManager when the resources for the connection are available. TRUE to automatically activate the connection, FALSE to require manual intervention to activate the connection.") +#define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTOCONNECT N_("Whether or not the connection should be automatically connected by NetworkManager when the resources for the connection are available. TRUE to automatically activate the connection, FALSE to require manual intervention to activate the connection. Note that autoconnect is not implemented for VPN profiles. See \"secondaries\" as an alternative to automatically connect VPN profiles.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTOCONNECT_PRIORITY N_("The autoconnect priority. If the connection is set to autoconnect, connections with higher priority will be preferred. Defaults to 0. The higher number means higher priority.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTOCONNECT_RETRIES N_("The number of times a connection should be tried when autoactivating before giving up. Zero means forever, -1 means the global default (4 times if not overridden). Setting this to 1 means to try activation only once before blocking autoconnect. Note that after a timeout, NetworkManager will try to autoconnect again.") -#define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES N_("Whether or not slaves of this connection should be automatically brought up when NetworkManager activates this connection. This only has a real effect for master connections. The permitted values are: 0: leave slave connections untouched, 1: activate all the slave connections with this connection, -1: default. If -1 (default) is set, global connection.autoconnect-slaves is read to determine the real value. If it is default as well, this fallbacks to 0.") +#define DESCRIBE_DOC_NM_SETTING_CONNECTION_AUTOCONNECT_SLAVES N_("Whether or not slaves of this connection should be automatically brought up when NetworkManager activates this connection. This only has a real effect for master connections. The properties \"autoconnect\", \"autoconnect-priority\" and \"autoconnect-retries\" are unrelated to this setting. The permitted values are: 0: leave slave connections untouched, 1: activate all the slave connections with this connection, -1: default. If -1 (default) is set, global connection.autoconnect-slaves is read to determine the real value. If it is default as well, this fallbacks to 0.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_GATEWAY_PING_TIMEOUT N_("If greater than zero, delay success of IP addressing until either the timeout is reached, or an IP gateway replies to a ping.") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_ID N_("A human readable unique identifier for the connection, like \"Work Wi-Fi\" or \"T-Mobile 3G\".") #define DESCRIBE_DOC_NM_SETTING_CONNECTION_INTERFACE_NAME N_("The name of the network interface this connection is bound to. If not set, then the connection can be attached to any interface of the appropriate type (subject to restrictions imposed by other settings). For software devices this specifies the name of the created device. For connection types where interface names cannot easily be made persistent (e.g. mobile broadband or USB Ethernet), this property should not be used. Setting this property restricts the interfaces a connection can be used with, and if interface names change or are reordered the connection may be applied to the wrong interface.") diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index e02fad35a1..40153c0ff9 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -1682,6 +1682,10 @@ nm_setting_connection_class_init (NMSettingConnectionClass *setting_class) * NetworkManager when the resources for the connection are available. * %TRUE to automatically activate the connection, %FALSE to require manual * intervention to activate the connection. + * + * Note that autoconnect is not implemented for VPN profiles. See + * #NMSettingConnection:secondaries as an alternative to automatically + * connect VPN profiles. **/ /* ---ifcfg-rh--- * property: autoconnect @@ -1874,7 +1878,9 @@ nm_setting_connection_class_init (NMSettingConnectionClass *setting_class) * * Whether or not slaves of this connection should be automatically brought up * when NetworkManager activates this connection. This only has a real effect - * for master connections. + * for master connections. The properties #NMSettingConnection:autoconnect, + * #NMSettingConnection:autoconnect-priority and #NMSettingConnection:autoconnect-retries + * are unrelated to this setting. * The permitted values are: 0: leave slave connections untouched, * 1: activate all the slave connections with this connection, -1: default. * If -1 (default) is set, global connection.autoconnect-slaves is read to From 34bbcc70b8823cc1650407418f42d5f4cf4a1131 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 14:18:48 +0200 Subject: [PATCH 05/20] core: sort D-Bus paths in nm_dbus_utils_g_value_set_object_path_from_hash() --- src/nm-dbus-utils.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index 5a57206c76..d22245107c 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -163,6 +163,10 @@ nm_dbus_utils_g_value_set_object_path_from_hash (GValue *value, } nm_assert (i <= n); strv[i] = NULL; + + /* sort the names, to give a well-defined, stable order. */ + nm_utils_strv_sort (strv, i); + g_value_take_boxed (value, strv); } From 5284690f180e7671d040bb6992fb3dcf3f5aa9e0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 17:05:00 +0200 Subject: [PATCH 06/20] core: use nm_utils_dbus_normalize_object_path() to cleanup D-Bus argument --- src/nm-active-connection.c | 5 ++-- src/nm-manager.c | 29 ++++++------------------ src/supplicant/nm-supplicant-interface.c | 5 ++-- 3 files changed, 11 insertions(+), 28 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 4b8b4e853e..0673a1db26 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1345,9 +1345,8 @@ set_property (GObject *object, guint prop_id, case PROP_SPECIFIC_OBJECT: /* construct-only */ tmp = g_value_get_string (value); - /* NM uses "/" to mean NULL */ - if (g_strcmp0 (tmp, "/") != 0) - priv->specific_object = g_strdup (tmp); + tmp = nm_utils_dbus_normalize_object_path (tmp); + priv->specific_object = g_strdup (tmp); break; case PROP_DEFAULT: priv->is_default = g_value_get_boolean (value); diff --git a/src/nm-manager.c b/src/nm-manager.c index 5074ae7e1a..f5b9ac723d 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4041,9 +4041,7 @@ _new_active_connection (NMManager *self, return NULL; } - /* Normalize the specific object */ - if (specific_object && g_strcmp0 (specific_object, "/") == 0) - specific_object = NULL; + specific_object = nm_utils_dbus_normalize_object_path (specific_object); is_vpn = nm_connection_is_type (NM_CONNECTION (connection), NM_SETTING_VPN_SETTING_NAME); @@ -4219,7 +4217,7 @@ nm_manager_activate_connection (NMManager *self, * @self: the #NMManager * @context: the D-Bus context of the requestor * @connection: the partial or complete #NMConnection to be activated - * @device_path: the object path of the device to be activated, or "/" + * @device_path: the object path of the device to be activated, or NULL * @out_device: on successful reutrn, the #NMDevice to be activated with @connection * @out_vpn: on successful return, %TRUE if @connection is a VPN connection * @error: location to store an error on failure @@ -4271,16 +4269,10 @@ validate_activation_request (NMManager *self, goto error; } - /* Check whether it's a VPN or not */ if ( nm_connection_get_setting_vpn (connection) || nm_connection_is_type (connection, NM_SETTING_VPN_SETTING_NAME)) vpn = TRUE; - /* Normalize device path */ - if (device_path && g_strcmp0 (device_path, "/") == 0) - device_path = NULL; - - /* And validate it */ if (device_path) { device = nm_manager_get_device_by_path (self, device_path); if (!device) { @@ -4400,13 +4392,9 @@ impl_manager_activate_connection (NMDBusObject *obj, g_variant_get (parameters, "(&o&o&o)", &connection_path, &device_path, &specific_object_path); - /* Normalize object paths */ - if (g_strcmp0 (connection_path, "/") == 0) - connection_path = NULL; - if (g_strcmp0 (specific_object_path, "/") == 0) - specific_object_path = NULL; - if (g_strcmp0 (device_path, "/") == 0) - device_path = NULL; + connection_path = nm_utils_dbus_normalize_object_path (connection_path); + specific_object_path = nm_utils_dbus_normalize_object_path (specific_object_path); + device_path = nm_utils_dbus_normalize_object_path (device_path); /* If the connection path is given and valid, that connection is activated. * Otherwise the "best" connection for the device is chosen and activated, @@ -4617,11 +4605,8 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, g_variant_get (parameters, "(@a{sa{sv}}&o&o)", &settings, &device_path, &specific_object_path); - /* Normalize object paths */ - if (g_strcmp0 (specific_object_path, "/") == 0) - specific_object_path = NULL; - if (g_strcmp0 (device_path, "/") == 0) - device_path = NULL; + specific_object_path = nm_utils_dbus_normalize_object_path (specific_object_path); + device_path = nm_utils_dbus_normalize_object_path (device_path); /* Try to create a new connection with the given settings. * We allow empty settings for AddAndActivateConnection(). In that case, diff --git a/src/supplicant/nm-supplicant-interface.c b/src/supplicant/nm-supplicant-interface.c index 9fe1c90b1f..3511b151e4 100644 --- a/src/supplicant/nm-supplicant-interface.c +++ b/src/supplicant/nm-supplicant-interface.c @@ -1074,9 +1074,8 @@ props_changed_cb (GDBusProxy *proxy, } if (g_variant_lookup (changed_properties, "CurrentBSS", "&o", &s)) { - if (strcmp (s, "/") == 0) - s = NULL; - if (g_strcmp0 (s, priv->current_bss) != 0) { + s = nm_utils_dbus_normalize_object_path (s); + if (!nm_streq0 (s, priv->current_bss)) { g_free (priv->current_bss); priv->current_bss = g_strdup (s); _notify (self, PROP_CURRENT_BSS); From 476208d223f7bc14622bb853fd83d7732065f581 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 17:11:32 +0200 Subject: [PATCH 07/20] core: don't explicitly set D-Bus path properties to "/" NMDBusObject already gets this right, by calling nm_dbus_utils_get_property(), which calls g_dbus_gvalue_to_gvariant(), which correctly converts NULL object paths to "/". We already rely on that elsewhere. No need for this workaround. --- src/nm-act-request.c | 2 +- src/nm-active-connection.c | 10 +++++----- src/nm-dbus-utils.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/nm-act-request.c b/src/nm-act-request.c index 344084d867..e069fb3a30 100644 --- a/src/nm-act-request.c +++ b/src/nm-act-request.c @@ -514,7 +514,7 @@ get_property (GObject *object, guint prop_id, || !NM_IN_SET (nm_active_connection_get_state (active), NM_ACTIVE_CONNECTION_STATE_ACTIVATED, NM_ACTIVE_CONNECTION_STATE_DEACTIVATING)) { - g_value_set_string (value, "/"); + g_value_set_string (value, NULL); return; } diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 0673a1db26..be61c472ce 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -1225,7 +1225,7 @@ get_property (GObject *object, guint prop_id, break; case PROP_SPECIFIC_OBJECT: - g_value_set_string (value, priv->specific_object ? priv->specific_object : "/"); + g_value_set_string (value, priv->specific_object); break; case PROP_DEVICES: strv = g_new0 (char *, 2); @@ -1251,19 +1251,19 @@ get_property (GObject *object, guint prop_id, break; case PROP_IP4_CONFIG: /* The IP and DHCP config properties may be overridden by a subclass */ - g_value_set_string (value, "/"); + g_value_set_string (value, NULL); break; case PROP_DHCP4_CONFIG: - g_value_set_string (value, "/"); + g_value_set_string (value, NULL); break; case PROP_DEFAULT6: g_value_set_boolean (value, priv->is_default6); break; case PROP_IP6_CONFIG: - g_value_set_string (value, "/"); + g_value_set_string (value, NULL); break; case PROP_DHCP6_CONFIG: - g_value_set_string (value, "/"); + g_value_set_string (value, NULL); break; case PROP_VPN: g_value_set_boolean (value, priv->vpn); diff --git a/src/nm-dbus-utils.c b/src/nm-dbus-utils.c index d22245107c..8e7dd122d0 100644 --- a/src/nm-dbus-utils.c +++ b/src/nm-dbus-utils.c @@ -117,7 +117,7 @@ nm_dbus_utils_g_value_set_object_path (GValue *value, gpointer object) && (path = nm_dbus_object_get_path (object))) g_value_set_string (value, path); else - g_value_set_string (value, "/"); + g_value_set_string (value, NULL); } void From 580a11da3a5ea8b56e7714e3ebb34eb4149a456d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 17:16:28 +0200 Subject: [PATCH 08/20] core: minor cleanup of handling specific-object in NMActiveConnection - use nm_assert() for something that ~really~ always should be given. - use nm_streq0() and nm_clear_g_free(). --- src/nm-active-connection.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index be61c472ce..c6d6645d51 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -515,9 +515,9 @@ nm_active_connection_set_specific_object (NMActiveConnection *self, /* Nothing that calls this function should be using paths from D-Bus, * where NM uses "/" to mean NULL. */ - g_assert (g_strcmp0 (specific_object, "/") != 0); + nm_assert (!nm_streq0 (specific_object, "/")); - if (g_strcmp0 (priv->specific_object, specific_object) == 0) + if (nm_streq0 (priv->specific_object, specific_object)) return; g_free (priv->specific_object); @@ -1430,8 +1430,7 @@ dispose (GObject *object) auth_cancel (self); - g_free (priv->specific_object); - priv->specific_object = NULL; + nm_clear_g_free (&priv->specific_object); _set_settings_connection (self, NULL); g_clear_object (&priv->applied_connection); From 1a33ab17de3ad2d9afe409295b7fd393c013f863 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 11 Apr 2018 17:35:23 +0200 Subject: [PATCH 09/20] core: downgrade assertion to nm_assert() It can be easily verified, that these assertions should not ever fail. Disable in production builds. --- src/nm-manager.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index f5b9ac723d..a4156fa2a7 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4243,9 +4243,9 @@ validate_activation_request (NMManager *self, NMAuthSubject *subject = NULL; char *error_desc = NULL; - g_assert (connection); - g_assert (out_device); - g_assert (out_vpn); + nm_assert (NM_IS_CONNECTION (connection)); + nm_assert (out_device); + nm_assert (out_vpn); /* Validate the caller */ subject = nm_auth_subject_new_unix_process_from_context (context); From f94167d8b1f94e59fb04f711cb999f380b083314 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 09:34:40 +0200 Subject: [PATCH 10/20] core: add nm_auth_is_subject_in_acl_set_error() helper --- src/nm-auth-utils.c | 29 +++++++++++++++++++++++++---- src/nm-auth-utils.h | 8 +++++++- 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index cba118250f..b41f6efa16 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -410,8 +410,8 @@ nm_auth_is_subject_in_acl (NMConnection *connection, return TRUE; if (!nm_session_monitor_uid_to_user (uid, &user)) { - if (out_error_desc) - *out_error_desc = g_strdup_printf ("Could not determine username for uid %lu", uid); + NM_SET_OUT (out_error_desc, + g_strdup_printf ("Could not determine username for uid %lu", uid)); return FALSE; } @@ -425,10 +425,31 @@ nm_auth_is_subject_in_acl (NMConnection *connection, /* Match the username returned by the session check to a user in the ACL */ if (!nm_setting_connection_permissions_user_allowed (s_con, user)) { - if (out_error_desc) - *out_error_desc = g_strdup_printf ("uid %lu has no permission to perform this operation", uid); + NM_SET_OUT (out_error_desc, + g_strdup_printf ("uid %lu has no permission to perform this operation", uid)); return FALSE; } return TRUE; } + +gboolean +nm_auth_is_subject_in_acl_set_error (NMConnection *connection, + NMAuthSubject *subject, + GQuark err_domain, + int err_code, + GError **error) +{ + char *error_desc = NULL; + + nm_assert (!error || !*error); + + if (nm_auth_is_subject_in_acl (connection, + subject, + error ? &error_desc : NULL)) + return TRUE; + + g_set_error_literal (error, err_domain, err_code, error_desc); + g_free (error_desc); + return FALSE; +} diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index a641c49dfe..5f9823b695 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -59,12 +59,18 @@ void nm_auth_chain_add_call (NMAuthChain *chain, void nm_auth_chain_destroy (NMAuthChain *chain); +NMAuthSubject *nm_auth_chain_get_subject (NMAuthChain *self); + /* Caller must free returned error description */ gboolean nm_auth_is_subject_in_acl (NMConnection *connection, NMAuthSubject *subect, char **out_error_desc); -NMAuthSubject *nm_auth_chain_get_subject (NMAuthChain *self); +gboolean nm_auth_is_subject_in_acl_set_error (NMConnection *connection, + NMAuthSubject *subject, + GQuark err_domain, + int err_code, + GError **error); #endif /* __NETWORKMANAGER_MANAGER_AUTH_H__ */ From aa86327e45fa5062620c489de4c78270c603f1c4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 09:48:16 +0200 Subject: [PATCH 11/20] core: cleanup code by using nm_auth_is_subject_in_acl_set_error() --- src/nm-manager.c | 115 ++++++++------------------ src/settings/nm-settings-connection.c | 33 +++----- src/settings/nm-settings.c | 31 +++---- 3 files changed, 58 insertions(+), 121 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index a4156fa2a7..f569386371 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2116,7 +2116,6 @@ device_auth_request_cb (NMDevice *device, NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); GError *error = NULL; NMAuthSubject *subject = NULL; - char *error_desc = NULL; NMAuthChain *chain; /* Validate the caller */ @@ -2129,15 +2128,13 @@ device_auth_request_cb (NMDevice *device, } /* Ensure the subject has permissions for this connection */ - if (connection && !nm_auth_is_subject_in_acl (connection, - subject, - &error_desc)) { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if ( connection + && !nm_auth_is_subject_in_acl_set_error (connection, + subject, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + &error)) goto done; - } /* Validate the request */ chain = nm_auth_chain_new_subject (subject, context, device_auth_done_cb, self); @@ -3746,7 +3743,6 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError * NMConnection *existing_connection = NULL; NMActiveConnection *master_ac = NULL; NMAuthSubject *subject; - char *error_desc = NULL; g_return_val_if_fail (NM_IS_MANAGER (self), FALSE); g_return_val_if_fail (NM_IS_ACTIVE_CONNECTION (active), FALSE); @@ -3754,14 +3750,14 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError * g_assert (NM_IS_VPN_CONNECTION (active) == FALSE); - connection = nm_active_connection_get_settings_connection (active); - g_assert (connection); - - applied = nm_active_connection_get_applied_connection (active); - device = nm_active_connection_get_device (active); g_return_val_if_fail (device != NULL, FALSE); + connection = nm_active_connection_get_settings_connection (active); + nm_assert (connection); + + applied = nm_active_connection_get_applied_connection (active); + /* If the device is active and its connection is not visible to the * user that's requesting this new activation, fail, since other users * should not be allowed to implicitly deactivate private connections @@ -3769,16 +3765,13 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError * */ existing_connection = nm_device_get_applied_connection (device); subject = nm_active_connection_get_subject (active); - if (existing_connection && - !nm_auth_is_subject_in_acl (existing_connection, - subject, - &error_desc)) { - g_set_error (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Private connection already active on the device: %s", - error_desc); - g_free (error_desc); + if ( existing_connection + && !nm_auth_is_subject_in_acl_set_error (existing_connection, + subject, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + error)) { + g_prefix_error (error, "Private connection already active on the device: "); return FALSE; } @@ -4159,25 +4152,18 @@ nm_manager_activate_connection (NMManager *self, { NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); NMActiveConnection *active; - char *error_desc = NULL; GSList *iter; - g_return_val_if_fail (self != NULL, NULL); - g_return_val_if_fail (connection != NULL, NULL); - g_return_val_if_fail (error != NULL, NULL); - g_return_val_if_fail (*error == NULL, NULL); + g_return_val_if_fail (self, NULL); + g_return_val_if_fail (connection, NULL); + g_return_val_if_fail (!error || !*error, NULL); - /* Ensure the subject has permissions for this connection */ - if (!nm_auth_is_subject_in_acl (NM_CONNECTION (connection), - subject, - &error_desc)) { - g_set_error_literal (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if (!nm_auth_is_subject_in_acl_set_error (NM_CONNECTION (connection), + subject, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + error)) return NULL; - } /* Look for a active connection that's equivalent and is already pending authorization * and eventual activation. This is used to de-duplicate concurrent activations which would @@ -4241,7 +4227,6 @@ validate_activation_request (NMManager *self, NMDevice *device = NULL; gboolean vpn = FALSE; NMAuthSubject *subject = NULL; - char *error_desc = NULL; nm_assert (NM_IS_CONNECTION (connection)); nm_assert (out_device); @@ -4257,17 +4242,12 @@ validate_activation_request (NMManager *self, return NULL; } - /* Ensure the subject has permissions for this connection */ - if (!nm_auth_is_subject_in_acl (connection, - subject, - &error_desc)) { - g_set_error_literal (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if (!nm_auth_is_subject_in_acl_set_error (connection, + subject, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + error)) goto error; - } if ( nm_connection_get_setting_vpn (connection) || nm_connection_is_type (connection, NM_SETTING_VPN_SETTING_NAME)) @@ -4800,7 +4780,6 @@ impl_manager_deactivate_connection (NMDBusObject *obj, GError *error = NULL; NMAuthSubject *subject = NULL; NMAuthChain *chain; - char *error_desc = NULL; const char *active_path; g_variant_get (parameters, "(&o)", &active_path); @@ -4826,16 +4805,12 @@ impl_manager_deactivate_connection (NMDBusObject *obj, goto done; } - /* Ensure the subject has permissions for this connection */ - if (!nm_auth_is_subject_in_acl (NM_CONNECTION (connection), - subject, - &error_desc)) { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if (!nm_auth_is_subject_in_acl_set_error (NM_CONNECTION (connection), + subject, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_PERMISSION_DENIED, + &error)) goto done; - } /* Validate the user request */ chain = nm_auth_chain_new_subject (subject, invocation, deactivate_net_auth_done_cb, self); @@ -5140,10 +5115,6 @@ impl_manager_sleep (NMDBusObject *obj, NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); GError *error = NULL; gs_unref_object NMAuthSubject *subject = NULL; -#if 0 - NMAuthChain *chain; - const char *error_desc = NULL; -#endif gboolean do_sleep; g_variant_get (parameters, "(b)", &do_sleep); @@ -5172,20 +5143,6 @@ impl_manager_sleep (NMDBusObject *obj, nm_audit_log_control_op (NM_AUDIT_OP_SLEEP_CONTROL, do_sleep ? "on" : "off", TRUE, subject, NULL); g_dbus_method_invocation_return_value (invocation, NULL); return; - -#if 0 - chain = nm_auth_chain_new (invocation, sleep_auth_done_cb, self, &error_desc); - if (chain) { - priv->auth_chains = g_slist_append (priv->auth_chains, chain); - nm_auth_chain_set_data (chain, "sleep", GUINT_TO_POINTER (do_sleep), NULL); - nm_auth_chain_add_call (chain, NM_AUTH_PERMISSION_SLEEP_WAKE, TRUE); - } else { - error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - error_desc); - g_dbus_method_invocation_take_error (invocation, error); - } -#endif } static void diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 6871499eb4..a71de91531 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -1465,24 +1465,19 @@ auth_start (NMSettingsConnection *self, { NMSettingsConnectionPrivate *priv = NM_SETTINGS_CONNECTION_GET_PRIVATE (self); AuthData *auth_data; - char *error_desc = NULL; + GError *error = NULL; nm_assert (nm_dbus_object_is_exported (NM_DBUS_OBJECT (self))); nm_assert (G_IS_DBUS_METHOD_INVOCATION (invocation)); nm_assert (NM_IS_AUTH_SUBJECT (subject)); - /* Ensure the caller can view this connection */ - if (!nm_auth_is_subject_in_acl (NM_CONNECTION (self), - subject, - &error_desc)) { - gs_free_error GError *error = NULL; - - error = g_error_new_literal (NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); - + if (!nm_auth_is_subject_in_acl_set_error (NM_CONNECTION (self), + subject, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_PERMISSION_DENIED, + &error)) { callback (self, invocation, subject, error, callback_data); + g_clear_error (&error); return; } @@ -1855,7 +1850,6 @@ settings_connection_update (NMSettingsConnection *self, GError *error = NULL; UpdateInfo *info; const char *permission; - char *error_desc = NULL; /* If the connection is read-only, that has to be changed at the source of * the problem (ex a system settings plugin that can't write connections out) @@ -1892,15 +1886,12 @@ settings_connection_update (NMSettingsConnection *self, * that's sending the update request. You can't make a connection * invisible to yourself. */ - if (!nm_auth_is_subject_in_acl (tmp ? tmp : NM_CONNECTION (self), - subject, - &error_desc)) { - error = g_error_new_literal (NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if (!nm_auth_is_subject_in_acl_set_error (tmp ? tmp : NM_CONNECTION (self), + subject, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_PERMISSION_DENIED, + &error)) goto error; - } info = g_slice_new0 (UpdateInfo); info->is_update2 = is_update2; diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index e2be3d120e..3dbd0b72fb 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -309,7 +309,6 @@ impl_settings_get_connection_by_uuid (NMDBusObject *obj, NMSettingsConnection *connection = NULL; gs_unref_object NMAuthSubject *subject = NULL; GError *error = NULL; - char *error_desc = NULL; const char *uuid; g_variant_get (parameters, "(&s)", &uuid); @@ -330,15 +329,12 @@ impl_settings_get_connection_by_uuid (NMDBusObject *obj, goto error; } - if (!nm_auth_is_subject_in_acl (NM_CONNECTION (connection), - subject, - &error_desc)) { - error = g_error_new_literal (NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if (!nm_auth_is_subject_in_acl_set_error (NM_CONNECTION (connection), + subject, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_PERMISSION_DENIED, + &error)) goto error; - } g_dbus_method_invocation_return_value (invocation, g_variant_new ("(o)", @@ -1252,7 +1248,6 @@ nm_settings_add_connection_dbus (NMSettings *self, NMAuthSubject *subject = NULL; NMAuthChain *chain; GError *error = NULL, *tmp_error = NULL; - char *error_desc = NULL; const char *perm; g_return_if_fail (connection != NULL); @@ -1295,18 +1290,12 @@ nm_settings_add_connection_dbus (NMSettings *self, goto done; } - /* Ensure the caller's username exists in the connection's permissions, - * or that the permissions is empty (ie, visible by everyone). - */ - if (!nm_auth_is_subject_in_acl (connection, - subject, - &error_desc)) { - error = g_error_new_literal (NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_PERMISSION_DENIED, - error_desc); - g_free (error_desc); + if (!nm_auth_is_subject_in_acl_set_error (connection, + subject, + NM_SETTINGS_ERROR, + NM_SETTINGS_ERROR_PERMISSION_DENIED, + &error)) goto done; - } /* If the caller is the only user in the connection's permissions, then * we use the 'modify.own' permission instead of 'modify.system'. If the From bac7a2821f3a52107f5c5dc447f337fdc56e3233 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 10:34:11 +0200 Subject: [PATCH 12/20] core: cleanup NMManager's validate_activation_request() - there are only two callers of validate_activation_request(). One of them, might already lookup the device before calling the validate function. Safe to looking up again. But this is not only an optimization, more importantly, it feels odd to first lookup a device, and then later look it up again. Are we guaranteed to use the same path? Why? Just avoid that question. - re-order some error checking for missing device, so that it is clearer. - use cleanup attribute to handle return value and drop the "goto error". --- src/nm-manager.c | 75 ++++++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 35 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index f569386371..07381ab942 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4205,6 +4205,9 @@ nm_manager_activate_connection (NMManager *self, * @connection: the partial or complete #NMConnection to be activated * @device_path: the object path of the device to be activated, or NULL * @out_device: on successful reutrn, the #NMDevice to be activated with @connection + * The caller may pass in a device which shortcuts the lookup by path. + * In this case, the passed in device must have the matching @device_path + * already. * @out_vpn: on successful return, %TRUE if @connection is a VPN connection * @error: location to store an error on failure * @@ -4226,7 +4229,7 @@ validate_activation_request (NMManager *self, { NMDevice *device = NULL; gboolean vpn = FALSE; - NMAuthSubject *subject = NULL; + gs_free NMAuthSubject *subject = NULL; nm_assert (NM_IS_CONNECTION (connection)); nm_assert (out_device); @@ -4247,60 +4250,62 @@ validate_activation_request (NMManager *self, NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, error)) - goto error; + return NULL; if ( nm_connection_get_setting_vpn (connection) || nm_connection_is_type (connection, NM_SETTING_VPN_SETTING_NAME)) vpn = TRUE; - if (device_path) { + if (*out_device) { + device = *out_device; + nm_assert (NM_IS_DEVICE (device)); + nm_assert (device_path); + nm_assert (nm_streq0 (device_path, nm_dbus_object_get_path (NM_DBUS_OBJECT (device)))); + nm_assert (device == nm_manager_get_device_by_path (self, device_path)); + } else if (device_path) { device = nm_manager_get_device_by_path (self, device_path); if (!device) { g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, "Device not found"); - goto error; + return NULL; } - } else + } else { device = nm_manager_get_best_device_for_connection (self, connection, TRUE, NULL); + if ( !device + && !vpn) { + gs_free char *iface = NULL; - if (!device && !vpn) { - gs_free char *iface = NULL; + /* VPN and software-device connections don't need a device yet, + * but non-virtual connections do ... */ + if (!nm_connection_is_virtual (connection)) { + g_set_error_literal (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "No suitable device found for this connection."); + return NULL; + } - /* VPN and software-device connections don't need a device yet, - * but non-virtual connections do ... */ - if (!nm_connection_is_virtual (connection)) { - g_set_error_literal (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "No suitable device found for this connection."); - goto error; + /* Look for an existing device with the connection's interface name */ + iface = nm_manager_get_connection_iface (self, connection, NULL, error); + if (!iface) + return NULL; + + device = find_device_by_iface (self, iface, connection, NULL); + if (!device) { + g_set_error_literal (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "Failed to find a compatible device for this connection"); + return NULL; + } } - - /* Look for an existing device with the connection's interface name */ - iface = nm_manager_get_connection_iface (self, connection, NULL, error); - if (!iface) - goto error; - - device = find_device_by_iface (self, iface, connection, NULL); - } - - if ((!vpn || device_path) && !device) { - g_set_error_literal (error, - NM_MANAGER_ERROR, - NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "Failed to find a compatible device for this connection"); - goto error; } *out_device = device; *out_vpn = vpn; - return subject; - -error: - g_object_unref (subject); - return NULL; + return g_steal_pointer (&subject); } /*****************************************************************************/ From bdc622fd31b7da46e36c0416b022c9d86e34d653 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 11:15:42 +0200 Subject: [PATCH 13/20] manager/trivial: rename boolean variable "vpn" to "is_vpn" --- src/nm-manager.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 07381ab942..d71e7b4abb 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4208,7 +4208,7 @@ nm_manager_activate_connection (NMManager *self, * The caller may pass in a device which shortcuts the lookup by path. * In this case, the passed in device must have the matching @device_path * already. - * @out_vpn: on successful return, %TRUE if @connection is a VPN connection + * @out_is_vpn: on successful return, %TRUE if @connection is a VPN connection * @error: location to store an error on failure * * Performs basic validation on an activation request, including ensuring that @@ -4224,16 +4224,16 @@ validate_activation_request (NMManager *self, NMConnection *connection, const char *device_path, NMDevice **out_device, - gboolean *out_vpn, + gboolean *out_is_vpn, GError **error) { NMDevice *device = NULL; - gboolean vpn = FALSE; + gboolean is_vpn = FALSE; gs_free NMAuthSubject *subject = NULL; nm_assert (NM_IS_CONNECTION (connection)); nm_assert (out_device); - nm_assert (out_vpn); + nm_assert (out_is_vpn); /* Validate the caller */ subject = nm_auth_subject_new_unix_process_from_context (context); @@ -4254,7 +4254,7 @@ validate_activation_request (NMManager *self, if ( nm_connection_get_setting_vpn (connection) || nm_connection_is_type (connection, NM_SETTING_VPN_SETTING_NAME)) - vpn = TRUE; + is_vpn = TRUE; if (*out_device) { device = *out_device; @@ -4274,7 +4274,7 @@ validate_activation_request (NMManager *self, } else { device = nm_manager_get_best_device_for_connection (self, connection, TRUE, NULL); if ( !device - && !vpn) { + && !is_vpn) { gs_free char *iface = NULL; /* VPN and software-device connections don't need a device yet, @@ -4304,7 +4304,7 @@ validate_activation_request (NMManager *self, } *out_device = device; - *out_vpn = vpn; + *out_is_vpn = is_vpn; return g_steal_pointer (&subject); } @@ -4583,7 +4583,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, NMAuthSubject *subject = NULL; GError *error = NULL; NMDevice *device = NULL; - gboolean vpn = FALSE; + gboolean is_vpn = FALSE; gs_unref_variant GVariant *settings = NULL; const char *device_path; const char *specific_object_path; @@ -4609,12 +4609,12 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, connection, device_path, &device, - &vpn, + &is_vpn, &error); if (!subject) goto error; - if (vpn) { + if (is_vpn) { /* Try to fill the VPN's connection setting and name at least */ if (!nm_connection_get_setting_vpn (connection)) { error = g_error_new_literal (NM_CONNECTION_ERROR, From 7fcdca29b6443083703f1350bacf70199b6b6eb7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 11:19:06 +0200 Subject: [PATCH 14/20] manager: add _connection_is_vpn() helper to unify checks for VPN type --- src/nm-manager.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index d71e7b4abb..fff4fc412f 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -338,6 +338,23 @@ static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark) /*****************************************************************************/ +static gboolean +_connection_is_vpn (NMConnection *connection) +{ + const char *type; + + type = nm_connection_get_connection_type (connection); + if (type) + return nm_streq (type, NM_SETTING_VPN_SETTING_NAME); + + /* we have an incomplete (invalid) connection at hand. That can only + * happen during AddAndActivate. Determine whether it's VPN type based + * on the existance of a [vpn] section. */ + return !!nm_connection_get_setting_vpn (connection); +} + +/*****************************************************************************/ + static gboolean concheck_enabled (NMManager *self, gboolean *out_changed) { @@ -4036,7 +4053,7 @@ _new_active_connection (NMManager *self, specific_object = nm_utils_dbus_normalize_object_path (specific_object); - is_vpn = nm_connection_is_type (NM_CONNECTION (connection), NM_SETTING_VPN_SETTING_NAME); + is_vpn = _connection_is_vpn (NM_CONNECTION (connection)); if (NM_IS_SETTINGS_CONNECTION (connection)) settings_connection = (NMSettingsConnection *) connection; @@ -4252,9 +4269,7 @@ validate_activation_request (NMManager *self, error)) return NULL; - if ( nm_connection_get_setting_vpn (connection) - || nm_connection_is_type (connection, NM_SETTING_VPN_SETTING_NAME)) - is_vpn = TRUE; + is_vpn = _connection_is_vpn (connection); if (*out_device) { device = *out_device; From 3e3d53ce6960dc0195c29a32c46cecafee3a8a70 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 11:32:18 +0200 Subject: [PATCH 15/20] manager: add is-vpn argument to _new_active_connection() and avoid searching existing activations - pass is-vpn to _new_active_connection(). It is confusing that _new_active_connection() would re-determine whether a connection is a VPN type, although that was already established previously. The confusing part is: will they come to the same result? Why? What if not? Instead pass it as argument and assert that we got it right. - the check for existing connections should look whether there is an existing active connection of type NMVpnConnection. Instead, what matters is, - do we have a connection of type VPN (otherwise, don't even bother to search for existing-ac) - is the connection already active? Checking whether the connection is already active, and ask backwards whether it's of type NMVpnConnection is odd, maybe even wrong in some cases. --- src/nm-manager.c | 40 +++++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index fff4fc412f..8dcfee238e 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -281,6 +281,7 @@ static void _emit_device_added_removed (NMManager *self, gboolean is_added); static NMActiveConnection *_new_active_connection (NMManager *self, + gboolean is_vpn, NMConnection *connection, NMConnection *applied, const char *specific_object, @@ -2398,8 +2399,13 @@ recheck_assume_connection (NMManager *self, GError *error = NULL; subject = nm_auth_subject_new_internal (); - active = _new_active_connection (self, NM_CONNECTION (connection), NULL, NULL, - device, subject, + active = _new_active_connection (self, + FALSE, + NM_CONNECTION (connection), + NULL, + NULL, + device, + subject, generated ? NM_ACTIVATION_TYPE_EXTERNAL : NM_ACTIVATION_TYPE_ASSUME, NM_ACTIVATION_REASON_AUTOCONNECT, &error); @@ -4026,6 +4032,7 @@ _new_vpn_active_connection (NMManager *self, static NMActiveConnection * _new_active_connection (NMManager *self, + gboolean is_vpn, NMConnection *connection, NMConnection *applied, const char *specific_object, @@ -4036,29 +4043,33 @@ _new_active_connection (NMManager *self, GError **error) { NMSettingsConnection *settings_connection = NULL; - NMActiveConnection *existing_ac; - gboolean is_vpn; g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL); g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); - /* Can't create new AC for already-active connection */ - existing_ac = active_connection_find_first_by_connection (self, connection); - if (NM_IS_VPN_CONNECTION (existing_ac)) { - g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_CONNECTION_ALREADY_ACTIVE, - "Connection '%s' is already active", - nm_connection_get_id (connection)); - return NULL; + nm_assert (is_vpn == _connection_is_vpn (connection)); + + if (is_vpn) { + /* FIXME: for VPN connections, we don't allow re-activating an + * already active connection. It's a bug, and should be fixed together + * when reworking VPN handling. */ + if (active_connection_find_first_by_connection (self, connection)) { + g_set_error (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_CONNECTION_ALREADY_ACTIVE, + "Connection '%s' is already active", + nm_connection_get_id (connection)); + return NULL; + } } specific_object = nm_utils_dbus_normalize_object_path (specific_object); - is_vpn = _connection_is_vpn (NM_CONNECTION (connection)); - if (NM_IS_SETTINGS_CONNECTION (connection)) settings_connection = (NMSettingsConnection *) connection; if (is_vpn) { + /* FIXME: apparently, activation here only works if @connection is + * a settings-connection. Which is not the case during AddAndActivatate. + * Probably, AddAndActivate is broken for VPN. */ if (activation_type != NM_ACTIVATION_TYPE_MANAGED) g_return_val_if_reached (NULL); return _new_vpn_active_connection (self, @@ -4200,6 +4211,7 @@ nm_manager_activate_connection (NMManager *self, } active = _new_active_connection (self, + FALSE, NM_CONNECTION (connection), applied, specific_object, @@ -4439,6 +4451,7 @@ impl_manager_activate_connection (NMDBusObject *obj, goto error; active = _new_active_connection (self, + is_vpn, NM_CONNECTION (connection), NULL, specific_object_path, @@ -4658,6 +4671,7 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, } active = _new_active_connection (self, + is_vpn, connection, NULL, specific_object_path, From 0458e4bb28434e338947e9e2174f9c1fbad2287e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 12:01:25 +0200 Subject: [PATCH 16/20] manager: use cleanup attribute in impl_manager_add_and_activate_connection() and related Also, in _add_and_activate_auth_done(), always steal the connection from active's user-data. Otherwise, the lifetime of the connection is extended until active gets destroyed. For example, if we would leak active, we would also leak connection that way. --- src/nm-manager.c | 62 +++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 8dcfee238e..dd2e5a4a98 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4499,7 +4499,7 @@ activation_add_done (NMSettings *settings, AddAndActivateInfo *info = user_data; NMManager *self; gs_unref_object NMActiveConnection *active = NULL; - GError *local = NULL; + gs_free_error GError *local = NULL; self = info->manager; active = info->active; @@ -4532,6 +4532,7 @@ activation_add_done (NMSettings *settings, } nm_assert (error); + nm_active_connection_set_state_fail (active, NM_ACTIVE_CONNECTION_STATE_REASON_UNKNOWN, error->message); @@ -4544,7 +4545,6 @@ activation_add_done (NMSettings *settings, NULL, nm_active_connection_get_subject (active), error->message); - g_clear_error (&local); } static void @@ -4559,27 +4559,12 @@ _add_and_activate_auth_done (NMActiveConnection *active, GDBusMethodInvocation *context = user_data2; AddAndActivateInfo *info; GError *error = NULL; + gs_unref_object NMConnection *connection = NULL; - if (success) { - NMConnection *connection; + connection = g_object_steal_qdata (G_OBJECT (active), + active_connection_add_and_activate_quark ()); - connection = g_object_steal_qdata (G_OBJECT (active), - active_connection_add_and_activate_quark ()); - - info = g_slice_new (AddAndActivateInfo); - info->manager = self; - info->active = g_object_ref (active); - - /* Basic sender auth checks performed; try to add the connection */ - nm_settings_add_connection_dbus (priv->settings, - connection, - FALSE, - context, - activation_add_done, - info); - g_object_unref (connection); - } else { - g_assert (error_desc); + if (!success) { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, error_desc); @@ -4590,9 +4575,23 @@ _add_and_activate_auth_done (NMActiveConnection *active, nm_active_connection_get_subject (active), error->message); g_dbus_method_invocation_take_error (context, error); + g_object_unref (active); + return; } - g_object_unref (active); + info = g_slice_new (AddAndActivateInfo); + info->manager = self; + + /* we pass on the reference to @active. */ + info->active = active; + + /* Basic sender auth checks performed; try to add the connection */ + nm_settings_add_connection_dbus (priv->settings, + connection, + FALSE, + context, + activation_add_done, + info); } static void @@ -4606,9 +4605,9 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, { NMManager *self = NM_MANAGER (obj); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - NMConnection *connection = NULL; + gs_unref_object NMConnection *connection = NULL; NMActiveConnection *active = NULL; - NMAuthSubject *subject = NULL; + gs_unref_object NMAuthSubject *subject = NULL; GError *error = NULL; NMDevice *device = NULL; gboolean is_vpn = FALSE; @@ -4683,22 +4682,25 @@ impl_manager_add_and_activate_connection (NMDBusObject *obj, if (!active) goto error; + /* FIXME: nm_active_connection_authorize() already has two user-data pointers + * to piggyback additional data. Instead of attaching the third argument to + * @active's user-data, add a third paramter. + * Or alternatively, allocate a data structure to pass on additional data. + * Then we don't need two user-data pointers. */ g_object_set_qdata_full (G_OBJECT (active), active_connection_add_and_activate_quark (), connection, g_object_unref); nm_active_connection_authorize (active, connection, _add_and_activate_auth_done, self, invocation); - g_object_unref (subject); + + /* we passed the pointers on to the callback of authorize. */ + g_steal_pointer (&connection); + g_steal_pointer (&active); return; error: nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ADD_ACTIVATE, NULL, FALSE, NULL, subject, error->message); - g_clear_object (&connection); - g_clear_object (&subject); - g_clear_object (&active); - - g_assert (error); g_dbus_method_invocation_take_error (invocation, error); } From 10753c36168a82cd658df8a7da800960fddd78ed Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 13:37:40 +0200 Subject: [PATCH 17/20] manager: merge VPN handling into _new_active_connection() Merge _new_vpn_active_connection() into _new_active_connection(). It was the only caller, and it is simpler to have all the code visible at one place. That also shows, that the device argument is ignored and not handled. Ensure that no device is specified for VPN type activations. --- src/nm-manager.c | 121 ++++++++++++++++++++++++----------------------- 1 file changed, 63 insertions(+), 58 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index dd2e5a4a98..67171e4b9a 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3985,51 +3985,6 @@ _internal_activate_generic (NMManager *self, NMActiveConnection *active, GError return success; } -static NMActiveConnection * -_new_vpn_active_connection (NMManager *self, - NMSettingsConnection *settings_connection, - const char *specific_object, - NMAuthSubject *subject, - NMActivationReason activation_reason, - GError **error) -{ - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - NMActiveConnection *parent = NULL; - NMDevice *device = NULL; - - g_return_val_if_fail (!settings_connection || NM_IS_SETTINGS_CONNECTION (settings_connection), NULL); - - if (specific_object) { - /* Find the specific connection the client requested we use */ - parent = active_connection_get_by_path (self, specific_object); - if (!parent) { - g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_CONNECTION_NOT_ACTIVE, - "Base connection for VPN connection not active."); - return NULL; - } - } else - parent = priv->primary_connection; - - if (!parent) { - g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_CONNECTION, - "Could not find source connection."); - return NULL; - } - - device = nm_active_connection_get_device (parent); - if (!device) { - g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, - "Source connection had no active device."); - return NULL; - } - - return (NMActiveConnection *) nm_vpn_connection_new (settings_connection, - device, - nm_dbus_object_get_path (NM_DBUS_OBJECT (parent)), - activation_reason, - subject); -} - static NMActiveConnection * _new_active_connection (NMManager *self, gboolean is_vpn, @@ -4042,6 +3997,7 @@ _new_active_connection (NMManager *self, NMActivationReason activation_reason, GError **error) { + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); NMSettingsConnection *settings_connection = NULL; g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL); @@ -4049,6 +4005,9 @@ _new_active_connection (NMManager *self, nm_assert (is_vpn == _connection_is_vpn (connection)); + nm_assert ( ( is_vpn && !device) + || (!is_vpn && NM_IS_DEVICE (device))); + if (is_vpn) { /* FIXME: for VPN connections, we don't allow re-activating an * already active connection. It's a bug, and should be fixed together @@ -4067,20 +4026,48 @@ _new_active_connection (NMManager *self, settings_connection = (NMSettingsConnection *) connection; if (is_vpn) { + NMActiveConnection *parent = NULL; + /* FIXME: apparently, activation here only works if @connection is * a settings-connection. Which is not the case during AddAndActivatate. * Probably, AddAndActivate is broken for VPN. */ if (activation_type != NM_ACTIVATION_TYPE_MANAGED) g_return_val_if_reached (NULL); - return _new_vpn_active_connection (self, - settings_connection, - specific_object, - subject, - activation_reason, - error); + + g_return_val_if_fail (!settings_connection || NM_IS_SETTINGS_CONNECTION (settings_connection), NULL); + + if (specific_object) { + /* Find the specific connection the client requested we use */ + parent = active_connection_get_by_path (self, specific_object); + if (!parent) { + g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_CONNECTION_NOT_ACTIVE, + "Base connection for VPN connection not active."); + return NULL; + } + } else + parent = priv->primary_connection; + + if (!parent) { + g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_CONNECTION, + "Could not find source connection."); + return NULL; + } + + device = nm_active_connection_get_device (parent); + if (!device) { + g_set_error_literal (error, NM_MANAGER_ERROR, NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "Source connection had no active device"); + return NULL; + } + + return (NMActiveConnection *) nm_vpn_connection_new (settings_connection, + device, + nm_dbus_object_get_path (NM_DBUS_OBJECT (parent)), + activation_reason, + subject); } - if (device && (activation_type == NM_ACTIVATION_TYPE_MANAGED)) + if (activation_type == NM_ACTIVATION_TYPE_MANAGED) nm_device_sys_iface_state_set (device, NM_DEVICE_SYS_IFACE_STATE_MANAGED); return (NMActiveConnection *) nm_act_request_new (settings_connection, @@ -4178,14 +4165,17 @@ nm_manager_activate_connection (NMManager *self, NMActivationReason activation_reason, GError **error) { - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); + NMManagerPrivate *priv; NMActiveConnection *active; GSList *iter; - g_return_val_if_fail (self, NULL); - g_return_val_if_fail (connection, NULL); + g_return_val_if_fail (NM_IS_MANAGER (self), NULL); + g_return_val_if_fail (NM_IS_SETTINGS_CONNECTION (connection), NULL); + g_return_val_if_fail (NM_IS_DEVICE (device), NULL); g_return_val_if_fail (!error || !*error, NULL); + priv = NM_MANAGER_GET_PRIVATE (self); + if (!nm_auth_is_subject_in_acl_set_error (NM_CONNECTION (connection), subject, NM_MANAGER_ERROR, @@ -4298,10 +4288,9 @@ validate_activation_request (NMManager *self, "Device not found"); return NULL; } - } else { + } else if (!is_vpn) { device = nm_manager_get_best_device_for_connection (self, connection, TRUE, NULL); - if ( !device - && !is_vpn) { + if (!device) { gs_free char *iface = NULL; /* VPN and software-device connections don't need a device yet, @@ -4330,6 +4319,22 @@ validate_activation_request (NMManager *self, } } + if (is_vpn && device) { + /* VPN's are treated specially. Maybe the should accept a device as well, + * however, later on during activation, we don't handle the device. + * + * Maybe we should, and maybe it makes sense to specify a device + * when activating a VPN. But for now, just error out. */ + g_set_error_literal (error, + NM_MANAGER_ERROR, + NM_MANAGER_ERROR_UNKNOWN_DEVICE, + "Cannot specify device when activating VPN"); + return NULL; + } + + nm_assert ( ( is_vpn && !device) + || (!is_vpn && NM_IS_DEVICE (device))); + *out_device = device; *out_is_vpn = is_vpn; return g_steal_pointer (&subject); From 5c4a6e9b6dfc5fe432c0bfacb4ae64a19b17042d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 14:02:39 +0200 Subject: [PATCH 18/20] manager: ensure valid specific_object path is passed to _new_active_connection() From the D-Bus layer, no specific-object is represented by "/". We should early on normalize such values to NULL, and not expect or handle them later (like during _new_active_connection()). --- src/nm-manager.c | 19 +++++++++---------- src/vpn/nm-vpn-connection.c | 1 + 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 67171e4b9a..f39035dbd3 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4008,7 +4008,14 @@ _new_active_connection (NMManager *self, nm_assert ( ( is_vpn && !device) || (!is_vpn && NM_IS_DEVICE (device))); + nm_assert (!nm_streq0 (specific_object, "/")); + + if (NM_IS_SETTINGS_CONNECTION (connection)) + settings_connection = (NMSettingsConnection *) connection; + if (is_vpn) { + NMActiveConnection *parent; + /* FIXME: for VPN connections, we don't allow re-activating an * already active connection. It's a bug, and should be fixed together * when reworking VPN handling. */ @@ -4018,15 +4025,6 @@ _new_active_connection (NMManager *self, nm_connection_get_id (connection)); return NULL; } - } - - specific_object = nm_utils_dbus_normalize_object_path (specific_object); - - if (NM_IS_SETTINGS_CONNECTION (connection)) - settings_connection = (NMSettingsConnection *) connection; - - if (is_vpn) { - NMActiveConnection *parent = NULL; /* FIXME: apparently, activation here only works if @connection is * a settings-connection. Which is not the case during AddAndActivatate. @@ -4173,6 +4171,7 @@ nm_manager_activate_connection (NMManager *self, g_return_val_if_fail (NM_IS_SETTINGS_CONNECTION (connection), NULL); g_return_val_if_fail (NM_IS_DEVICE (device), NULL); g_return_val_if_fail (!error || !*error, NULL); + nm_assert (!nm_streq0 (specific_object, "/")); priv = NM_MANAGER_GET_PRIVATE (self); @@ -4192,7 +4191,7 @@ nm_manager_activate_connection (NMManager *self, active = iter->data; if ( connection == nm_active_connection_get_settings_connection (active) - && g_strcmp0 (nm_active_connection_get_specific_object (active), specific_object) == 0 + && nm_streq0 (nm_active_connection_get_specific_object (active), specific_object) && nm_active_connection_get_device (active) == device && nm_auth_subject_is_internal (nm_active_connection_get_subject (active)) && nm_auth_subject_is_internal (subject) diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index dd7be77492..436fc68b47 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -855,6 +855,7 @@ nm_vpn_connection_new (NMSettingsConnection *settings_connection, { g_return_val_if_fail (!settings_connection || NM_IS_SETTINGS_CONNECTION (settings_connection), NULL); g_return_val_if_fail (NM_IS_DEVICE (parent_device), NULL); + g_return_val_if_fail (specific_object, NULL); return (NMVpnConnection *) g_object_new (NM_TYPE_VPN_CONNECTION, NM_ACTIVE_CONNECTION_INT_SETTINGS_CONNECTION, settings_connection, From 9fe4239f33d13f06cf375ffb900071f612079193 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 12 Apr 2018 15:40:32 +0200 Subject: [PATCH 19/20] manager: some refactoring of error paths to return early Often, functions perform a series of steps, and when they fail, they bail out. It's simpler if the code is structured that way, so you can read it from top to bottom and whenever something is wrong, either return directly (or goto a cleanup label at the bottom). --- src/nm-manager.c | 54 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index f39035dbd3..9b0654c7a6 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4092,14 +4092,16 @@ _internal_activation_auth_done (NMActiveConnection *active, priv->authorizing_connections = g_slist_remove (priv->authorizing_connections, active); + if (!success) + goto fail; + /* Don't continue with an internal activation if an equivalent active * connection already exists. Note that slave autoconnections always force a * reconnection. We also check this earlier, but there we may fail to * detect a duplicate if the existing active connection is undergoing * authorization in impl_manager_activate_connection(). */ - if ( success - && nm_auth_subject_is_internal (nm_active_connection_get_subject (active)) + if ( nm_auth_subject_is_internal (nm_active_connection_get_subject (active)) && nm_active_connection_get_activation_reason (active) != NM_ACTIVATION_REASON_AUTOCONNECT_SLAVES) { c_list_for_each_entry (ac, &priv->active_connections_lst_head, active_connections_lst) { if ( nm_active_connection_get_device (ac) == nm_active_connection_get_device (active) @@ -4112,17 +4114,15 @@ _internal_activation_auth_done (NMActiveConnection *active, NM_MANAGER_ERROR_CONNECTION_ALREADY_ACTIVE, "Connection '%s' is already active", nm_active_connection_get_settings_connection_id (active)); - success = FALSE; - break; + goto fail; } } } - if (success) { - if (_internal_activate_generic (self, active, &error)) - return; - } + if (_internal_activate_generic (self, active, &error)) + return; +fail: nm_assert (error_desc || error); nm_active_connection_set_state_fail (active, NM_ACTIVE_CONNECTION_STATE_REASON_UNKNOWN, @@ -4209,10 +4209,11 @@ nm_manager_activate_connection (NMManager *self, activation_type, activation_reason, error); - if (active) { - priv->authorizing_connections = g_slist_prepend (priv->authorizing_connections, active); - nm_active_connection_authorize (active, NULL, _internal_activation_auth_done, self, NULL); - } + if (!active) + return NULL; + + priv->authorizing_connections = g_slist_prepend (priv->authorizing_connections, active); + nm_active_connection_authorize (active, NULL, _internal_activation_auth_done, self, NULL); return active; } @@ -4358,24 +4359,27 @@ _activation_auth_done (NMActiveConnection *active, subject = nm_active_connection_get_subject (active); connection = nm_active_connection_get_settings_connection (active); - if (success) { - if (_internal_activate_generic (self, active, &error)) { - nm_settings_connection_autoconnect_blocked_reason_set (connection, - NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_USER_REQUEST, - FALSE); - g_dbus_method_invocation_return_value (context, - g_variant_new ("(o)", - nm_dbus_object_get_path (NM_DBUS_OBJECT (active)))); - nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ACTIVATE, connection, TRUE, NULL, - subject, NULL); - return; - } - } else { + if (!success) { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, error_desc); + goto fail; } + if (!_internal_activate_generic (self, active, &error)) + goto fail; + + nm_settings_connection_autoconnect_blocked_reason_set (connection, + NM_SETTINGS_AUTO_CONNECT_BLOCKED_REASON_USER_REQUEST, + FALSE); + g_dbus_method_invocation_return_value (context, + g_variant_new ("(o)", + nm_dbus_object_get_path (NM_DBUS_OBJECT (active)))); + nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ACTIVATE, connection, TRUE, NULL, + subject, NULL); + return; + +fail: nm_audit_log_connection_op (NM_AUDIT_OP_CONN_ACTIVATE, connection, FALSE, NULL, subject, error->message); nm_active_connection_set_state_fail (active, From c3fb02641a22412c6094bb0b54d3945dedbe1237 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 17 Apr 2018 07:31:13 +0200 Subject: [PATCH 20/20] device: set device's sys-iface-state only shortly before activating device During _new_active_connection() we just create the NMActiveConnection instance to proceed with authorization. The caller might not even authorize, so we must not touch the device yet. Do that only later. --- src/nm-manager.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 9b0654c7a6..e692aea54f 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -3806,6 +3806,9 @@ _internal_activate_device (NMManager *self, NMActiveConnection *active, GError * return FALSE; } + if (nm_active_connection_get_activation_type (active) == NM_ACTIVATION_TYPE_MANAGED) + nm_device_sys_iface_state_set (device, NM_DEVICE_SYS_IFACE_STATE_MANAGED); + /* Create any backing resources the device needs */ if (!nm_device_is_real (device)) { NMDevice *parent; @@ -4065,9 +4068,6 @@ _new_active_connection (NMManager *self, subject); } - if (activation_type == NM_ACTIVATION_TYPE_MANAGED) - nm_device_sys_iface_state_set (device, NM_DEVICE_SYS_IFACE_STATE_MANAGED); - return (NMActiveConnection *) nm_act_request_new (settings_connection, applied, specific_object,