From b997805689cfe2613d84141a5bf6e8f723bb8093 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 7 Aug 2015 17:40:37 +0200 Subject: fixup! activation-request: cease using the secrets if the original connection changed The @reqid returned from nm_settings_connection_get_secrets() is not the same domain as g_idle_add(). Hence, we cannot mix them but must treat them separate. While at it, replace the guint32 return type by an opaque pointer. Also, invoke now ~always~ the callback (possibly with G_IO_ERROR/CANCELLED). --- src/devices/nm-device-ethernet.c | 6 +- src/devices/wifi/nm-device-wifi.c | 6 +- src/devices/wwan/nm-modem.c | 38 +++------ src/nm-activation-request.c | 170 +++++++++++++++++++++++--------------- src/nm-activation-request.h | 18 ++-- src/nm-active-connection.c | 4 +- src/ppp-manager/nm-ppp-manager.c | 18 ++-- 7 files changed, 147 insertions(+), 113 deletions(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index fd7184ca52..0c7f19d397 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -480,7 +480,7 @@ supplicant_interface_release (NMDeviceEthernet *self) static void wired_secrets_cb (NMActRequest *req, - guint32 call_id, + NMActRequestGetSecretsCallId call_id, NMSettingsConnection *connection, GError *error, gpointer user_data) @@ -488,7 +488,9 @@ wired_secrets_cb (NMActRequest *req, NMDeviceEthernet *self = NM_DEVICE_ETHERNET (user_data); NMDevice *dev = NM_DEVICE (self); - g_return_if_fail (req == nm_device_get_act_request (dev)); + if (req != nm_device_get_act_request (dev)) + return; + g_return_if_fail (nm_device_get_state (dev) == NM_DEVICE_STATE_NEED_AUTH); g_return_if_fail (nm_act_request_get_settings_connection (req) == connection); diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 232fb358df..8e94aaacc1 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -1645,7 +1645,7 @@ cleanup_association_attempt (NMDeviceWifi *self, gboolean disconnect) static void wifi_secrets_cb (NMActRequest *req, - guint32 call_id, + NMActRequestGetSecretsCallId call_id, NMSettingsConnection *connection, GError *error, gpointer user_data) @@ -1653,7 +1653,9 @@ wifi_secrets_cb (NMActRequest *req, NMDevice *device = NM_DEVICE (user_data); NMDeviceWifi *self = NM_DEVICE_WIFI (device); - g_return_if_fail (req == nm_device_get_act_request (device)); + if (req != nm_device_get_act_request (device)) + return; + g_return_if_fail (nm_device_get_state (device) == NM_DEVICE_STATE_NEED_AUTH); g_return_if_fail (nm_act_request_get_settings_connection (req) == connection); diff --git a/src/devices/wwan/nm-modem.c b/src/devices/wwan/nm-modem.c index bf210fef8c..fd6de8afdf 100644 --- a/src/devices/wwan/nm-modem.c +++ b/src/devices/wwan/nm-modem.c @@ -31,6 +31,7 @@ #include "nm-dbus-glib-types.h" #include "nm-modem-enum-types.h" #include "nm-route-manager.h" +#include "gsystem-local-alloc.h" G_DEFINE_TYPE (NMModem, nm_modem, G_TYPE_OBJECT) @@ -74,7 +75,7 @@ typedef struct { NMActRequest *act_request; guint32 secrets_tries; - guint32 secrets_id; + NMActRequestGetSecretsCallId secrets_id; guint32 mm_ip_timeout; @@ -723,15 +724,12 @@ cancel_get_secrets (NMModem *self) { NMModemPrivate *priv = NM_MODEM_GET_PRIVATE (self); - if (priv->secrets_id) { - nm_act_request_cancel_secrets (priv->act_request, priv->secrets_id); - priv->secrets_id = 0; - } + nm_act_request_cancel_secrets (priv->act_request, priv->secrets_id); } static void modem_secrets_cb (NMActRequest *req, - guint32 call_id, + NMActRequestGetSecretsCallId call_id, NMSettingsConnection *connection, GError *error, gpointer user_data) @@ -741,7 +739,10 @@ modem_secrets_cb (NMActRequest *req, g_return_if_fail (call_id == priv->secrets_id); - priv->secrets_id = 0; + priv->secrets_id = NULL; + + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; if (error) nm_log_warn (LOGD_MB, "(%s): %s", nm_modem_get_uid (self), error->message); @@ -768,10 +769,8 @@ nm_modem_get_secrets (NMModem *self, hint, modem_secrets_cb, self); - if (priv->secrets_id) - g_signal_emit (self, signals[AUTH_REQUESTED], 0); - - return !!(priv->secrets_id); + g_signal_emit (self, signals[AUTH_REQUESTED], 0); + return TRUE; } /*****************************************************************************/ @@ -791,8 +790,7 @@ nm_modem_act_stage1_prepare (NMModem *self, NMDeviceStateReason *reason) { NMModemPrivate *priv = NM_MODEM_GET_PRIVATE (self); - NMActStageReturn ret; - GPtrArray *hints = NULL; + gs_unref_ptrarray GPtrArray *hints = NULL; const char *setting_name = NULL; NMSecretAgentGetSecretsFlags flags = NM_SECRET_AGENT_GET_SECRETS_FLAG_ALLOW_INTERACTION; NMConnection *connection; @@ -821,18 +819,8 @@ nm_modem_act_stage1_prepare (NMModem *self, hints ? g_ptr_array_index (hints, 0) : NULL, modem_secrets_cb, self); - if (priv->secrets_id) { - g_signal_emit (self, signals[AUTH_REQUESTED], 0); - ret = NM_ACT_STAGE_RETURN_POSTPONE; - } else { - *reason = NM_DEVICE_STATE_REASON_NO_SECRETS; - ret = NM_ACT_STAGE_RETURN_FAILURE; - } - - if (hints) - g_ptr_array_free (hints, TRUE); - - return ret; + g_signal_emit (self, signals[AUTH_REQUESTED], 0); + return NM_ACT_STAGE_RETURN_POSTPONE; } /*****************************************************************************/ diff --git a/src/nm-activation-request.c b/src/nm-activation-request.c index 3f31007d23..82326e884c 100644 --- a/src/nm-activation-request.c +++ b/src/nm-activation-request.c @@ -42,6 +42,11 @@ G_DEFINE_TYPE (NMActRequest, nm_act_request, NM_TYPE_ACTIVE_CONNECTION) NM_TYPE_ACT_REQUEST, \ NMActRequestPrivate)) +typedef enum { + GET_SECRETS_INFO_TYPE_REQ, + GET_SECRETS_INFO_TYPE_IDLE, +} GetSecretsInfoType; + typedef struct { char *table; char *rule; @@ -99,17 +104,37 @@ check_connection_unmodified (NMActRequest *self, GError **error) return TRUE; } -typedef struct { +/*******************************************************************/ + +struct _NMActRequestGetSecretsCallId { NMActRequest *self; - guint32 call_id; NMActRequestSecretsFunc callback; gpointer callback_data; - GError *error; -} GetSecretsInfo; + GetSecretsInfoType type; + union { + struct { + guint32 id; + } req; + struct { + guint32 id; + GError *error; + } idle; + } t; +}; + +typedef struct _NMActRequestGetSecretsCallId GetSecretsInfo; + +static void +get_secrets_info_free (GetSecretsInfo *info) +{ + if (info->type == GET_SECRETS_INFO_TYPE_IDLE) + g_clear_error (&info->t.idle.error); + g_slice_free (GetSecretsInfo, info); +} static void get_secrets_cb (NMSettingsConnection *connection, - guint32 call_id, + guint32 req_id, const char *agent_username, const char *setting_name, GError *error, @@ -119,7 +144,7 @@ get_secrets_cb (NMSettingsConnection *connection, NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (info->self); gs_free_error GError *local = NULL; - g_return_if_fail (info->call_id == call_id); + g_return_if_fail (info->t.req.id == req_id); priv->secrets_calls = g_slist_remove (priv->secrets_calls, info); if (!error) { @@ -127,22 +152,19 @@ get_secrets_cb (NMSettingsConnection *connection, error = local; } - info->callback (info->self, call_id, connection, error, info->callback_data); - g_free (info); + info->callback (info->self, info, connection, error, info->callback_data); + get_secrets_info_free (info); } static gboolean -not_getting_secrets_cb (gpointer user_data) +get_secrets_idle_cb (GetSecretsInfo *info) { - GetSecretsInfo *info = user_data; - - info->callback (info->self, info->call_id, NULL, info->error, info->callback_data); - g_free (info); - + info->callback (info->self, info, NULL, info->t.idle.error, info->callback_data); + get_secrets_info_free (info); return FALSE; } -guint32 +NMActRequestGetSecretsCallId nm_act_request_get_secrets (NMActRequest *self, const char *setting_name, NMSecretAgentGetSecretsFlags flags, @@ -154,73 +176,89 @@ nm_act_request_get_secrets (NMActRequest *self, GetSecretsInfo *info; NMSettingsConnection *connection; const char *hints[2] = { hint, NULL }; + GError *local = NULL; + guint32 req_id; - g_return_val_if_fail (self, 0); g_return_val_if_fail (NM_IS_ACT_REQUEST (self), 0); priv = NM_ACT_REQUEST_GET_PRIVATE (self); connection = nm_act_request_get_settings_connection (self); - info = g_malloc0 (sizeof (GetSecretsInfo)); + info = g_slice_new (GetSecretsInfo); info->self = self; info->callback = callback; info->callback_data = callback_data; - if (!check_connection_unmodified (self, &info->error)) - return info->call_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE, - not_getting_secrets_cb, - info, - (GDestroyNotify) not_getting_secrets_cb); + priv->secrets_calls = g_slist_append (priv->secrets_calls, info); + + if (!check_connection_unmodified (self, &local)) + goto schedule_dummy; if (nm_active_connection_get_user_requested (NM_ACTIVE_CONNECTION (self))) flags |= NM_SECRET_AGENT_GET_SECRETS_FLAG_USER_REQUESTED; - info->call_id = nm_settings_connection_get_secrets (connection, - nm_active_connection_get_subject (NM_ACTIVE_CONNECTION (self)), - setting_name, - flags, - hints, - get_secrets_cb, - info, - &info->error); - if (info->call_id > 0) - priv->secrets_calls = g_slist_append (priv->secrets_calls, info); + req_id = nm_settings_connection_get_secrets (connection, + nm_active_connection_get_subject (NM_ACTIVE_CONNECTION (self)), + setting_name, + flags, + hints, + get_secrets_cb, + info, + &local); + g_assert ((req_id > 0 && !local) || (req_id == 0 && local)); + if (req_id > 0) { + info->type = GET_SECRETS_INFO_TYPE_REQ; + info->t.req.id = req_id; + } else { +schedule_dummy: + info->type = GET_SECRETS_INFO_TYPE_IDLE; + g_propagate_error (&info->t.idle.error, local); + info->t.idle.id = g_idle_add ((GSourceFunc) get_secrets_idle_cb, info); + } + return info; +} + +static void +get_secrets_cancel (NMActRequest *self, GetSecretsInfo *info, gboolean shutdown) +{ + gs_free_error GError *error = NULL; + + if (info->type == GET_SECRETS_INFO_TYPE_REQ) + nm_settings_connection_cancel_secrets (nm_act_request_get_settings_connection (self), info->t.req.id); else - info->call_id = g_idle_add_full (G_PRIORITY_DEFAULT_IDLE, - not_getting_secrets_cb, - info, - (GDestroyNotify) not_getting_secrets_cb); + g_source_remove (info->t.idle.id); + + if (shutdown) { + /* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously + * when the user calls nm_act_request_cancel_secrets(). */ + g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_NOT_CONNECTED, + "Disposing NMActRequest instance"); + } else { + g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED, + "Request cancelled"); + } - return info->call_id; + info->callback (info->self, info, NULL, error, info->callback_data); + get_secrets_info_free (info); } void -nm_act_request_cancel_secrets (NMActRequest *self, guint32 call_id) +nm_act_request_cancel_secrets (NMActRequest *self, NMActRequestGetSecretsCallId call_id) { NMActRequestPrivate *priv; - NMSettingsConnection *connection; - GSList *iter; g_return_if_fail (self); g_return_if_fail (NM_IS_ACT_REQUEST (self)); - g_return_if_fail (call_id > 0); - - priv = NM_ACT_REQUEST_GET_PRIVATE (self); - connection = nm_act_request_get_settings_connection (self); - for (iter = priv->secrets_calls; iter; iter = g_slist_next (iter)) { - GetSecretsInfo *info = iter->data; + if (!call_id) + return; - /* Remove the matching info */ - if (info->call_id == call_id) { - priv->secrets_calls = g_slist_remove_link (priv->secrets_calls, iter); - g_slist_free (iter); + priv = NM_ACT_REQUEST_GET_PRIVATE (self); - nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), call_id); - g_free (info); - break; - } + if (g_slist_find (priv->secrets_calls, call_id)) { + priv->secrets_calls = g_slist_remove (priv->secrets_calls, call_id); + get_secrets_cancel (self, call_id, FALSE); } } @@ -479,9 +517,18 @@ nm_act_request_init (NMActRequest *req) static void dispose (GObject *object) { - NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (object); + NMActRequest *self = NM_ACT_REQUEST (object); + NMActRequestPrivate *priv = NM_ACT_REQUEST_GET_PRIVATE (self); NMSettingsConnection *connection; - GSList *iter; + + /* Kill any in-progress secrets requests */ + connection = nm_act_request_get_settings_connection (NM_ACT_REQUEST (object)); + while (priv->secrets_calls) { + GetSecretsInfo *info = priv->secrets_calls->data; + + priv->secrets_calls = g_slist_delete_link (priv->secrets_calls, priv->secrets_calls); + get_secrets_cancel (self, info, TRUE); + } /* Clear any share rules */ if (priv->share_rules) { @@ -489,17 +536,6 @@ dispose (GObject *object) clear_share_rules (NM_ACT_REQUEST (object)); } - /* Kill any in-progress secrets requests */ - connection = nm_act_request_get_settings_connection (NM_ACT_REQUEST (object)); - for (iter = priv->secrets_calls; connection && iter; iter = g_slist_next (iter)) { - GetSecretsInfo *info = iter->data; - - nm_settings_connection_cancel_secrets (NM_SETTINGS_CONNECTION (connection), info->call_id); - g_free (info); - } - g_slist_free (priv->secrets_calls); - priv->secrets_calls = NULL; - G_OBJECT_CLASS (nm_act_request_parent_class)->dispose (object); } diff --git a/src/nm-activation-request.h b/src/nm-activation-request.h index bc9c7a8d0e..bc511bab00 100644 --- a/src/nm-activation-request.h +++ b/src/nm-activation-request.h @@ -33,6 +33,8 @@ #define NM_IS_ACT_REQUEST_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), NM_TYPE_ACT_REQUEST)) #define NM_ACT_REQUEST_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), NM_TYPE_ACT_REQUEST, NMActRequestClass)) +typedef struct _NMActRequestGetSecretsCallId *NMActRequestGetSecretsCallId; + struct _NMActRequest { NMActiveConnection parent; }; @@ -64,19 +66,19 @@ void nm_act_request_add_share_rule (NMActRequest *req, /* Secrets handling */ typedef void (*NMActRequestSecretsFunc) (NMActRequest *req, - guint32 call_id, + NMActRequestGetSecretsCallId call_id, NMSettingsConnection *connection, GError *error, gpointer user_data); -guint32 nm_act_request_get_secrets (NMActRequest *req, - const char *setting_name, - NMSecretAgentGetSecretsFlags flags, - const char *hint, - NMActRequestSecretsFunc callback, - gpointer callback_data); +NMActRequestGetSecretsCallId nm_act_request_get_secrets (NMActRequest *req, + const char *setting_name, + NMSecretAgentGetSecretsFlags flags, + const char *hint, + NMActRequestSecretsFunc callback, + gpointer callback_data); -void nm_act_request_cancel_secrets (NMActRequest *req, guint32 call_id); +void nm_act_request_cancel_secrets (NMActRequest *req, NMActRequestGetSecretsCallId call_id); #endif /* __NETWORKMANAGER_ACTIVATION_REQUEST_H__ */ diff --git a/src/nm-active-connection.c b/src/nm-active-connection.c index 6bc02031ad..cb097c1b59 100644 --- a/src/nm-active-connection.c +++ b/src/nm-active-connection.c @@ -236,7 +236,9 @@ nm_active_connection_set_connection (NMActiveConnection *self, { NMActiveConnectionPrivate *priv = NM_ACTIVE_CONNECTION_GET_PRIVATE (self); - /* Can't change connection after the ActiveConnection is exported over D-Bus */ + /* Can't change connection after the ActiveConnection is exported over D-Bus. + * Also, if we would do that, we would have to cancel all pending secret requests + * from NMActRequest. */ g_return_if_fail (!nm_exported_object_is_exported (NM_EXPORTED_OBJECT (self))); g_return_if_fail (priv->connection == NULL || !NM_IS_SETTINGS_CONNECTION (priv->connection)); diff --git a/src/ppp-manager/nm-ppp-manager.c b/src/ppp-manager/nm-ppp-manager.c index dde9e2a163..344f57a71b 100644 --- a/src/ppp-manager/nm-ppp-manager.c +++ b/src/ppp-manager/nm-ppp-manager.c @@ -78,7 +78,7 @@ typedef struct { NMActRequest *act_req; DBusGMethodInvocation *pending_secrets_context; - guint32 secrets_id; + NMActRequestGetSecretsCallId secrets_id; const char *secrets_setting_name; guint32 ppp_watch_id; @@ -244,7 +244,7 @@ static void remove_timeout_handler (NMPPPManager *manager) { NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (manager); - + if (priv->ppp_timeout_handler) { g_source_remove (priv->ppp_timeout_handler); priv->ppp_timeout_handler = 0; @@ -256,11 +256,10 @@ cancel_get_secrets (NMPPPManager *self) { NMPPPManagerPrivate *priv = NM_PPP_MANAGER_GET_PRIVATE (self); - if (priv->secrets_id) { + if (priv->secrets_id) nm_act_request_cancel_secrets (priv->act_req, priv->secrets_id); - priv->secrets_id = 0; - } - priv->secrets_setting_name = NULL; + + g_return_if_fail (!priv->secrets_id && !priv->secrets_setting_name); } static gboolean @@ -324,7 +323,7 @@ extract_details_from_connection (NMConnection *connection, static void ppp_secrets_cb (NMActRequest *req, - guint32 call_id, + NMActRequestGetSecretsCallId call_id, NMSettingsConnection *connection, GError *error, gpointer user_data) @@ -339,6 +338,9 @@ ppp_secrets_cb (NMActRequest *req, g_return_if_fail (req == priv->act_req); g_return_if_fail (call_id == priv->secrets_id); + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + goto out; + if (error) { nm_log_warn (LOGD_PPP, "%s", error->message); dbus_g_method_return_error (priv->pending_secrets_context, error); @@ -362,7 +364,7 @@ ppp_secrets_cb (NMActRequest *req, out: priv->pending_secrets_context = NULL; - priv->secrets_id = 0; + priv->secrets_id = NULL; priv->secrets_setting_name = NULL; } -- cgit v1.2.1