From 6bcc1eda0b3381e9a8bab1fc393ca1e58a54af05 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 14:08:22 +0200 Subject: [PATCH 01/12] macros: add NM_PRINT_FMT_QUOTE_STRING() macro --- include/nm-macros-internal.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/nm-macros-internal.h b/include/nm-macros-internal.h index 291dc0742a..7ae08bf727 100644 --- a/include/nm-macros-internal.h +++ b/include/nm-macros-internal.h @@ -175,6 +175,7 @@ (cond) ? (prefix) : "", \ (cond) ? (str) : (str_else), \ (cond) ? (suffix) : "" +#define NM_PRINT_FMT_QUOTE_STRING(arg) NM_PRINT_FMT_QUOTED((arg), "\"", (arg), "\"", "(null)") /*****************************************************************************/ From 8ed98a381b2fb1e71182b7400b84997a81af5d30 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 23:22:08 +0200 Subject: [PATCH 02/12] secret-agent: fix leak of @dbus_owner The @dbus_owner field was only cleaned up when the proxy disconnected and leaked otherwise. Also, don't clear @dbus_owner together with the proxy. Otherwise, get_description() might yield different results after the proxy got cleared. That can lead to problems because NMAgentManager tracks the secrets agents by their "dbus-owner" -- IOW, NMAgentManager uses the "dbus-owner" as identifer for the secret agent. Thus it must not change. Fixes: 2a2fd1216b15efc6ef15ba4e49c0aa7b5969e6d7 --- src/settings/nm-secret-agent.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index c2c220fd41..0d267ee42f 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -470,8 +470,6 @@ name_owner_changed_cb (GObject *proxy, g_signal_handlers_disconnect_by_func (priv->proxy, name_owner_changed_cb, self); g_clear_object (&priv->proxy); g_signal_emit (self, signals[DISCONNECTED], 0); - - g_clear_pointer (&priv->dbus_owner, g_free); } else g_free (owner); } @@ -555,6 +553,7 @@ finalize (GObject *object) g_free (priv->description); g_free (priv->identifier); g_free (priv->owner_username); + g_free (priv->dbus_owner); g_slist_free_full (priv->permissions, g_free); g_hash_table_destroy (priv->requests); From 88e485bc1d846920062921cad152bd0996b73d0d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 24 Aug 2015 19:20:55 +0200 Subject: [PATCH 03/12] secret-agent: refactor call-id to be of an opaque pointer type instead of a void pointer This gives some type safety. --- src/settings/nm-agent-manager.c | 14 ++++++------- src/settings/nm-secret-agent.c | 14 +++++++------ src/settings/nm-secret-agent.h | 36 +++++++++++++++++---------------- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 34c2fecce1..d9dc1daf9c 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -396,7 +396,7 @@ struct _Request { /* Current agent being asked for secrets */ NMSecretAgent *current; - gconstpointer current_call_id; + NMSecretAgentCallId current_call_id; /* Stores the sorted list of NMSecretAgents which will be asked for secrets */ GSList *pending; @@ -770,7 +770,7 @@ connection_request_new_other (NMConnection *connection, static void get_done_cb (NMSecretAgent *agent, - gconstpointer call_id, + NMSecretAgentCallId call_id, GVariant *secrets, GError *error, gpointer user_data) @@ -1220,7 +1220,7 @@ nm_agent_manager_cancel_secrets (NMAgentManager *self, static void save_done_cb (NMSecretAgent *agent, - gconstpointer call_id, + NMSecretAgentCallId call_id, GVariant *secrets, GError *error, gpointer user_data) @@ -1315,10 +1315,10 @@ nm_agent_manager_save_secrets (NMAgentManager *self, static void delete_done_cb (NMSecretAgent *agent, - gconstpointer call_id, - GVariant *secrets, - GError *error, - gpointer user_data) + NMSecretAgentCallId call_id, + GVariant *secrets, + GError *error, + gpointer user_data) { Request *req = user_data; diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 0d267ee42f..e34f015e56 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -63,14 +63,16 @@ static guint signals[LAST_SIGNAL] = { 0 }; /*************************************************************/ -typedef struct { +struct _NMSecretAgentCallId { NMSecretAgent *agent; GCancellable *cancellable; char *path; char *setting_name; NMSecretAgentCallback callback; gpointer callback_data; -} Request; +}; + +typedef struct _NMSecretAgentCallId Request; static Request * request_new (NMSecretAgent *agent, @@ -279,7 +281,7 @@ get_callback (GObject *proxy, g_hash_table_remove (priv->requests, r); } -gconstpointer +NMSecretAgentCallId nm_secret_agent_get_secrets (NMSecretAgent *self, NMConnection *connection, const char *setting_name, @@ -337,7 +339,7 @@ cancel_done (GObject *proxy, GAsyncResult *result, gpointer user_data) } void -nm_secret_agent_cancel_secrets (NMSecretAgent *self, gconstpointer call) +nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId call) { NMSecretAgentPrivate *priv; Request *r = (gpointer) call; @@ -378,7 +380,7 @@ agent_save_cb (GObject *proxy, g_hash_table_remove (priv->requests, r); } -gconstpointer +NMSecretAgentCallId nm_secret_agent_save_secrets (NMSecretAgent *self, NMConnection *connection, NMSecretAgentCallback callback, @@ -426,7 +428,7 @@ agent_delete_cb (GObject *proxy, g_hash_table_remove (priv->requests, r); } -gconstpointer +NMSecretAgentCallId nm_secret_agent_delete_secrets (NMSecretAgent *self, NMConnection *connection, NMSecretAgentCallback callback, diff --git a/src/settings/nm-secret-agent.h b/src/settings/nm-secret-agent.h index 7883979d5b..668b1e559c 100644 --- a/src/settings/nm-secret-agent.h +++ b/src/settings/nm-secret-agent.h @@ -43,6 +43,8 @@ typedef struct { void (*disconnected) (NMSecretAgent *self); } NMSecretAgentClass; +typedef struct _NMSecretAgentCallId *NMSecretAgentCallId; + GType nm_secret_agent_get_type (void); NMSecretAgent *nm_secret_agent_new (GDBusMethodInvocation *context, @@ -76,30 +78,30 @@ gboolean nm_secret_agent_has_permission (NMSecretAgent *agent, const char *permission); typedef void (*NMSecretAgentCallback) (NMSecretAgent *agent, - gconstpointer call, + NMSecretAgentCallId call_id, GVariant *new_secrets, /* NULL for save & delete */ GError *error, gpointer user_data); -gconstpointer nm_secret_agent_get_secrets (NMSecretAgent *agent, - NMConnection *connection, - const char *setting_name, - const char **hints, - NMSecretAgentGetSecretsFlags flags, - NMSecretAgentCallback callback, - gpointer callback_data); +NMSecretAgentCallId nm_secret_agent_get_secrets (NMSecretAgent *agent, + NMConnection *connection, + const char *setting_name, + const char **hints, + NMSecretAgentGetSecretsFlags flags, + NMSecretAgentCallback callback, + gpointer callback_data); void nm_secret_agent_cancel_secrets (NMSecretAgent *agent, - gconstpointer call_id); + NMSecretAgentCallId call_id); -gconstpointer nm_secret_agent_save_secrets (NMSecretAgent *agent, - NMConnection *connection, - NMSecretAgentCallback callback, - gpointer callback_data); +NMSecretAgentCallId nm_secret_agent_save_secrets (NMSecretAgent *agent, + NMConnection *connection, + NMSecretAgentCallback callback, + gpointer callback_data); -gconstpointer nm_secret_agent_delete_secrets (NMSecretAgent *agent, - NMConnection *connection, - NMSecretAgentCallback callback, - gpointer callback_data); +NMSecretAgentCallId nm_secret_agent_delete_secrets (NMSecretAgent *agent, + NMConnection *connection, + NMSecretAgentCallback callback, + gpointer callback_data); #endif /* __NETWORKMANAGER_SECRET_AGENT_H__ */ From cf16010fb6bef8f75471694a6469f0ccca97e256 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Aug 2015 10:02:03 +0200 Subject: [PATCH 04/12] agent-manager: fix checking for D-Bus error after gdbus switch With gdbus, errors are now properly translated. We must check for the error domain/code, intead of the dbus-error. Fixes: df6706813a698e7a697739b0940bd8f528713aab --- src/settings/nm-agent-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index d9dc1daf9c..1d202b49eb 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -791,7 +791,7 @@ get_done_cb (NMSecretAgent *agent, error ? error->code : -1, (error && error->message) ? error->message : "(unknown)"); - if (_nm_dbus_error_has_name (error, NM_DBUS_INTERFACE_SECRET_AGENT ".UserCanceled")) { + if (g_error_matches (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_USER_CANCELED)) { error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_USER_CANCELED, "User canceled the secrets request."); From 9cace5b4117eb05efb24b8fc9cec533231f3c8fc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Aug 2015 15:53:08 +0200 Subject: [PATCH 05/12] libnm/trivial: add code comment to _nm_dbus_error_has_name() --- libnm-core/nm-dbus-utils.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libnm-core/nm-dbus-utils.c b/libnm-core/nm-dbus-utils.c index 730a0eb3ec..8627278623 100644 --- a/libnm-core/nm-dbus-utils.c +++ b/libnm-core/nm-dbus-utils.c @@ -269,6 +269,11 @@ _nm_dbus_proxy_call_sync (GDBusProxy *proxy, * * Checks if @error is set and corresponds to the D-Bus error @dbus_error_name. * + * This should only be used for "foreign" D-Bus errors (eg, errors + * from BlueZ or wpa_supplicant). All NetworkManager D-Bus errors + * should be properly mapped by gdbus to one of the domains/codes in + * nm-errors.h. + * * Returns: %TRUE or %FALSE */ gboolean From 92dda6472cb881fc9965a014f16ffabb370b1e55 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Aug 2015 10:36:50 +0200 Subject: [PATCH 06/12] secret-agent: rework handling of asynchronous request and cancelling Refactor the handling of the asynchronous requests so that now NMSecretAgent has the following properties: - The callback will *always* be invoked exactly once (sans crashes). Even if you cancel the call or if you dispose NMSecretAgent with pending calls. That allows the caller to rely on being called back and possibly cleanup the user-data. - Callbacks are always invoked asynchronously with respect to their start-call. - You can cancel all 3 types of operations, not only the 'GetSecrets' call. Note that this will still not cancel the calls 'DeleteSecrets' and 'SaveSecrets' on a D-Bus level. When cancelling, the callback will be invoked synchronously with respect to the cancel call, with an GError indicating the cancellation (G_IO_ERROR_CANCELLED). - During dispose, the callback is also invoked synchronously, with some other error reason. This also fixes a crash where handling of the asynchronous data was messed up and the priv->requests hash would end up to containing dangling pointers. https://bugzilla.redhat.com/show_bug.cgi?id=1253407 --- src/settings/nm-agent-manager.c | 70 ++++++++----- src/settings/nm-secret-agent.c | 171 ++++++++++++++++++++++++-------- 2 files changed, 176 insertions(+), 65 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 1d202b49eb..c94cbae9d9 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -585,9 +585,12 @@ request_next_agent (Request *req) { GError *error = NULL; - req->current_call_id = NULL; - if (req->current) - g_object_unref (req->current); + if (req->current) { + if (req->current_call_id) + nm_secret_agent_cancel_secrets (req->current, req->current_call_id); + g_clear_object (&req->current); + } + g_warn_if_fail (!req->current_call_id); if (req->pending) { /* Send the request to the next agent */ @@ -600,8 +603,6 @@ request_next_agent (Request *req) req->next_callback (req); } else { - req->current = NULL; - /* No more secret agents are available to fulfill this secrets request */ error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, NM_AGENT_MANAGER_ERROR_NO_SECRETS, @@ -783,13 +784,22 @@ get_done_cb (NMSecretAgent *agent, char *agent_uname = NULL; g_return_if_fail (call_id == parent->current_call_id); + g_return_if_fail (agent == parent->current); + + parent->current_call_id = NULL; if (error) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent failed secrets request %p/%s/%s: (%d) %s", + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + nm_log_dbg (LOGD_AGENTS, "(%s) get secrets request cancelled: %p/%s/%s", + nm_secret_agent_get_description (agent), + req, parent->detail, req->setting_name); + return; + } + + nm_log_dbg (LOGD_AGENTS, "(%s) agent failed secrets request %p/%s/%s: %s", nm_secret_agent_get_description (agent), req, parent->detail, req->setting_name, - error ? error->code : -1, - (error && error->message) ? error->message : "(unknown)"); + error->message); if (g_error_matches (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_USER_CANCELED)) { error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, @@ -900,9 +910,8 @@ get_agent_request_secrets (ConnectionRequest *req, gboolean include_system_secre req->flags, get_done_cb, req); - if (parent->current_call_id == NULL) { - /* Shouldn't hit this, but handle it anyway */ - g_warn_if_fail (parent->current_call_id != NULL); + if (!parent->current_call_id) { + g_warn_if_reached (); request_next_agent (parent); } @@ -1230,13 +1239,22 @@ save_done_cb (NMSecretAgent *agent, const char *agent_dbus_owner; g_return_if_fail (call_id == parent->current_call_id); + g_return_if_fail (agent == parent->current); + + parent->current_call_id = NULL; if (error) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent failed save secrets request %p/%s: (%d) %s", + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + nm_log_dbg (LOGD_AGENTS, "(%s) save secrets request cancelled: %p/%s", + nm_secret_agent_get_description (agent), + req, parent->detail); + return; + } + + nm_log_dbg (LOGD_AGENTS, "(%s) agent failed save secrets request %p/%s: %s", nm_secret_agent_get_description (agent), req, parent->detail, - error ? error->code : -1, - (error && error->message) ? error->message : "(unknown)"); + error->message); /* Try the next agent */ request_next_agent (parent); maybe_remove_agent_on_error (agent, error); @@ -1260,9 +1278,8 @@ save_next_cb (Request *parent) req->connection, save_done_cb, req); - if (parent->current_call_id == NULL) { - /* Shouldn't hit this, but handle it anyway */ - g_warn_if_fail (parent->current_call_id != NULL); + if (!parent->current_call_id) { + g_warn_if_reached (); request_next_agent (parent); } } @@ -1323,12 +1340,20 @@ delete_done_cb (NMSecretAgent *agent, Request *req = user_data; g_return_if_fail (call_id == req->current_call_id); + g_return_if_fail (agent == req->current); + + req->current_call_id = NULL; if (error) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent failed delete secrets request %p/%s: (%d) %s", + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { + nm_log_dbg (LOGD_AGENTS, "(%s) delete secrets request cancelled: %p/%s", + nm_secret_agent_get_description (agent), req, req->detail); + return; + } + + nm_log_dbg (LOGD_AGENTS, "(%s) agent failed delete secrets request %p/%s: %s", nm_secret_agent_get_description (agent), req, req->detail, - error ? error->code : -1, - (error && error->message) ? error->message : "(unknown)"); + error->message); } else { nm_log_dbg (LOGD_AGENTS, "(%s) agent deleted secrets for request %p/%s", nm_secret_agent_get_description (agent), req, req->detail); @@ -1349,9 +1374,8 @@ delete_next_cb (Request *parent) req->connection, delete_done_cb, req); - if (parent->current_call_id == NULL) { - /* Shouldn't hit this, but handle it anyway */ - g_warn_if_fail (parent->current_call_id != NULL); + if (!parent->current_call_id) { + g_warn_if_reached (); request_next_agent (parent); } } diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index e34f015e56..d1c41a8287 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -68,6 +68,7 @@ struct _NMSecretAgentCallId { GCancellable *cancellable; char *path; char *setting_name; + gboolean is_get_secrets; NMSecretAgentCallback callback; gpointer callback_data; }; @@ -98,10 +99,29 @@ request_free (Request *r) { g_free (r->path); g_free (r->setting_name); - g_object_unref (r->cancellable); + if (r->cancellable) + g_object_unref (r->cancellable); g_slice_free (Request, r); } +static gboolean +request_check_return (Request *r) +{ + NMSecretAgentPrivate *priv; + + if (!r->cancellable) + return FALSE; + + g_return_val_if_fail (NM_IS_SECRET_AGENT (r->agent), FALSE); + + priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); + + if (!g_hash_table_remove (priv->requests, r)) + g_return_val_if_reached (FALSE); + + return TRUE; +} + /*************************************************************/ const char * @@ -264,21 +284,19 @@ get_callback (GObject *proxy, gpointer user_data) { Request *r = user_data; - NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); - GVariant *secrets = NULL; - GError *error = NULL; - if (!g_cancellable_is_cancelled (r->cancellable)) { + if (request_check_return (r)) { + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); + gs_free_error GError *error = NULL; + gs_unref_variant GVariant *secrets = NULL; + nmdbus_secret_agent_call_get_secrets_finish (priv->proxy, &secrets, result, &error); if (error) g_dbus_error_strip_remote_error (error); - r->callback (r->agent, r, secrets, error, r->callback_data); - g_clear_pointer (&secrets, g_variant_unref); - g_clear_error (&error); } - g_hash_table_remove (priv->requests, r); + request_free (r); } NMSecretAgentCallId @@ -309,6 +327,8 @@ nm_secret_agent_get_secrets (NMSecretAgent *self, flags &= ~NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS; r = request_new (self, nm_connection_get_path (connection), setting_name, callback, callback_data); + r->is_get_secrets = TRUE; + g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_get_secrets (priv->proxy, dict, nm_connection_get_path (connection), @@ -317,11 +337,12 @@ nm_secret_agent_get_secrets (NMSecretAgent *self, flags, r->cancellable, get_callback, r); - g_hash_table_insert (priv->requests, r, r); return r; } +/*************************************************************/ + static void cancel_done (GObject *proxy, GAsyncResult *result, gpointer user_data) { @@ -329,33 +350,85 @@ cancel_done (GObject *proxy, GAsyncResult *result, gpointer user_data) GError *error = NULL; if (!nmdbus_secret_agent_call_cancel_get_secrets_finish (NMDBUS_SECRET_AGENT (proxy), result, &error)) { - g_dbus_error_strip_remote_error (error); - nm_log_dbg (LOGD_AGENTS, "(%s): agent failed to cancel secrets: %s", - description, error->message); + nm_log_dbg (LOGD_AGENTS, "%s%s%s: agent failed to cancel secrets: %s", + NM_PRINT_FMT_QUOTED (description, "(", description, ")", "???"), + error->message); g_clear_error (&error); } g_free (description); } +static void +do_cancel_secrets (NMSecretAgent *self, Request *r, gboolean disposing) +{ + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); + GCancellable *cancellable; + NMSecretAgentCallback callback; + gpointer callback_data; + + g_return_if_fail (r->agent == self); + g_return_if_fail (r->cancellable); + + if ( r->is_get_secrets + && priv->proxy) { + /* for GetSecrets call, we must cancel the request. */ + nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy, + r->path, r->setting_name, + NULL, + cancel_done, + g_strdup (nm_secret_agent_get_description (self))); + } + + cancellable = r->cancellable; + callback = r->callback; + callback_data = r->callback_data; + + /* During g_cancellable_cancel() the d-bus method might return synchronously. + * Clear r->cancellable first, so that it doesn't actually do anything. + * After that, @r might be already freed. */ + r->cancellable = NULL; + g_cancellable_cancel (cancellable); + g_object_unref (cancellable); + + /* Don't free the request @r. It will be freed when the d-bus call returns. + * Only clear r->cancellable to indicate that the request was cancelled. */ + + if (callback) { + GError *error = NULL; + + if (disposing) { + /* hijack an error code. G_IO_ERROR_CANCELLED is only used synchronously + * when the user calls nm_act_request_cancel_secrets(). + * When the user disposes the instance, we also invoke the callback synchronously, + * but with a different error-reason. */ + g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED, + "Disposing NMSecretAgent instance"); + } else { + g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_CANCELLED, + "Request cancelled"); + } + /* @r might be a dangling pointer at this point. However, that is no problem + * to pass it as (opaque) call_id. */ + callback (self, r, NULL, error, callback_data); + g_error_free (error); + } +} + void -nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId call) +nm_secret_agent_cancel_secrets (NMSecretAgent *self, NMSecretAgentCallId call_id) { NMSecretAgentPrivate *priv; - Request *r = (gpointer) call; + Request *r = call_id; + + g_return_if_fail (NM_IS_SECRET_AGENT (self)); + g_return_if_fail (r); - g_return_if_fail (self != NULL); priv = NM_SECRET_AGENT_GET_PRIVATE (self); - g_return_if_fail (priv->proxy != NULL); - g_return_if_fail (r != NULL); + if (!g_hash_table_remove (priv->requests, r)) + g_return_if_reached (); - g_cancellable_cancel (r->cancellable); - - nmdbus_secret_agent_call_cancel_get_secrets (priv->proxy, - r->path, r->setting_name, - NULL, - cancel_done, - g_strdup (nm_secret_agent_get_description (self))); + do_cancel_secrets (self, r, FALSE); } /*************************************************************/ @@ -366,18 +439,18 @@ agent_save_cb (GObject *proxy, gpointer user_data) { Request *r = user_data; - NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); - GError *error = NULL; - if (!g_cancellable_is_cancelled (r->cancellable)) { + if (request_check_return (r)) { + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); + gs_free_error GError *error = NULL; + nmdbus_secret_agent_call_save_secrets_finish (priv->proxy, result, &error); if (error) g_dbus_error_strip_remote_error (error); r->callback (r->agent, r, NULL, error, r->callback_data); - g_clear_error (&error); } - g_hash_table_remove (priv->requests, r); + request_free (r); } NMSecretAgentCallId @@ -401,31 +474,35 @@ nm_secret_agent_save_secrets (NMSecretAgent *self, dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL); r = request_new (self, cpath, NULL, callback, callback_data); + g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_save_secrets (priv->proxy, dict, cpath, - NULL, + NULL, /* cancelling the request does *not* cancel the D-Bus call. */ agent_save_cb, r); - g_hash_table_insert (priv->requests, r, r); return r; } +/*************************************************************/ + static void agent_delete_cb (GObject *proxy, GAsyncResult *result, gpointer user_data) { Request *r = user_data; - NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); - GError *error = NULL; - if (!g_cancellable_is_cancelled (r->cancellable)) { + if (request_check_return (r)) { + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (r->agent); + gs_free_error GError *error = NULL; + nmdbus_secret_agent_call_delete_secrets_finish (priv->proxy, result, &error); + if (error) + g_dbus_error_strip_remote_error (error); r->callback (r->agent, r, NULL, error, r->callback_data); - g_clear_error (&error); } - g_hash_table_remove (priv->requests, r); + request_free (r); } NMSecretAgentCallId @@ -449,15 +526,17 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self, dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_NO_SECRETS); r = request_new (self, cpath, NULL, callback, callback_data); + g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_delete_secrets (priv->proxy, dict, cpath, - NULL, + NULL, /* cancelling the request does *not* cancel the D-Bus call. */ agent_delete_cb, r); - g_hash_table_insert (priv->requests, r, r); return r; } +/*************************************************************/ + static void name_owner_changed_cb (GObject *proxy, GParamSpec *pspec, @@ -532,14 +611,22 @@ nm_secret_agent_init (NMSecretAgent *self) { NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); - priv->requests = g_hash_table_new_full (g_direct_hash, g_direct_equal, - NULL, (GDestroyNotify) request_free); + priv->requests = g_hash_table_new (g_direct_hash, g_direct_equal); } static void dispose (GObject *object) { - NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (object); + NMSecretAgent *self = NM_SECRET_AGENT (object); + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); + GHashTableIter iter; + Request *r; + + g_hash_table_iter_init (&iter, priv->requests); + while (g_hash_table_iter_next (&iter, (gpointer *) &r, NULL)) { + g_hash_table_iter_remove (&iter); + do_cancel_secrets (self, r, TRUE); + } g_clear_object (&priv->proxy); g_clear_object (&priv->subject); From e5c59d1f3834c613bbf0c296370d01af8cd946ca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 14:29:34 +0200 Subject: [PATCH 07/12] secret-agent: don't assert against existing getpwuid() entry There is a race and there is no guarantee that getpwuid() can lookup a uid that (previously) existed. Just accept %NULL as @owner_username. --- src/settings/nm-secret-agent.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index d1c41a8287..2503b6adaf 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -568,6 +568,7 @@ nm_secret_agent_new (GDBusMethodInvocation *context, char *hash_str; struct passwd *pw; GDBusProxy *proxy; + char *owner_username = NULL; g_return_val_if_fail (context != NULL, NULL); g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); @@ -575,14 +576,14 @@ nm_secret_agent_new (GDBusMethodInvocation *context, g_return_val_if_fail (identifier != NULL, NULL); pw = getpwuid (nm_auth_subject_get_unix_process_uid (subject)); - g_return_val_if_fail (pw != NULL, NULL); - g_return_val_if_fail (pw->pw_name[0] != '\0', NULL); + if (pw && pw->pw_name && pw->pw_name[0]) + owner_username = g_strdup (pw->pw_name); self = (NMSecretAgent *) g_object_new (NM_TYPE_SECRET_AGENT, NULL); priv = NM_SECRET_AGENT_GET_PRIVATE (self); priv->identifier = g_strdup (identifier); - priv->owner_username = g_strdup (pw->pw_name); + priv->owner_username = owner_username; priv->dbus_owner = g_strdup (nm_auth_subject_get_unix_process_dbus_sender (subject)); priv->capabilities = capabilities; priv->subject = g_object_ref (subject); From ea14cd45f1cfd60089e05a3bb9a85a0948e1b5ef Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 15:11:54 +0200 Subject: [PATCH 08/12] agent-manager: remove @asked field from request This code was unused, because we never enqueued any hashes to the @asked list. Note that hashing also might give wrong hash collisions, so this was buggy anyway. Also, note that impl_agent_manager_register_with_capabilities() already ensures that duplicate agents are not registered in the first place (find_agent_by_identifier_and_uid()). --- src/settings/nm-agent-manager.c | 10 ---------- src/settings/nm-secret-agent.c | 14 -------------- src/settings/nm-secret-agent.h | 2 -- 3 files changed, 26 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index c94cbae9d9..baacc30a6d 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -401,12 +401,6 @@ struct _Request { /* Stores the sorted list of NMSecretAgents which will be asked for secrets */ GSList *pending; - /* Stores the list of NMSecretAgent hashes that we've already - * asked for secrets, so that we don't ask the same agent twice - * if it quits and re-registers during this secrets request. - */ - GSList *asked; - guint32 idle_id; RequestAddAgentFunc add_agent_callback; @@ -466,7 +460,6 @@ request_free (Request *req) g_free (req->detail); g_free (req->verb); g_slist_free_full (req->pending, g_object_unref); - g_slist_free (req->asked); memset (req, 0, sizeof (Request)); g_free (req); } @@ -535,9 +528,6 @@ request_add_agent (Request *req, NMSecretAgent *agent) g_return_if_fail (req != NULL); g_return_if_fail (agent != NULL); - if (g_slist_find (req->asked, GUINT_TO_POINTER (nm_secret_agent_get_hash (agent)))) - return; - if (req->add_agent_callback && !req->add_agent_callback (req, agent)) return; diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 2503b6adaf..6d11236005 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -45,7 +45,6 @@ typedef struct { char *owner_username; char *dbus_owner; NMSecretAgentCapabilities capabilities; - guint32 hash; GSList *permissions; @@ -190,14 +189,6 @@ nm_secret_agent_get_capabilities (NMSecretAgent *agent) return NM_SECRET_AGENT_GET_PRIVATE (agent)->capabilities; } -guint32 -nm_secret_agent_get_hash (NMSecretAgent *agent) -{ - g_return_val_if_fail (NM_IS_SECRET_AGENT (agent), 0); - - return NM_SECRET_AGENT_GET_PRIVATE (agent)->hash; -} - NMAuthSubject * nm_secret_agent_get_subject (NMSecretAgent *agent) { @@ -565,7 +556,6 @@ nm_secret_agent_new (GDBusMethodInvocation *context, { NMSecretAgent *self; NMSecretAgentPrivate *priv; - char *hash_str; struct passwd *pw; GDBusProxy *proxy; char *owner_username = NULL; @@ -588,10 +578,6 @@ nm_secret_agent_new (GDBusMethodInvocation *context, priv->capabilities = capabilities; priv->subject = g_object_ref (subject); - hash_str = g_strdup_printf ("%16lu%s", nm_auth_subject_get_unix_process_uid (subject), identifier); - priv->hash = g_str_hash (hash_str); - g_free (hash_str); - proxy = nm_bus_manager_new_proxy (nm_bus_manager_get (), context, NMDBUS_TYPE_SECRET_AGENT_PROXY, diff --git a/src/settings/nm-secret-agent.h b/src/settings/nm-secret-agent.h index 668b1e559c..b220dfd53f 100644 --- a/src/settings/nm-secret-agent.h +++ b/src/settings/nm-secret-agent.h @@ -66,8 +66,6 @@ gulong nm_secret_agent_get_pid (NMSecretAgent *agent); NMSecretAgentCapabilities nm_secret_agent_get_capabilities (NMSecretAgent *agent); -guint32 nm_secret_agent_get_hash (NMSecretAgent *agent); - NMAuthSubject *nm_secret_agent_get_subject (NMSecretAgent *agent); void nm_secret_agent_add_permission (NMSecretAgent *agent, From 0b3e0215386adc54cc79920bc71bf5a0b65069c4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 12:51:20 +0200 Subject: [PATCH 09/12] secret-agent: add trace logging to secret agent --- src/settings/nm-secret-agent.c | 80 +++++++++++++++++++++++++++++----- 1 file changed, 68 insertions(+), 12 deletions(-) diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 6d11236005..ad97bc5222 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -32,6 +32,26 @@ #include "nmdbus-secret-agent.h" +#define _NMLOG_PREFIX_NAME "secret-agent" +#define _NMLOG_DOMAIN LOGD_AGENTS +#define _NMLOG(level, ...) \ + G_STMT_START { \ + if (nm_logging_enabled ((level), (_NMLOG_DOMAIN))) { \ + char __prefix[32]; \ + \ + if ((self)) \ + g_snprintf (__prefix, sizeof (__prefix), "%s[%p]", ""_NMLOG_PREFIX_NAME"", (self)); \ + else \ + g_strlcpy (__prefix, _NMLOG_PREFIX_NAME, sizeof (__prefix)); \ + _nm_log ((level), (_NMLOG_DOMAIN), 0, \ + "%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + __prefix _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } G_STMT_END + +#define LOG_REQ_FMT "req[%p,%s,%s%s%s%s]" +#define LOG_REQ_ARG(req) (req), (req)->dbus_command, NM_PRINT_FMT_QUOTE_STRING ((req)->path), ((req)->cancellable ? "" : " (cancelled)") + G_DEFINE_TYPE (NMSecretAgent, nm_secret_agent, G_TYPE_OBJECT) #define NM_SECRET_AGENT_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), \ @@ -66,6 +86,7 @@ struct _NMSecretAgentCallId { NMSecretAgent *agent; GCancellable *cancellable; char *path; + const char *dbus_command; char *setting_name; gboolean is_get_secrets; NMSecretAgentCallback callback; @@ -75,7 +96,8 @@ struct _NMSecretAgentCallId { typedef struct _NMSecretAgentCallId Request; static Request * -request_new (NMSecretAgent *agent, +request_new (NMSecretAgent *self, + const char *dbus_command, /* this must be a static string. */ const char *path, const char *setting_name, NMSecretAgentCallback callback, @@ -84,18 +106,24 @@ request_new (NMSecretAgent *agent, Request *r; r = g_slice_new0 (Request); - r->agent = agent; + r->agent = self; r->path = g_strdup (path); r->setting_name = g_strdup (setting_name); + r->dbus_command = dbus_command, r->callback = callback; r->callback_data = callback_data; r->cancellable = g_cancellable_new (); + _LOGT ("request "LOG_REQ_FMT": created", LOG_REQ_ARG (r)); return r; } +#define request_new(self,dbus_command,path,setting_name,callback,callback_data) request_new(self,""dbus_command"",path,setting_name,callback,callback_data) static void request_free (Request *r) { + NMSecretAgent *self = r->agent; + + _LOGT ("request "LOG_REQ_FMT": destroyed", LOG_REQ_ARG (r)); g_free (r->path); g_free (r->setting_name); if (r->cancellable) @@ -123,6 +151,15 @@ request_check_return (Request *r) /*************************************************************/ +static char * +_create_description (const char *dbus_owner, const char *identifier, gulong uid) +{ + return g_strdup_printf ("%s/%s/%lu", + dbus_owner, + identifier, + uid); +} + const char * nm_secret_agent_get_description (NMSecretAgent *agent) { @@ -132,10 +169,9 @@ nm_secret_agent_get_description (NMSecretAgent *agent) priv = NM_SECRET_AGENT_GET_PRIVATE (agent); if (!priv->description) { - priv->description = g_strdup_printf ("%s/%s/%lu", - priv->dbus_owner, - priv->identifier, - nm_auth_subject_get_unix_process_uid (priv->subject)); + priv->description = _create_description (priv->dbus_owner, + priv->identifier, + nm_auth_subject_get_unix_process_uid (priv->subject)); } return priv->description; @@ -317,7 +353,7 @@ nm_secret_agent_get_secrets (NMSecretAgent *self, flags &= ~NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM; flags &= ~NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS; - r = request_new (self, nm_connection_get_path (connection), setting_name, callback, callback_data); + r = request_new (self, "GetSecrets", nm_connection_get_path (connection), setting_name, callback, callback_data); r->is_get_secrets = TRUE; g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_get_secrets (priv->proxy, @@ -464,7 +500,7 @@ nm_secret_agent_save_secrets (NMSecretAgent *self, /* Caller should have ensured that only agent-owned secrets exist in 'connection' */ dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_ALL); - r = request_new (self, cpath, NULL, callback, callback_data); + r = request_new (self, "SaveSecrets", cpath, NULL, callback, callback_data); g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_save_secrets (priv->proxy, dict, cpath, @@ -516,7 +552,7 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self, /* No secrets sent; agents must be smart enough to track secrets using the UUID or something */ dict = nm_connection_to_dbus (connection, NM_CONNECTION_SERIALIZE_NO_SECRETS); - r = request_new (self, cpath, NULL, callback, callback_data); + r = request_new (self, "DeleteSecrets", cpath, NULL, callback, callback_data); g_hash_table_add (priv->requests, r); nmdbus_secret_agent_call_delete_secrets (priv->proxy, dict, cpath, @@ -538,6 +574,8 @@ name_owner_changed_cb (GObject *proxy, char *owner; owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (proxy)); + _LOGT ("name-owner-changed: %s%s%s", + NM_PRINT_FMT_QUOTED (owner, "owner = \"", owner, "\"", "disconnected")); if (!owner) { g_signal_handlers_disconnect_by_func (priv->proxy, name_owner_changed_cb, self); g_clear_object (&priv->proxy); @@ -556,25 +594,40 @@ nm_secret_agent_new (GDBusMethodInvocation *context, { NMSecretAgent *self; NMSecretAgentPrivate *priv; + const char *dbus_owner; struct passwd *pw; GDBusProxy *proxy; char *owner_username = NULL; + char *description = NULL; + char buf_subject[64]; + gulong uid; g_return_val_if_fail (context != NULL, NULL); g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); g_return_val_if_fail (nm_auth_subject_is_unix_process (subject), NULL); g_return_val_if_fail (identifier != NULL, NULL); - pw = getpwuid (nm_auth_subject_get_unix_process_uid (subject)); + uid = nm_auth_subject_get_unix_process_uid (subject); + + pw = getpwuid (uid); if (pw && pw->pw_name && pw->pw_name[0]) owner_username = g_strdup (pw->pw_name); + dbus_owner = nm_auth_subject_get_unix_process_dbus_sender (subject); + self = (NMSecretAgent *) g_object_new (NM_TYPE_SECRET_AGENT, NULL); + priv = NM_SECRET_AGENT_GET_PRIVATE (self); + _LOGT ("constructed: %s, owner=%s%s%s (%s)", + (description = _create_description (dbus_owner, identifier, uid)), + NM_PRINT_FMT_QUOTE_STRING (owner_username), + nm_auth_subject_to_string (subject, buf_subject, sizeof (buf_subject))); + priv->identifier = g_strdup (identifier); priv->owner_username = owner_username; - priv->dbus_owner = g_strdup (nm_auth_subject_get_unix_process_dbus_sender (subject)); + priv->dbus_owner = g_strdup (dbus_owner); + priv->description = description; priv->capabilities = capabilities; priv->subject = g_object_ref (subject); @@ -624,7 +677,8 @@ dispose (GObject *object) static void finalize (GObject *object) { - NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (object); + NMSecretAgent *self = NM_SECRET_AGENT (object); + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); g_free (priv->description); g_free (priv->identifier); @@ -635,6 +689,8 @@ finalize (GObject *object) g_hash_table_destroy (priv->requests); G_OBJECT_CLASS (nm_secret_agent_parent_class)->finalize (object); + + _LOGT ("finalized"); } static void From 214faf4695563d5d601653251ae5f95c0220105c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 12:51:20 +0200 Subject: [PATCH 10/12] agent-manager: refactor logging in agent-manager --- src/settings/nm-agent-manager.c | 226 ++++++++++++++++++++------------ 1 file changed, 144 insertions(+), 82 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index baacc30a6d..3ed2e57916 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -40,6 +40,42 @@ #include "nmdbus-agent-manager.h" +NM_DEFINE_SINGLETON_INSTANCE (NMAgentManager); + +#define _NMLOG_PREFIX_NAME "agent-manager" +#define _NMLOG_DOMAIN LOGD_AGENTS +#define _NMLOG(level, agent, ...) \ + G_STMT_START { \ + if (nm_logging_enabled ((level), (_NMLOG_DOMAIN))) { \ + char __prefix1[32]; \ + char __prefix2[128]; \ + NMSecretAgent *__agent = (agent); \ + \ + if (!(self)) \ + g_snprintf (__prefix1, sizeof (__prefix1), "%s%s", ""_NMLOG_PREFIX_NAME"", "[]"); \ + else if ((self) != singleton_instance) \ + g_snprintf (__prefix1, sizeof (__prefix1), "%s[%p]", ""_NMLOG_PREFIX_NAME"", (self)); \ + else \ + g_strlcpy (__prefix1, _NMLOG_PREFIX_NAME, sizeof (__prefix1)); \ + if (__agent) { \ + g_snprintf (__prefix2, sizeof (__prefix2), \ + ": req[%p, %s]", \ + __agent, \ + nm_secret_agent_get_description (__agent)); \ + } else \ + __prefix2[0] = '\0'; \ + _nm_log ((level), (_NMLOG_DOMAIN), 0, \ + "%s%s: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + __prefix1, __prefix2 _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + } \ + } G_STMT_END + +#define LOG_REQ_FMT "[%p/%s%s%s]" +#define LOG_REQ_ARG(req) (req), NM_PRINT_FMT_QUOTE_STRING ((req)->detail) + +#define LOG_REQ2_FMT "[%p/%s%s%s/%s%s%s]" +#define LOG_REQ2_ARG(req) (req), NM_PRINT_FMT_QUOTE_STRING ((req)->parent.detail), NM_PRINT_FMT_QUOTE_STRING ((req)->setting_name) + G_DEFINE_TYPE (NMAgentManager, nm_agent_manager, NM_TYPE_EXPORTED_OBJECT) #define NM_AGENT_MANAGER_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), \ @@ -94,8 +130,7 @@ remove_agent (NMAgentManager *self, const char *owner) if (!agent) return FALSE; - nm_log_dbg (LOGD_AGENTS, "(%s) agent unregistered or disappeared", - nm_secret_agent_get_description (agent)); + _LOGD (agent, "agent unregistered or disappeared"); /* Remove this agent from any in-progress secrets requests */ g_hash_table_iter_init (&iter, priv->requests); @@ -221,8 +256,7 @@ agent_register_permissions_done (NMAuthChain *chain, sender = nm_secret_agent_get_dbus_owner (agent); g_hash_table_insert (priv->agents, g_strdup (sender), agent); - nm_log_dbg (LOGD_AGENTS, "(%s) agent registered", - nm_secret_agent_get_description (agent)); + _LOGD (agent, "agent registered"); g_dbus_method_invocation_return_value (context, NULL); /* Signal an agent was registered */ @@ -308,8 +342,7 @@ impl_agent_manager_register_with_capabilities (NMAgentManager *self, g_signal_connect (agent, NM_SECRET_AGENT_DISCONNECTED, G_CALLBACK (agent_disconnected_cb), self); - nm_log_dbg (LOGD_AGENTS, "(%s) requesting permissions", - nm_secret_agent_get_description (agent)); + _LOGD (agent, "requesting permissions"); /* Kick off permissions requests for this agent */ chain = nm_auth_chain_new_subject (subject, context, agent_register_permissions_done, self); @@ -388,6 +421,8 @@ typedef void (*RequestCancelFunc) (Request *req); /* Basic secrets request structure */ struct _Request { + NMAgentManager *self; + guint32 reqid; char *detail; char *verb; @@ -417,6 +452,7 @@ static guint32 next_req_id = 1; static Request * request_new (gsize struct_size, + NMAgentManager *self, const char *detail, const char *verb, NMAuthSubject *subject, @@ -430,6 +466,7 @@ request_new (gsize struct_size, Request *req; req = g_malloc0 (struct_size); + req->self = g_object_ref (self); req->reqid = next_req_id++; req->detail = g_strdup (detail); req->verb = g_strdup (verb); @@ -460,6 +497,9 @@ request_free (Request *req) g_free (req->detail); g_free (req->verb); g_slist_free_full (req->pending, g_object_unref); + + g_object_unref (req->self); + memset (req, 0, sizeof (Request)); g_free (req); } @@ -525,9 +565,13 @@ agent_compare_func (gconstpointer aa, gconstpointer bb, gpointer user_data) static void request_add_agent (Request *req, NMSecretAgent *agent) { + NMAgentManager *self; + g_return_if_fail (req != NULL); g_return_if_fail (agent != NULL); + self = req->self; + if (req->add_agent_callback && !req->add_agent_callback (req, agent)) return; @@ -538,18 +582,16 @@ request_add_agent (Request *req, NMSecretAgent *agent) agent_uid = nm_secret_agent_get_owner_uid (agent); subject_uid = nm_auth_subject_get_unix_process_uid (req->subject); if (agent_uid != subject_uid) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent ignored for secrets request %p/%s " - "(uid %ld not required %ld)", - nm_secret_agent_get_description (agent), - req, req->detail, - (long)agent_uid, (long)subject_uid); + _LOGD (agent, "agent ignored for secrets request "LOG_REQ_FMT" " + "(uid %ld not required %ld)", + LOG_REQ_ARG (req), + (long) agent_uid, (long) subject_uid); return; } } - nm_log_dbg (LOGD_AGENTS, "(%s) agent allowed for secrets request %p/%s", - nm_secret_agent_get_description (agent), - req, req->detail); + _LOGD (agent, "agent allowed for secrets request "LOG_REQ_FMT, + LOG_REQ_ARG (req)); /* Add this agent to the list, sorted appropriately */ req->pending = g_slist_insert_sorted_with_data (req->pending, @@ -573,8 +615,11 @@ request_add_agents (NMAgentManager *self, Request *req) static void request_next_agent (Request *req) { + NMAgentManager *self; GError *error = NULL; + self = req->self; + if (req->current) { if (req->current_call_id) nm_secret_agent_cancel_secrets (req->current, req->current_call_id); @@ -587,9 +632,8 @@ request_next_agent (Request *req) req->current = req->pending->data; req->pending = g_slist_remove (req->pending, req->current); - nm_log_dbg (LOGD_AGENTS, "(%s) agent %s secrets for request %p/%s", - nm_secret_agent_get_description (req->current), - req->verb, req, req->detail); + _LOGD (req->current, "agent %s secrets for request "LOG_REQ_FMT, + req->verb, LOG_REQ_ARG (req)); req->next_callback (req); } else { @@ -605,18 +649,22 @@ request_next_agent (Request *req) static void request_remove_agent (Request *req, NMSecretAgent *agent, GSList **pending_reqs) { + NMAgentManager *self; + g_return_if_fail (req != NULL); g_return_if_fail (agent != NULL); + self = req->self; + req->pending = g_slist_remove (req->pending, agent); if (agent == req->current) { - nm_log_dbg (LOGD_AGENTS, "(%s) current agent removed from secrets request %p/%s", - nm_secret_agent_get_description (agent), req, req->detail); + _LOGD (agent, "current agent removed from secrets request "LOG_REQ_FMT, + LOG_REQ_ARG (req)); *pending_reqs = g_slist_prepend (*pending_reqs, req); } else { - nm_log_dbg (LOGD_AGENTS, "(%s) agent removed from secrets request %p/%s", - nm_secret_agent_get_description (agent), req, req->detail); + _LOGD (agent, "agent removed from secrets request "LOG_REQ_FMT, + LOG_REQ_ARG (req)); } } @@ -673,16 +721,18 @@ connection_request_free (gpointer data) static gboolean connection_request_add_agent (Request *parent, NMSecretAgent *agent) { + NMAgentManager *self; ConnectionRequest *req = (ConnectionRequest *) parent; NMAuthSubject *subject = nm_secret_agent_get_subject(agent); + self = parent->self; + /* 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 (req->connection, subject, NULL)) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent ignored for secrets request %p/%s (not in ACL)", - nm_secret_agent_get_description (agent), - parent, parent->detail); + _LOGD (agent, "agent ignored for secrets request "LOG_REQ_FMT" (not in ACL)", + LOG_REQ_ARG (parent)); /* Connection not visible to this agent's user */ return FALSE; } @@ -691,7 +741,8 @@ connection_request_add_agent (Request *parent, NMSecretAgent *agent) } static ConnectionRequest * -connection_request_new_get (NMConnection *connection, +connection_request_new_get (NMAgentManager *self, + NMConnection *connection, NMAuthSubject *subject, GVariant *existing_secrets, const char *setting_name, @@ -710,6 +761,7 @@ connection_request_new_get (NMConnection *connection, ConnectionRequest *req; req = (ConnectionRequest *) request_new (sizeof (ConnectionRequest), + self, nm_connection_get_id (connection), verb, subject, @@ -735,7 +787,8 @@ connection_request_new_get (NMConnection *connection, } static ConnectionRequest * -connection_request_new_other (NMConnection *connection, +connection_request_new_other (NMAgentManager *self, + NMConnection *connection, NMAuthSubject *subject, const char *verb, RequestCompleteFunc complete_callback, @@ -745,6 +798,7 @@ connection_request_new_other (NMConnection *connection, ConnectionRequest *req; req = (ConnectionRequest *) request_new (sizeof (ConnectionRequest), + self, nm_connection_get_id (connection), verb, subject, @@ -766,6 +820,7 @@ get_done_cb (NMSecretAgent *agent, GError *error, gpointer user_data) { + NMAgentManager *self; Request *parent = user_data; ConnectionRequest *req = user_data; GVariant *setting_secrets; @@ -776,20 +831,20 @@ get_done_cb (NMSecretAgent *agent, g_return_if_fail (call_id == parent->current_call_id); g_return_if_fail (agent == parent->current); + self = parent->self; + parent->current_call_id = NULL; if (error) { if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - nm_log_dbg (LOGD_AGENTS, "(%s) get secrets request cancelled: %p/%s/%s", - nm_secret_agent_get_description (agent), - req, parent->detail, req->setting_name); + _LOGD (agent, "get secrets request cancelled: "LOG_REQ2_FMT, + LOG_REQ2_ARG (req)); return; } - nm_log_dbg (LOGD_AGENTS, "(%s) agent failed secrets request %p/%s/%s: %s", - nm_secret_agent_get_description (agent), - req, parent->detail, req->setting_name, - error->message); + _LOGD (agent, "agent failed secrets request "LOG_REQ2_FMT": %s", + LOG_REQ2_ARG (req), + error->message); if (g_error_matches (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_USER_CANCELED)) { error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, @@ -808,17 +863,15 @@ get_done_cb (NMSecretAgent *agent, /* Ensure the setting we wanted secrets for got returned and has something in it */ setting_secrets = g_variant_lookup_value (secrets, req->setting_name, NM_VARIANT_TYPE_SETTING); if (!setting_secrets || !g_variant_n_children (setting_secrets)) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent returned no secrets for request %p/%s/%s", - nm_secret_agent_get_description (agent), - req, parent->detail, req->setting_name); + _LOGD (agent, "agent returned no secrets for request "LOG_REQ2_FMT, + LOG_REQ2_ARG (req)); /* Try the next agent */ request_next_agent (parent); return; } - nm_log_dbg (LOGD_AGENTS, "(%s) agent returned secrets for request %p/%s/%s", - nm_secret_agent_get_description (agent), - req, parent->detail, req->setting_name); + _LOGD (agent, "agent returned secrets for request "LOG_REQ2_FMT, + LOG_REQ2_ARG (req)); /* Get the agent's username */ pw = getpwuid (nm_secret_agent_get_owner_uid (agent)); @@ -914,17 +967,19 @@ get_agent_modify_auth_cb (NMAuthChain *chain, GDBusMethodInvocation *context, gpointer user_data) { + NMAgentManager *self; Request *parent = user_data; ConnectionRequest *req = user_data; const char *perm; + self = parent->self; + req->chain = NULL; if (error) { - nm_log_dbg (LOGD_AGENTS, "(%s) agent %p/%s/%s MODIFY check error: (%d) %s", - nm_secret_agent_get_description (parent->current), - req, parent->detail, req->setting_name, - error->code, error->message ? error->message : "(unknown)"); + _LOGD (parent->current, "agent "LOG_REQ2_FMT" MODIFY check error: (%d) %s", + LOG_REQ2_ARG (req), + error->code, error->message ? error->message : "(unknown)"); /* Try the next agent */ request_next_agent (parent); } else { @@ -937,10 +992,9 @@ get_agent_modify_auth_cb (NMAuthChain *chain, if (nm_auth_chain_get_result (chain, perm) == NM_AUTH_CALL_RESULT_YES) req->current_has_modify = TRUE; - nm_log_dbg (LOGD_AGENTS, "(%s) agent %p/%s/%s MODIFY check result %s", - nm_secret_agent_get_description (parent->current), - req, parent->detail, req->setting_name, - req->current_has_modify ? "YES" : "NO"); + _LOGD (parent->current, "agent "LOG_REQ2_FMT" MODIFY check result %s", + LOG_REQ2_ARG (req), + req->current_has_modify ? "YES" : "NO"); get_agent_request_secrets (req, req->current_has_modify); } @@ -994,10 +1048,13 @@ has_system_secrets (NMConnection *connection) static void get_next_cb (Request *parent) { + NMAgentManager *self; ConnectionRequest *req = (ConnectionRequest *) parent; NMSettingConnection *s_con; const char *agent_dbus_owner, *perm; + self = parent->self; + req->current_has_modify = FALSE; agent_dbus_owner = nm_secret_agent_get_dbus_owner (parent->current); @@ -1010,8 +1067,8 @@ get_next_cb (Request *parent) */ if ( (req->flags != NM_SECRET_AGENT_GET_SECRETS_FLAG_NONE) && (req->existing_secrets || has_system_secrets (req->connection))) { - nm_log_dbg (LOGD_AGENTS, "(%p/%s/%s) request has system secrets; checking agent %s for MODIFY", - req, parent->detail, req->setting_name, agent_dbus_owner); + _LOGD (NULL, "("LOG_REQ2_FMT") request has system secrets; checking agent %s for MODIFY", + LOG_REQ2_ARG (req), agent_dbus_owner); req->chain = nm_auth_chain_new_subject (nm_secret_agent_get_subject (parent->current), NULL, @@ -1033,8 +1090,8 @@ get_next_cb (Request *parent) nm_auth_chain_add_call (req->chain, perm, TRUE); } else { - nm_log_dbg (LOGD_AGENTS, "(%p/%s/%s) requesting user-owned secrets from agent %s", - req, parent->detail, req->setting_name, agent_dbus_owner); + _LOGD (NULL, "("LOG_REQ2_FMT") requesting user-owned secrets from agent %s", + LOG_REQ2_ARG (req), agent_dbus_owner); get_agent_request_secrets (req, FALSE); } @@ -1043,10 +1100,13 @@ get_next_cb (Request *parent) static gboolean get_start (gpointer user_data) { + NMAgentManager *self; Request *parent = user_data; ConnectionRequest *req = user_data; GVariant *setting_secrets = NULL; + self = parent->self; + parent->idle_id = 0; /* Check if there are any existing secrets */ @@ -1073,14 +1133,14 @@ get_start (gpointer user_data) /* Do we have everything we need? */ if ( (req->flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_ONLY_SYSTEM) || ((nm_connection_need_secrets (tmp, NULL) == NULL) && (new_secrets == FALSE))) { - nm_log_dbg (LOGD_AGENTS, "(%p/%s/%s) system settings secrets sufficient", - req, parent->detail, req->setting_name); + _LOGD (NULL, "("LOG_REQ2_FMT") system settings secrets sufficient", + LOG_REQ2_ARG (req)); /* Got everything, we're done */ req_complete_success (parent, req->existing_secrets, NULL, NULL); } else { - nm_log_dbg (LOGD_AGENTS, "(%p/%s/%s) system settings secrets insufficient, asking agents", - req, parent->detail, req->setting_name); + _LOGD (NULL, "("LOG_REQ2_FMT") system settings secrets insufficient, asking agents", + LOG_REQ2_ARG (req)); /* We don't, so ask some agents for additional secrets */ if ( req->flags & NM_SECRET_AGENT_GET_SECRETS_FLAG_NO_ERRORS @@ -1179,7 +1239,8 @@ nm_agent_manager_get_secrets (NMAgentManager *self, * both returning NULL if they didn't hash anything. */ - req = connection_request_new_get (connection, + req = connection_request_new_get (self, + connection, subject, existing_secrets, setting_name, @@ -1224,36 +1285,35 @@ save_done_cb (NMSecretAgent *agent, GError *error, gpointer user_data) { - Request *parent = user_data; + NMAgentManager *self; ConnectionRequest *req = user_data; + Request *parent = (Request *) req; const char *agent_dbus_owner; g_return_if_fail (call_id == parent->current_call_id); g_return_if_fail (agent == parent->current); + self = parent->self; + parent->current_call_id = NULL; if (error) { if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - nm_log_dbg (LOGD_AGENTS, "(%s) save secrets request cancelled: %p/%s", - nm_secret_agent_get_description (agent), - req, parent->detail); + _LOGD (agent, "save secrets request cancelled: "LOG_REQ_FMT, + LOG_REQ_ARG (parent)); return; } - nm_log_dbg (LOGD_AGENTS, "(%s) agent failed save secrets request %p/%s: %s", - nm_secret_agent_get_description (agent), - req, parent->detail, - error->message); + _LOGD (agent, "agent failed save secrets request "LOG_REQ_FMT": %s", + LOG_REQ_ARG (parent), error->message); /* Try the next agent */ request_next_agent (parent); maybe_remove_agent_on_error (agent, error); return; } - nm_log_dbg (LOGD_AGENTS, "(%s) agent saved secrets for request %p/%s", - nm_secret_agent_get_description (agent), - req, parent->detail); + _LOGD (agent, "agent saved secrets for request "LOG_REQ_FMT, + LOG_REQ_ARG (parent)); agent_dbus_owner = nm_secret_agent_get_dbus_owner (agent); req_complete_success (parent, NULL, NULL, agent_dbus_owner); @@ -1303,7 +1363,8 @@ nm_agent_manager_save_secrets (NMAgentManager *self, nm_connection_get_path (connection), nm_connection_get_id (connection)); - req = connection_request_new_other (connection, + req = connection_request_new_other (self, + connection, subject, "saving", save_complete_cb, @@ -1327,26 +1388,28 @@ delete_done_cb (NMSecretAgent *agent, GError *error, gpointer user_data) { + NMAgentManager *self; Request *req = user_data; g_return_if_fail (call_id == req->current_call_id); g_return_if_fail (agent == req->current); + self = req->self; + req->current_call_id = NULL; if (error) { if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { - nm_log_dbg (LOGD_AGENTS, "(%s) delete secrets request cancelled: %p/%s", - nm_secret_agent_get_description (agent), req, req->detail); + _LOGD (agent, "delete secrets request cancelled: "LOG_REQ_FMT, + LOG_REQ_ARG (req)); return; } - nm_log_dbg (LOGD_AGENTS, "(%s) agent failed delete secrets request %p/%s: %s", - nm_secret_agent_get_description (agent), req, req->detail, - error->message); + _LOGD (agent, "agent failed delete secrets request "LOG_REQ_FMT": %s", + LOG_REQ_ARG (req), error->message); } else { - nm_log_dbg (LOGD_AGENTS, "(%s) agent deleted secrets for request %p/%s", - nm_secret_agent_get_description (agent), req, req->detail); + _LOGD (agent, "agent deleted secrets for request "LOG_REQ_FMT, + LOG_REQ_ARG (req)); } /* Tell the next agent to delete secrets */ @@ -1400,7 +1463,8 @@ nm_agent_manager_delete_secrets (NMAgentManager *self, nm_connection_get_id (connection)); subject = nm_auth_subject_new_internal (); - req = connection_request_new_other (connection, + req = connection_request_new_other (self, + connection, subject, "deleting", delete_complete_cb, @@ -1478,12 +1542,10 @@ agent_permissions_changed_done (NMAuthChain *chain, agent = nm_auth_chain_get_data (chain, "agent"); g_assert (agent); - if (error) { - nm_log_dbg (LOGD_AGENTS, "(%s) failed to request updated agent permissions", - nm_secret_agent_get_description (agent)); - } else { - nm_log_dbg (LOGD_AGENTS, "(%s) updated agent permissions", - nm_secret_agent_get_description (agent)); + if (error) + _LOGD (agent, "failed to request updated agent permissions"); + else { + _LOGD (agent, "updated agent permissions"); if (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED) == NM_AUTH_CALL_RESULT_YES) share_protected = TRUE; From 9b35d29d06e401deab2cd073ed3b93198beb3a6e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 21 Aug 2015 18:12:05 +0200 Subject: [PATCH 11/12] secret-agent: fix detection of disapearing secret-agent The signal "notify:g-name-owner" is only emitted for well-known names, but not for unique connection names (":1.x") such as the secret agent's connection. Also, it will not be emited for private connections. That meant that when the client disconnected without explicitly unregistering, the NMSecretAgent was not cleaned up and leaked indefinitely. As only one instance of secret agent with a certain 'identifier/uid' pair can register, this bug also prevented the client from registering until restart of NetworkManager. Fixes: df6706813a698e7a697739b0940bd8f528713aab --- src/nm-bus-manager.c | 37 +++++++++-- src/nm-bus-manager.h | 5 +- src/settings/nm-secret-agent.c | 117 +++++++++++++++++++++++++++------ 3 files changed, 132 insertions(+), 27 deletions(-) diff --git a/src/nm-bus-manager.c b/src/nm-bus-manager.c index 584db5e8a6..5f528a6f0e 100644 --- a/src/nm-bus-manager.c +++ b/src/nm-bus-manager.c @@ -847,10 +847,38 @@ nm_bus_manager_unregister_object (NMBusManager *self, gpointer object) g_dbus_interface_skeleton_unexport (G_DBUS_INTERFACE_SKELETON (object)); } +gboolean +nm_bus_manager_connection_is_private (NMBusManager *self, + GDBusConnection *connection) +{ + NMBusManagerPrivate *priv; + GSList *iter; + + g_return_val_if_fail (NM_IS_BUS_MANAGER (self), FALSE); + g_return_val_if_fail (G_IS_DBUS_CONNECTION (connection), FALSE); + + if (g_dbus_connection_get_unique_name (connection)) + return FALSE; + + /* Assert that we still track the private connection. The caller + * of nm_bus_manager_connection_is_private() want's to subscribe + * to NM_BUS_MANAGER_PRIVATE_CONNECTION_DISCONNECTED, thus the signal + * never comes if we don't track the connection. */ + priv = NM_BUS_MANAGER_GET_PRIVATE (self); + for (iter = priv->private_servers; iter; iter = g_slist_next (iter)) { + PrivateServer *s = iter->data; + + if (g_hash_table_contains (s->connections, + connection)) + return TRUE; + } + g_return_val_if_reached (TRUE); +} + /** * nm_bus_manager_new_proxy: * @self: the #NMBusManager - * @context: the method call context this proxy should be created + * @connection: the GDBusConnection for which this connection should be created * @proxy_type: the type of #GDBusProxy to create * @name: any name on the message bus * @path: name of the object instance to call methods on @@ -865,23 +893,20 @@ nm_bus_manager_unregister_object (NMBusManager *self, gpointer object) */ GDBusProxy * nm_bus_manager_new_proxy (NMBusManager *self, - GDBusMethodInvocation *context, + GDBusConnection *connection, GType proxy_type, const char *name, const char *path, const char *iface) { NMBusManagerPrivate *priv = NM_BUS_MANAGER_GET_PRIVATE (self); - GDBusConnection *connection; GSList *iter; const char *owner; GDBusProxy *proxy; GError *error = NULL; g_return_val_if_fail (g_type_is_a (proxy_type, G_TYPE_DBUS_PROXY), NULL); - - connection = g_dbus_method_invocation_get_connection (context); - g_assert (connection); + g_return_val_if_fail (G_IS_DBUS_CONNECTION (connection), NULL); /* Might be a private connection, for which @name is fake */ for (iter = priv->private_servers; iter; iter = g_slist_next (iter)) { diff --git a/src/nm-bus-manager.h b/src/nm-bus-manager.h index cf6a701f1a..86aa5df6eb 100644 --- a/src/nm-bus-manager.h +++ b/src/nm-bus-manager.h @@ -72,6 +72,9 @@ gboolean nm_bus_manager_get_caller_info (NMBusManager *self, gulong *out_uid, gulong *out_pid); +gboolean nm_bus_manager_connection_is_private (NMBusManager *self, + GDBusConnection *connection); + gboolean nm_bus_manager_get_unix_user (NMBusManager *self, const char *sender, gulong *out_uid); @@ -97,7 +100,7 @@ void nm_bus_manager_private_server_register (NMBusManager *self, const char *tag); GDBusProxy *nm_bus_manager_new_proxy (NMBusManager *self, - GDBusMethodInvocation *context, + GDBusConnection *connection, GType proxy_type, const char *name, const char *path, diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index ad97bc5222..d77f5e0e29 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -69,6 +69,10 @@ typedef struct { GSList *permissions; NMDBusSecretAgent *proxy; + NMBusManager *bus_mgr; + GDBusConnection *connection; + gboolean connection_is_private; + guint on_disconnected_id; GHashTable *requests; } NMSecretAgentPrivate; @@ -565,23 +569,67 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self, /*************************************************************/ static void -name_owner_changed_cb (GObject *proxy, - GParamSpec *pspec, - gpointer user_data) +_on_disconnected_cleanup (NMSecretAgentPrivate *priv) +{ + if (priv->on_disconnected_id) { + if (priv->connection_is_private) { + g_signal_handler_disconnect (priv->bus_mgr, + priv->on_disconnected_id); + } else { + g_dbus_connection_signal_unsubscribe (priv->connection, + priv->on_disconnected_id); + } + priv->on_disconnected_id = 0; + } + + g_clear_object (&priv->connection); + g_clear_object (&priv->proxy); + g_clear_object (&priv->bus_mgr); +} + +static void +_on_disconnected_private_connection (NMBusManager *mgr, + GDBusConnection *connection, + NMSecretAgent *self) +{ + NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); + + if (priv->connection != connection) + return; + + _LOGT ("private connection disconnected"); + + _on_disconnected_cleanup (priv); + g_signal_emit (self, signals[DISCONNECTED], 0); +} + +static void +_on_disconnected_name_owner_changed (GDBusConnection *connection, + const gchar *sender_name, + const gchar *object_path, + const gchar *interface_name, + const gchar *signal_name, + GVariant *parameters, + gpointer user_data) { NMSecretAgent *self = NM_SECRET_AGENT (user_data); NMSecretAgentPrivate *priv = NM_SECRET_AGENT_GET_PRIVATE (self); - char *owner; + const char *old_owner, *new_owner; - owner = g_dbus_proxy_get_name_owner (G_DBUS_PROXY (proxy)); - _LOGT ("name-owner-changed: %s%s%s", - NM_PRINT_FMT_QUOTED (owner, "owner = \"", owner, "\"", "disconnected")); - if (!owner) { - g_signal_handlers_disconnect_by_func (priv->proxy, name_owner_changed_cb, self); - g_clear_object (&priv->proxy); + g_variant_get (parameters, + "(&s&s&s)", + NULL, + &old_owner, + &new_owner); + + _LOGT ("name-owner-changed: %s%s%s => %s%s%s", + NM_PRINT_FMT_QUOTE_STRING (old_owner), + NM_PRINT_FMT_QUOTE_STRING (new_owner)); + + if (!*new_owner) { + _on_disconnected_cleanup (priv); g_signal_emit (self, signals[DISCONNECTED], 0); - } else - g_free (owner); + } } /*************************************************************/ @@ -601,12 +649,17 @@ nm_secret_agent_new (GDBusMethodInvocation *context, char *description = NULL; char buf_subject[64]; gulong uid; + GDBusConnection *connection; g_return_val_if_fail (context != NULL, NULL); g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); g_return_val_if_fail (nm_auth_subject_is_unix_process (subject), NULL); g_return_val_if_fail (identifier != NULL, NULL); + connection = g_dbus_method_invocation_get_connection (context); + + g_return_val_if_fail (G_IS_DBUS_CONNECTION (connection), NULL); + uid = nm_auth_subject_get_unix_process_uid (subject); pw = getpwuid (uid); @@ -619,10 +672,16 @@ nm_secret_agent_new (GDBusMethodInvocation *context, priv = NM_SECRET_AGENT_GET_PRIVATE (self); - _LOGT ("constructed: %s, owner=%s%s%s (%s)", + priv->bus_mgr = g_object_ref (nm_bus_manager_get ()); + priv->connection = g_object_ref (connection); + priv->connection_is_private = nm_bus_manager_connection_is_private (priv->bus_mgr, connection); + + _LOGT ("constructed: %s, owner=%s%s%s (%s), private-connection=%d, unique-name=%s%s%s", (description = _create_description (dbus_owner, identifier, uid)), NM_PRINT_FMT_QUOTE_STRING (owner_username), - nm_auth_subject_to_string (subject, buf_subject, sizeof (buf_subject))); + nm_auth_subject_to_string (subject, buf_subject, sizeof (buf_subject)), + priv->connection_is_private, + NM_PRINT_FMT_QUOTE_STRING (g_dbus_connection_get_unique_name (priv->connection))); priv->identifier = g_strdup (identifier); priv->owner_username = owner_username; @@ -631,18 +690,35 @@ nm_secret_agent_new (GDBusMethodInvocation *context, priv->capabilities = capabilities; priv->subject = g_object_ref (subject); - proxy = nm_bus_manager_new_proxy (nm_bus_manager_get (), - context, + proxy = nm_bus_manager_new_proxy (priv->bus_mgr, + priv->connection, NMDBUS_TYPE_SECRET_AGENT_PROXY, priv->dbus_owner, NM_DBUS_PATH_SECRET_AGENT, NM_DBUS_INTERFACE_SECRET_AGENT); g_assert (proxy); - g_signal_connect (proxy, "notify::g-name-owner", - G_CALLBACK (name_owner_changed_cb), - self); priv->proxy = NMDBUS_SECRET_AGENT (proxy); + /* we cannot subscribe to notify::g-name-owner because that doesn't work + * for unique names and it doesn't work for private connections. */ + if (priv->connection_is_private) { + priv->on_disconnected_id = g_signal_connect (priv->bus_mgr, + NM_BUS_MANAGER_PRIVATE_CONNECTION_DISCONNECTED, + G_CALLBACK (_on_disconnected_private_connection), + self); + } else { + priv->on_disconnected_id = g_dbus_connection_signal_subscribe (priv->connection, + "org.freedesktop.DBus", /* name */ + "org.freedesktop.DBus", /* interface */ + "NameOwnerChanged", /* signal name */ + "/org/freedesktop/DBus", /* path */ + priv->dbus_owner, /* arg0 */ + G_DBUS_SIGNAL_FLAGS_NONE, + _on_disconnected_name_owner_changed, + self, + NULL); + } + return self; } @@ -668,7 +744,8 @@ dispose (GObject *object) do_cancel_secrets (self, r, TRUE); } - g_clear_object (&priv->proxy); + _on_disconnected_cleanup (priv); + g_clear_object (&priv->subject); G_OBJECT_CLASS (nm_secret_agent_parent_class)->dispose (object); From 13386f760ac46564c363ea690d517d941fd1686e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Aug 2015 16:26:26 +0200 Subject: [PATCH 12/12] agent-manager: fix leak of secret-agent --- src/settings/nm-agent-manager.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 3ed2e57916..b3f2f675b0 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -500,6 +500,9 @@ request_free (Request *req) g_object_unref (req->self); + if (req->current) + g_object_unref (req->current); + memset (req, 0, sizeof (Request)); g_free (req); } @@ -656,15 +659,19 @@ request_remove_agent (Request *req, NMSecretAgent *agent, GSList **pending_reqs) self = req->self; - req->pending = g_slist_remove (req->pending, agent); - if (agent == req->current) { + nm_assert (!g_slist_find (req->pending, agent)); + _LOGD (agent, "current agent removed from secrets request "LOG_REQ_FMT, LOG_REQ_ARG (req)); *pending_reqs = g_slist_prepend (*pending_reqs, req); } else { + req->pending = g_slist_remove (req->pending, agent); + _LOGD (agent, "agent removed from secrets request "LOG_REQ_FMT, LOG_REQ_ARG (req)); + + g_object_unref (agent); } }