From 15a530d0bda7efa777d514338212b93e065f2129 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 10:17:51 +0200 Subject: [PATCH 01/32] core: drop unused function sleep_auth_done_cb() This function was left as a reminder now for 9 years. Get rid of it. Related: 8310593ce48a ('core: ignore authorization for sleep/wake requests (but restrict to root) (rh #638640)') --- src/nm-manager.c | 39 --------------------------------------- 1 file changed, 39 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 3622a0a5ad..f6d89419cf 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -5984,45 +5984,6 @@ _internal_sleep (NMManager *self, gboolean do_sleep) _notify (self, PROP_SLEEPING); } -#if 0 -static void -sleep_auth_done_cb (NMAuthChain *chain, - GError *error, - GDBusMethodInvocation *context, - gpointer user_data) -{ - NMManager *self = NM_MANAGER (user_data); - NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GError *ret_error; - NMAuthCallResult result; - gboolean do_sleep; - - priv->auth_chains = g_slist_remove (priv->auth_chains, chain); - - result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_SLEEP_WAKE); - if (error) { - _LOGD (LOGD_SUSPEND, "Sleep/wake request failed: %s", error->message); - ret_error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Sleep/wake request failed: %s", - error->message); - g_dbus_method_invocation_take_error (context, ret_error); - } else if (result != NM_AUTH_CALL_RESULT_YES) { - ret_error = g_error_new_literal (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Not authorized to sleep/wake"); - g_dbus_method_invocation_take_error (context, ret_error); - } else { - /* Auth success */ - do_sleep = GPOINTER_TO_UINT (nm_auth_chain_get_data (chain, "sleep")); - _internal_sleep (self, do_sleep); - g_dbus_method_invocation_return_value (context, NULL); - } - - nm_auth_chain_destroy (chain); -} -#endif - static void impl_manager_sleep (NMDBusObject *obj, const NMDBusInterfaceInfoExtended *interface_info, From aab06c7c4bb1f3cae201b23e9105498b648f4496 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 10:33:53 +0200 Subject: [PATCH 02/32] auth-chain: drop refcounting of NMAuthChain for simple flags NMAuthChain is not really ref-counted, there is no need for that additional complexity. But it is graceful towards calling nm_auth_chain_destroy() from inside the callback. The caller may do so. But we don't need a "ref_count" to track that. Two flags suffice: one to say whether destroy was called and one to indicate that we are in the process of finishing (to delay deallocating the NMAuthChain struct). We already had the "done" flag that we used to indicate that the chain is finished. So, we just need one more flag instead. --- src/nm-auth-utils.c | 88 ++++++++++++++++++++++++++------------------- 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index a0ad84c106..ec915e2bde 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -41,9 +41,8 @@ struct NMAuthChain { NMAuthChainResultFunc done_func; gpointer user_data; - guint32 refcount; - - bool done:1; + bool is_destroyed:1; + bool is_finishing:1; }; typedef struct { @@ -55,6 +54,10 @@ typedef struct { /*****************************************************************************/ +static void _auth_chain_destroy (NMAuthChain *self); + +/*****************************************************************************/ + static void _ASSERT_call (AuthCall *call) { @@ -212,20 +215,6 @@ nm_auth_chain_get_subject (NMAuthChain *self) /*****************************************************************************/ -static gboolean -auth_chain_finish (NMAuthChain *self) -{ - self->done = TRUE; - - /* Ensure we stay alive across the callback */ - nm_assert (self->refcount == 1); - self->refcount++; - self->done_func (self, NULL, self->context, self->user_data); - nm_assert (NM_IN_SET (self->refcount, 1, 2)); - nm_auth_chain_destroy (self); - return FALSE; -} - static void auth_call_complete (AuthCall *call) { @@ -235,13 +224,19 @@ auth_call_complete (AuthCall *call) self = call->chain; - nm_assert (!self->done); + nm_assert (!self->is_finishing); auth_call_free (call); if (c_list_is_empty (&self->auth_call_lst_head)) { /* we are on an idle-handler or a clean call-stack (non-reentrant). */ - auth_chain_finish (self); + nm_assert (!self->is_destroyed); + nm_assert (!self->is_finishing); + + self->is_finishing = TRUE; + self->done_func (self, NULL, self->context, self->user_data); + nm_assert (self->is_finishing); + _auth_chain_destroy (self); } } @@ -282,13 +277,17 @@ nm_auth_chain_add_call (NMAuthChain *self, g_return_if_fail (self); g_return_if_fail (self->subject); - g_return_if_fail (!self->done); + g_return_if_fail (!self->is_finishing); + g_return_if_fail (!self->is_destroyed); g_return_if_fail (permission && *permission); - g_return_if_fail (nm_auth_subject_is_unix_process (self->subject) || nm_auth_subject_is_internal (self->subject)); + nm_assert ( nm_auth_subject_is_unix_process (self->subject) + || nm_auth_subject_is_internal (self->subject)); - call = g_slice_new0 (AuthCall); - call->chain = self; - call->permission = g_strdup (permission); + call = g_slice_new (AuthCall); + *call = (AuthCall) { + .chain = self, + .permission = g_strdup (permission), + }; c_list_link_tail (&self->auth_call_lst_head, &call->auth_call_lst); call->call_id = nm_auth_manager_check_authorization (auth_manager, self->subject, @@ -310,6 +309,7 @@ nm_auth_chain_new_context (GDBusMethodInvocation *context, NMAuthChain *chain; g_return_val_if_fail (context, NULL); + nm_assert (done_func); subject = nm_auth_subject_new_unix_process_from_context (context); if (!subject) @@ -333,15 +333,18 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, NMAuthChain *self; g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), NULL); - nm_assert (nm_auth_subject_is_unix_process (subject) || nm_auth_subject_is_internal (subject)); + nm_assert ( nm_auth_subject_is_unix_process (subject) + || nm_auth_subject_is_internal (subject)); + nm_assert (done_func); - self = g_slice_new0 (NMAuthChain); - c_list_init (&self->auth_call_lst_head); - self->refcount = 1; - self->done_func = done_func; - self->user_data = user_data; - self->context = context ? g_object_ref (context) : NULL; - self->subject = g_object_ref (subject); + self = g_slice_new (NMAuthChain); + *self = (NMAuthChain) { + .done_func = done_func, + .user_data = user_data, + .context = nm_g_object_ref (context), + .subject = g_object_ref (subject), + .auth_call_lst_head = C_LIST_INIT (self->auth_call_lst_head), + }; return self; } @@ -361,13 +364,23 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, void nm_auth_chain_destroy (NMAuthChain *self) { - AuthCall *call; - g_return_if_fail (self); - g_return_if_fail (NM_IN_SET (self->refcount, 1, 2)); + g_return_if_fail (!self->is_destroyed); - if (--self->refcount > 0) + self->is_destroyed = TRUE; + + if (self->is_finishing) { + /* we are called from inside the callback. Keep the instance alive for the moment. */ return; + } + + _auth_chain_destroy (self); +} + +static void +_auth_chain_destroy (NMAuthChain *self) +{ + AuthCall *call; nm_clear_g_object (&self->subject); nm_clear_g_object (&self->context); @@ -395,7 +408,8 @@ nm_auth_is_subject_in_acl (NMConnection *connection, g_return_val_if_fail (connection, FALSE); g_return_val_if_fail (NM_IS_AUTH_SUBJECT (subject), FALSE); - g_return_val_if_fail (nm_auth_subject_is_internal (subject) || nm_auth_subject_is_unix_process (subject), FALSE); + nm_assert ( nm_auth_subject_is_internal (subject) + || nm_auth_subject_is_unix_process (subject)); if (nm_auth_subject_is_internal (subject)) return TRUE; From 89d0fdfa36f31360418cd91b59a3dac5e7371b7d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 10:08:09 +0200 Subject: [PATCH 03/32] core: don't call nm_auth_chain_destroy() from the callback NMAuthChain is not ref-counted. You may call nm_auth_chain_destroy() once before the callback gets invoked. This destroys the auth-chain instance right away. You may call nm_auth_chain_destroy() once from inside the callback. This basically has no effect but is allowed for convenince. All this does is remembering that destroy was called and asserts that destroy gets called at most once. After the callback returns, the auth-chain will always be destroyed. That means, generally there is no need to call nm_auth_chain_destroy() from inside the callback. Remove that code, and refactor some code to return early (where it makes sense). --- src/nm-auth-utils.c | 9 ++- src/nm-manager.c | 97 +++++++++++++-------------------- src/settings/nm-agent-manager.c | 18 ++---- src/settings/nm-settings.c | 18 +++--- 4 files changed, 57 insertions(+), 85 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index ec915e2bde..49a931f153 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -355,11 +355,14 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, * Destroys the auth-chain. By destroying the auth-chain, you also cancel * the receipt of the done-callback. IOW, the callback will not be invoked. * - * The only exception is, if may call nm_auth_chain_destroy() from inside - * the callback. In this case, @self stays alive until the callback returns. + * The only exception is, you may call nm_auth_chain_destroy() from inside + * the callback. In this case the call has no effect and @self stays alive + * until the callback returns. * * Note that you might only destroy an auth-chain exactly once, and never - * after the callback was handled. + * after the callback was handled. After the callback returns, the auth chain + * always gets automatically destroyed. So you only need to explicitly destroy + * it, if you want to abort it before the callback complets. */ void nm_auth_chain_destroy (NMAuthChain *self) diff --git a/src/nm-manager.c b/src/nm-manager.c index f6d89419cf..96b2d40e90 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1142,7 +1142,7 @@ _reload_auth_cb (NMAuthChain *chain, char s_buf[60]; NMConfigChangeFlags reload_type = NM_CONFIG_CHANGE_NONE; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auth_chains = g_slist_remove (priv->auth_chains, chain); flags = GPOINTER_TO_UINT (nm_auth_chain_get_data (chain, "flags")); @@ -1188,14 +1188,11 @@ _reload_auth_cb (NMAuthChain *chain, if (ret_error) { g_dbus_method_invocation_take_error (context, ret_error); - goto out; + return; } nm_config_reload (priv->config, reload_type, TRUE); g_dbus_method_invocation_return_value (context, NULL); - -out: - nm_auth_chain_destroy (chain); } static void @@ -2344,23 +2341,23 @@ device_auth_done_cb (NMAuthChain *chain, { NMManager *self = NM_MANAGER (user_data); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GError *error = NULL; + gs_free_error GError *error = NULL; NMAuthCallResult result; NMDevice *device; const char *permission; NMDeviceAuthRequestFunc callback; NMAuthSubject *subject; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auth_chains = g_slist_remove (priv->auth_chains, chain); permission = nm_auth_chain_get_data (chain, "requested-permission"); - g_assert (permission); + nm_assert (permission); callback = nm_auth_chain_get_data (chain, "callback"); - g_assert (callback); + nm_assert (callback); device = nm_auth_chain_get_data (chain, "device"); - g_assert (device); + nm_assert (NM_IS_DEVICE (device)); result = nm_auth_chain_get_result (chain, permission); subject = nm_auth_chain_get_subject (chain); @@ -2380,16 +2377,13 @@ device_auth_done_cb (NMAuthChain *chain, permission); } - g_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); + nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); callback (device, context, subject, error, nm_auth_chain_get_data (chain, "user-data")); - - g_clear_error (&error); - nm_auth_chain_destroy (chain); } static void @@ -5636,7 +5630,7 @@ deactivate_net_auth_done_cb (NMAuthChain *chain, NMActiveConnection *active; char *path; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auth_chains = g_slist_remove (priv->auth_chains, chain); @@ -5680,8 +5674,6 @@ deactivate_net_auth_done_cb (NMAuthChain *chain, g_dbus_method_invocation_take_error (context, error); else g_dbus_method_invocation_return_value (context, NULL); - - nm_auth_chain_destroy (chain); } static void @@ -6069,7 +6061,7 @@ enable_net_done_cb (NMAuthChain *chain, gboolean enable; NMAuthSubject *subject; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auth_chains = g_slist_remove (priv->auth_chains, chain); enable = GPOINTER_TO_UINT (nm_auth_chain_get_data (chain, "enable")); @@ -6086,21 +6078,18 @@ enable_net_done_cb (NMAuthChain *chain, ret_error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to enable/disable networking"); - } else { - /* Auth success */ - _internal_enable (self, enable); - g_dbus_method_invocation_return_value (context, NULL); - nm_audit_log_control_op (NM_AUDIT_OP_NET_CONTROL, enable ? "on" : "off", TRUE, - subject, NULL); } - if (ret_error) { nm_audit_log_control_op (NM_AUDIT_OP_NET_CONTROL, enable ? "on" : "off", FALSE, subject, ret_error->message); g_dbus_method_invocation_take_error (context, ret_error); + return; } - nm_auth_chain_destroy (chain); + _internal_enable (self, enable); + g_dbus_method_invocation_return_value (context, NULL); + nm_audit_log_control_op (NM_AUDIT_OP_NET_CONTROL, enable ? "on" : "off", TRUE, + subject, NULL); } static void @@ -6174,7 +6163,7 @@ get_permissions_done_cb (NMAuthChain *chain, GError *ret_error; GVariantBuilder results; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auth_chains = g_slist_remove (priv->auth_chains, chain); if (error) { @@ -6184,31 +6173,30 @@ get_permissions_done_cb (NMAuthChain *chain, "Permissions request failed: %s", error->message); g_dbus_method_invocation_take_error (context, ret_error); - } else { - g_variant_builder_init (&results, G_VARIANT_TYPE ("a{ss}")); - - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_NETWORK); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SLEEP_WAKE); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_WIFI); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_WWAN); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_WIMAX); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_NETWORK_CONTROL); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_SYSTEM); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_OWN); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_HOSTNAME); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_GLOBAL_DNS); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_RELOAD); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_CHECKPOINT_ROLLBACK); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_STATISTICS); - get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_CONNECTIVITY_CHECK); - - g_dbus_method_invocation_return_value (context, - g_variant_new ("(a{ss})", &results)); + return; } - nm_auth_chain_destroy (chain); + g_variant_builder_init (&results, G_VARIANT_TYPE ("a{ss}")); + + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_NETWORK); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SLEEP_WAKE); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_WIFI); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_WWAN); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_WIMAX); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_NETWORK_CONTROL); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_SYSTEM); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_OWN); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_HOSTNAME); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_SETTINGS_MODIFY_GLOBAL_DNS); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_RELOAD); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_CHECKPOINT_ROLLBACK); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_STATISTICS); + get_perm_add_result (self, chain, &results, NM_AUTH_PERMISSION_ENABLE_DISABLE_CONNECTIVITY_CHECK); + + g_dbus_method_invocation_return_value (context, + g_variant_new ("(a{ss})", &results)); } static void @@ -6400,10 +6388,9 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to recheck connectivity"); } - if (error) { g_dbus_method_invocation_take_error (context, error); - goto out; + return; } data = g_slice_new (ConnectivityCheckData); @@ -6434,9 +6421,6 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, data); /* @data got destroyed. */ } - -out: - nm_auth_chain_destroy (chain); } static void @@ -6875,7 +6859,6 @@ out: g_dbus_method_invocation_return_dbus_error (invocation, error_name, error_message); else g_dbus_method_invocation_return_value (invocation, NULL); - nm_auth_chain_destroy (chain); } void @@ -7009,8 +6992,6 @@ checkpoint_auth_done_cb (NMAuthChain *chain, g_dbus_method_invocation_take_error (context, error); else g_dbus_method_invocation_return_value (context, variant); - - nm_auth_chain_destroy (chain); } static void diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 814dee85a6..f5acbb1a42 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -323,7 +323,7 @@ agent_register_permissions_done (NMAuthChain *chain, NMAuthCallResult result; CList *iter; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->chains = g_slist_remove (priv->chains, chain); @@ -358,8 +358,6 @@ agent_register_permissions_done (NMAuthChain *chain, c_list_for_each (iter, &priv->requests) request_add_agent (c_list_entry (iter, Request, lst_request), agent); } - - nm_auth_chain_destroy (chain); } static NMSecretAgent * @@ -537,8 +535,7 @@ request_free (Request *req) case REQUEST_TYPE_CON_DEL: g_object_unref (req->con.connection); g_free (req->con.path); - if (req->con.chain) - nm_auth_chain_destroy (req->con.chain); + nm_clear_pointer (&req->con.chain, nm_auth_chain_destroy); if (req->request_type == REQUEST_TYPE_CON_GET) { g_free (req->con.get.setting_name); g_strfreev (req->con.get.hints); @@ -815,11 +812,8 @@ request_remove_agent (Request *req, NMSecretAgent *agent) case REQUEST_TYPE_CON_GET: case REQUEST_TYPE_CON_SAVE: case REQUEST_TYPE_CON_DEL: - if (req->con.chain) { - /* This cancels the pending authorization requests. */ - nm_auth_chain_destroy (req->con.chain); - req->con.chain = NULL; - } + /* This cancels the pending authorization requests. */ + nm_clear_pointer (&req->con.chain, nm_auth_chain_destroy); break; default: g_assert_not_reached (); @@ -1053,8 +1047,6 @@ _con_get_request_start_validated (NMAuthChain *chain, _con_get_request_start_proceed (req, req->con.current_has_modify); } - - nm_auth_chain_destroy (chain); } static void @@ -1505,8 +1497,6 @@ agent_permissions_changed_done (NMAuthChain *chain, nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, share_protected); nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, share_open); - - nm_auth_chain_destroy (chain); } static void diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index b49d5d5aee..7e92e4f9ca 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1141,7 +1141,7 @@ pk_add_cb (NMAuthChain *chain, NMSettings *self = NM_SETTINGS (user_data); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); NMAuthCallResult result; - GError *error = NULL; + gs_free_error GError *error = NULL; NMConnection *connection = NULL; gs_unref_object NMSettingsConnection *added = NULL; NMSettingsAddCallback callback; @@ -1150,12 +1150,13 @@ pk_add_cb (NMAuthChain *chain, const char *perm; gboolean save_to_disk; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auths = g_slist_remove (priv->auths, chain); perm = nm_auth_chain_get_data (chain, "perm"); - g_assert (perm); + nm_assert (perm); + result = nm_auth_chain_get_result (chain, perm); if (chain_error) { @@ -1189,11 +1190,10 @@ pk_add_cb (NMAuthChain *chain, callback (self, added, error, context, subject, callback_data); /* Send agent-owned secrets to the agents */ - if (!error && added && nm_settings_has_connection (self, added)) + if ( !error + && added + && nm_settings_has_connection (self, added)) send_agent_owned_secrets (self, added, subject); - - g_clear_error (&error); - nm_auth_chain_destroy (chain); } /* FIXME: remove if/when kernel supports adhoc wpa */ @@ -1513,7 +1513,7 @@ pk_hostname_cb (NMAuthChain *chain, GError *error = NULL; const char *hostname; - g_assert (context); + nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auths = g_slist_remove (priv->auths, chain); @@ -1543,8 +1543,6 @@ pk_hostname_cb (NMAuthChain *chain, g_dbus_method_invocation_take_error (context, error); else g_dbus_method_invocation_return_value (context, NULL); - - nm_auth_chain_destroy (chain); } static void From 4d79e3276b2ff67c729a4fd3ca611c69064b865d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 11:52:54 +0200 Subject: [PATCH 04/32] auth-chain: embed copy of permission string in AuthCall Since the allocated AuthCall struct is small, we can just copy the permission string inside the struct. No need to strdup() the string separately. --- src/nm-auth-utils.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 49a931f153..808dfca23e 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -49,7 +49,7 @@ typedef struct { CList auth_call_lst; NMAuthChain *chain; NMAuthManagerCallId *call_id; - char *permission; + char permission[]; } AuthCall; /*****************************************************************************/ @@ -74,7 +74,6 @@ auth_call_free (AuthCall *call) if (call->call_id) nm_auth_manager_check_authorization_cancel (call->call_id); c_list_unlink_stale (&call->auth_call_lst); - g_free (call->permission); g_slice_free (AuthCall, call); } @@ -274,6 +273,7 @@ nm_auth_chain_add_call (NMAuthChain *self, { AuthCall *call; NMAuthManager *auth_manager = nm_auth_manager_get (); + gsize l_p_1; g_return_if_fail (self); g_return_if_fail (self->subject); @@ -283,11 +283,11 @@ nm_auth_chain_add_call (NMAuthChain *self, nm_assert ( nm_auth_subject_is_unix_process (self->subject) || nm_auth_subject_is_internal (self->subject)); - call = g_slice_new (AuthCall); - *call = (AuthCall) { - .chain = self, - .permission = g_strdup (permission), - }; + l_p_1 = strlen (permission) + 1; + call = g_malloc (sizeof (AuthCall) + l_p_1); + call->chain = self; + call->call_id = NULL; + memcpy (call->permission, permission, l_p_1); c_list_link_tail (&self->auth_call_lst_head, &call->auth_call_lst); call->call_id = nm_auth_manager_check_authorization (auth_manager, self->subject, From 6085ded6f409608420156934f8ddc301ed81c154 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 11:37:49 +0200 Subject: [PATCH 05/32] auth-chain: cleanup chain-data in NMAuthChain - consistently name the ChainData variable "chain_data" - return the ChainData element from _get_data(). This way it also can be used by nm_auth_chain_steal_data(), which needs the ChainData element. --- src/nm-auth-utils.c | 86 ++++++++++++++++++++++----------------------- 1 file changed, 43 insertions(+), 43 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 808dfca23e..6c1998d436 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -80,10 +80,7 @@ auth_call_free (AuthCall *call) /*****************************************************************************/ typedef struct { - - /* must be the first field. */ const char *tag; - gpointer data; GDestroyNotify destroy; char tag_data[]; @@ -92,45 +89,47 @@ typedef struct { static ChainData * chain_data_new (const char *tag, gpointer data, GDestroyNotify destroy) { - ChainData *tmp; - gsize l = strlen (tag); + ChainData *chain_data; + gsize l_p_1; - tmp = g_malloc (sizeof (ChainData) + l + 1); - tmp->tag = &tmp->tag_data[0]; - tmp->data = data; - tmp->destroy = destroy; - memcpy (&tmp->tag_data[0], tag, l + 1); - return tmp; + nm_assert (tag); + nm_assert (data); + + l_p_1 = strlen (tag) + 1; + chain_data = g_malloc (sizeof (ChainData) + l_p_1); + chain_data->tag = &chain_data->tag_data[0]; + chain_data->data = data; + chain_data->destroy = destroy; + memcpy (&chain_data->tag_data[0], tag, l_p_1); + return chain_data; } static void -chain_data_free (gpointer data) +chain_data_free (ChainData *chain_data) { - ChainData *tmp = data; - - if (tmp->destroy) - tmp->destroy (tmp->data); - g_free (tmp); + if (chain_data->destroy) + chain_data->destroy (chain_data->data); + g_free (chain_data); } -static gpointer +static ChainData * _get_data (NMAuthChain *self, const char *tag) { - ChainData *tmp; - if (!self->data_hash) return NULL; - tmp = g_hash_table_lookup (self->data_hash, &tag); - return tmp ? tmp->data : NULL; + return g_hash_table_lookup (self->data_hash, &tag); } gpointer nm_auth_chain_get_data (NMAuthChain *self, const char *tag) { + ChainData *chain_data; + g_return_val_if_fail (self, NULL); g_return_val_if_fail (tag, NULL); - return _get_data (self, tag); + chain_data = _get_data (self, tag); + return chain_data ? chain_data->data : NULL; } /** @@ -147,24 +146,21 @@ nm_auth_chain_get_data (NMAuthChain *self, const char *tag) gpointer nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) { - ChainData *tmp; - gpointer value = NULL; + ChainData *chain_data; + gpointer value; g_return_val_if_fail (self, NULL); g_return_val_if_fail (tag, NULL); - if (!self->data_hash) + chain_data = _get_data (self, tag); + if (!chain_data) return NULL; - tmp = g_hash_table_lookup (self->data_hash, &tag); - if (!tmp) - return NULL; - - value = tmp->data; + value = chain_data->data; /* Make sure the destroy handler isn't called when freeing */ - tmp->destroy = NULL; - g_hash_table_remove (self->data_hash, tmp); + chain_data->destroy = NULL; + g_hash_table_remove (self->data_hash, chain_data); return value; } @@ -180,14 +176,16 @@ nm_auth_chain_set_data (NMAuthChain *self, if (data == NULL) { if (self->data_hash) g_hash_table_remove (self->data_hash, &tag); - } else { - if (!self->data_hash) { - self->data_hash = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, - NULL, chain_data_free); - } - g_hash_table_add (self->data_hash, - chain_data_new (tag, data, data_destroy)); + return; } + + if (!self->data_hash) { + G_STATIC_ASSERT (G_STRUCT_OFFSET (ChainData, tag) == 0); + self->data_hash = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, + NULL, (GDestroyNotify) chain_data_free); + } + g_hash_table_add (self->data_hash, + chain_data_new (tag, data, data_destroy)); } /*****************************************************************************/ @@ -195,13 +193,15 @@ nm_auth_chain_set_data (NMAuthChain *self, NMAuthCallResult nm_auth_chain_get_result (NMAuthChain *self, const char *permission) { - gpointer data; + ChainData *chain_data; g_return_val_if_fail (self, NM_AUTH_CALL_RESULT_UNKNOWN); g_return_val_if_fail (permission, NM_AUTH_CALL_RESULT_UNKNOWN); - data = _get_data (self, permission); - return data ? GPOINTER_TO_UINT (data) : NM_AUTH_CALL_RESULT_UNKNOWN; + chain_data = _get_data (self, permission); + return chain_data + ? (NMAuthCallResult) GPOINTER_TO_UINT (chain_data->data) + : NM_AUTH_CALL_RESULT_UNKNOWN; } NMAuthSubject * From f8de94736e4e14d6298262ea55f4d5a4622c0b2c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 11:21:22 +0200 Subject: [PATCH 06/32] auth-chain: use linked list instead of hash table to track user data of NMAuthChain The number of user-datas attached to the NMAuthChain is generally fixed and small. For example, in current code impl_manager_get_permissions() will be the instance that ends up with the largest number of data items. It performs zero calls of nm_auth_chain_set_data() but 16 times nm_auth_chain_add_call(). So currently the maximum number is 16. With such a fixed, small number of elements it is expected to be more efficient to just track the elements in a CList instead of a GHashTable. --- src/nm-auth-utils.c | 61 +++++++++++++++++++++++++++++++-------------- 1 file changed, 42 insertions(+), 19 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 6c1998d436..6f898a87e7 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -31,7 +31,7 @@ /*****************************************************************************/ struct NMAuthChain { - GHashTable *data_hash; + CList data_lst_head; CList auth_call_lst_head; @@ -80,14 +80,16 @@ auth_call_free (AuthCall *call) /*****************************************************************************/ typedef struct { - const char *tag; + CList data_lst; gpointer data; GDestroyNotify destroy; - char tag_data[]; + char tag[]; } ChainData; static ChainData * -chain_data_new (const char *tag, gpointer data, GDestroyNotify destroy) +chain_data_new_stale (const char *tag, + gpointer data, + GDestroyNotify destroy) { ChainData *chain_data; gsize l_p_1; @@ -97,16 +99,16 @@ chain_data_new (const char *tag, gpointer data, GDestroyNotify destroy) l_p_1 = strlen (tag) + 1; chain_data = g_malloc (sizeof (ChainData) + l_p_1); - chain_data->tag = &chain_data->tag_data[0]; chain_data->data = data; chain_data->destroy = destroy; - memcpy (&chain_data->tag_data[0], tag, l_p_1); + memcpy (&chain_data->tag[0], tag, l_p_1); return chain_data; } static void chain_data_free (ChainData *chain_data) { + c_list_unlink_stale (&chain_data->data_lst); if (chain_data->destroy) chain_data->destroy (chain_data->data); g_free (chain_data); @@ -115,9 +117,13 @@ chain_data_free (ChainData *chain_data) static ChainData * _get_data (NMAuthChain *self, const char *tag) { - if (!self->data_hash) - return NULL; - return g_hash_table_lookup (self->data_hash, &tag); + ChainData *chain_data; + + c_list_for_each_entry (chain_data, &self->data_lst_head, data_lst) { + if (nm_streq (chain_data->tag, tag)) + return chain_data; + } + return NULL; } gpointer @@ -160,7 +166,7 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) /* Make sure the destroy handler isn't called when freeing */ chain_data->destroy = NULL; - g_hash_table_remove (self->data_hash, chain_data); + chain_data_free (chain_data); return value; } @@ -170,22 +176,36 @@ nm_auth_chain_set_data (NMAuthChain *self, gpointer data, GDestroyNotify data_destroy) { + ChainData *chain_data; + g_return_if_fail (self); g_return_if_fail (tag); + /* we should not track a large number of elements via a linked list. If this becomes + * necessary, revert the code to use GHashTable again. */ + nm_assert (c_list_length (&self->data_lst_head) < 25); + + chain_data = _get_data (self, tag); + if (data == NULL) { - if (self->data_hash) - g_hash_table_remove (self->data_hash, &tag); + if (chain_data) + chain_data_free (chain_data); return; } - if (!self->data_hash) { - G_STATIC_ASSERT (G_STRUCT_OFFSET (ChainData, tag) == 0); - self->data_hash = g_hash_table_new_full (nm_pstr_hash, nm_pstr_equal, - NULL, (GDestroyNotify) chain_data_free); + if (chain_data) { + gpointer old_data = chain_data->data; + GDestroyNotify old_destroy = chain_data->destroy; + + chain_data->data = data; + chain_data->destroy = data_destroy; + if (old_destroy) + old_destroy (old_data); + return; } - g_hash_table_add (self->data_hash, - chain_data_new (tag, data, data_destroy)); + + chain_data = chain_data_new_stale (tag, data, data_destroy); + c_list_link_front (&self->data_lst_head, &chain_data->data_lst); } /*****************************************************************************/ @@ -343,6 +363,7 @@ nm_auth_chain_new_subject (NMAuthSubject *subject, .user_data = user_data, .context = nm_g_object_ref (context), .subject = g_object_ref (subject), + .data_lst_head = C_LIST_INIT (self->data_lst_head), .auth_call_lst_head = C_LIST_INIT (self->auth_call_lst_head), }; return self; @@ -384,6 +405,7 @@ static void _auth_chain_destroy (NMAuthChain *self) { AuthCall *call; + ChainData *chain_data; nm_clear_g_object (&self->subject); nm_clear_g_object (&self->context); @@ -391,7 +413,8 @@ _auth_chain_destroy (NMAuthChain *self) while ((call = c_list_first_entry (&self->auth_call_lst_head, AuthCall, auth_call_lst))) auth_call_free (call); - nm_clear_pointer (&self->data_hash, g_hash_table_destroy); + while ((chain_data = c_list_first_entry (&self->data_lst_head, ChainData, data_lst))) + chain_data_free (chain_data); g_slice_free (NMAuthChain, self); } From d460ec8e674107b2245a47cfe721fa84399a9bb9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 09:37:54 +0200 Subject: [PATCH 07/32] core: remove unused error argument from NMAuthChainResultFunc NMAuthChain usually requests several permissions at once. Hence, an error argument in the overall callback does not make sense, because you wouldn't know which request failed. If at all, it could only mean that the overall request failed (like an D-Bus failure communicating to D-Bus *for all permisssions*), but we don't need to handle that specially. In fact, we don't really care why permission was not granted, whether it's due to an error or legitimate reasons. The error in the callback was always set to %NULL. Remove it. --- src/nm-auth-utils.c | 2 +- src/nm-auth-utils.h | 1 - src/nm-manager.c | 76 ++++---------------------- src/settings/nm-agent-manager.c | 94 ++++++++++++--------------------- src/settings/nm-settings.c | 16 +----- 5 files changed, 48 insertions(+), 141 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 6f898a87e7..ef61a92050 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -253,7 +253,7 @@ auth_call_complete (AuthCall *call) nm_assert (!self->is_finishing); self->is_finishing = TRUE; - self->done_func (self, NULL, self->context, self->user_data); + self->done_func (self, self->context, self->user_data); nm_assert (self->is_finishing); _auth_chain_destroy (self); } diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 5f9823b695..58180bc4b1 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -28,7 +28,6 @@ typedef struct NMAuthChain NMAuthChain; typedef void (*NMAuthChainResultFunc) (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data); diff --git a/src/nm-manager.c b/src/nm-manager.c index 96b2d40e90..b35761b2b3 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1129,7 +1129,6 @@ _config_changed_cb (NMConfig *config, NMConfigData *config_data, NMConfigChangeF static void _reload_auth_cb (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data) { @@ -1150,13 +1149,7 @@ _reload_auth_cb (NMAuthChain *chain, subject = nm_auth_chain_get_subject (chain); result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_RELOAD); - if (error) { - _LOGD (LOGD_CORE, "Reload request failed: %s", error->message); - ret_error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Reload request failed: %s", - error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { ret_error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to reload configuration"); @@ -2335,7 +2328,6 @@ nm_manager_rfkill_update (NMManager *self, RfKillType rtype) static void device_auth_done_cb (NMAuthChain *chain, - GError *auth_error, GDBusMethodInvocation *context, gpointer user_data) { @@ -2362,14 +2354,7 @@ device_auth_done_cb (NMAuthChain *chain, result = nm_auth_chain_get_result (chain, permission); subject = nm_auth_chain_get_subject (chain); - if (auth_error) { - /* translate the auth error into a manager permission denied error */ - _LOGD (LOGD_CORE, "%s request failed: %s", permission, auth_error->message); - error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "%s request failed: %s", - permission, auth_error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { _LOGD (LOGD_CORE, "%s request failed: not authorized", permission); error = g_error_new (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, @@ -5619,7 +5604,6 @@ nm_manager_deactivate_connection (NMManager *manager, static void deactivate_net_auth_done_cb (NMAuthChain *chain, - GError *auth_error, GDBusMethodInvocation *context, gpointer user_data) { @@ -5638,13 +5622,7 @@ deactivate_net_auth_done_cb (NMAuthChain *chain, result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_NETWORK_CONTROL); active = active_connection_get_by_path (self, path); - if (auth_error) { - _LOGD (LOGD_CORE, "Disconnect request failed: %s", auth_error->message); - error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Deactivate request failed: %s", - auth_error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to deactivate connections"); @@ -6050,13 +6028,11 @@ _internal_enable (NMManager *self, gboolean enable) static void enable_net_done_cb (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data) { NMManager *self = NM_MANAGER (user_data); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GError *ret_error = NULL; NMAuthCallResult result; gboolean enable; NMAuthSubject *subject; @@ -6068,18 +6044,12 @@ enable_net_done_cb (NMAuthChain *chain, subject = nm_auth_chain_get_subject (chain); result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_ENABLE_DISABLE_NETWORK); - if (error) { - _LOGD (LOGD_CORE, "Enable request failed: %s", error->message); - ret_error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Enable request failed: %s", - error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { + GError *ret_error; + ret_error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to enable/disable networking"); - } - if (ret_error) { nm_audit_log_control_op (NM_AUDIT_OP_NET_CONTROL, enable ? "on" : "off", FALSE, subject, ret_error->message); g_dbus_method_invocation_take_error (context, ret_error); @@ -6154,27 +6124,16 @@ get_perm_add_result (NMManager *self, NMAuthChain *chain, GVariantBuilder *resul static void get_permissions_done_cb (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data) { NMManager *self = NM_MANAGER (user_data); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - GError *ret_error; GVariantBuilder results; nm_assert (G_IS_DBUS_METHOD_INVOCATION (context)); priv->auth_chains = g_slist_remove (priv->auth_chains, chain); - if (error) { - _LOGD (LOGD_CORE, "Permissions request failed: %s", error->message); - ret_error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Permissions request failed: %s", - error->message); - g_dbus_method_invocation_take_error (context, ret_error); - return; - } g_variant_builder_init (&results, G_VARIANT_TYPE ("a{ss}")); @@ -6362,7 +6321,6 @@ device_connectivity_done (NMDevice *device, static void check_connectivity_auth_done_cb (NMAuthChain *chain, - GError *auth_error, GDBusMethodInvocation *context, gpointer user_data) { @@ -6377,13 +6335,7 @@ check_connectivity_auth_done_cb (NMAuthChain *chain, result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_NETWORK_CONTROL); - if (auth_error) { - _LOGD (LOGD_CORE, "CheckConnectivity request failed: %s", auth_error->message); - error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "Connectivity check request failed: %s", - auth_error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to recheck connectivity"); @@ -6792,7 +6744,6 @@ typedef struct { static void _dbus_set_property_auth_cb (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *invocation, gpointer user_data) { @@ -6815,10 +6766,9 @@ _dbus_set_property_auth_cb (NMAuthChain *chain, priv->auth_chains = g_slist_remove (priv->auth_chains, chain); result = nm_auth_chain_get_result (chain, property_info->writable.permission); - if ( error - || result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { error_name = NM_PERM_DENIED_ERROR; - error_message = error ? error->message : "Not authorized to perform this operation"; + error_message = "Not authorized to perform this operation"; goto out; } @@ -6923,7 +6873,6 @@ _checkpoint_mgr_get (NMManager *self, gboolean create_as_needed) static void checkpoint_auth_done_cb (NMAuthChain *chain, - GError *auth_error, GDBusMethodInvocation *context, gpointer user_data) { @@ -6947,12 +6896,7 @@ checkpoint_auth_done_cb (NMAuthChain *chain, NM_AUDIT_OP_CHECKPOINT_ADJUST_ROLLBACK_TIMEOUT)) arg = checkpoint_path = nm_auth_chain_get_data (chain, "checkpoint_path"); - if (auth_error) { - error = g_error_new (NM_MANAGER_ERROR, - NM_MANAGER_ERROR_PERMISSION_DENIED, - "checkpoint check request failed: %s", - auth_error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { error = g_error_new_literal (NM_MANAGER_ERROR, NM_MANAGER_ERROR_PERMISSION_DENIED, "Not authorized to checkpoint/rollback"); diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index f5acbb1a42..73f59c0b3c 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -311,7 +311,6 @@ validate_identifier (const char *identifier, GError **error) static void agent_register_permissions_done (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data) { @@ -319,7 +318,6 @@ agent_register_permissions_done (NMAuthChain *chain, NMAgentManagerPrivate *priv = NM_AGENT_MANAGER_GET_PRIVATE (self); NMSecretAgent *agent; const char *sender; - GError *local = NULL; NMAuthCallResult result; CList *iter; @@ -327,37 +325,29 @@ agent_register_permissions_done (NMAuthChain *chain, priv->chains = g_slist_remove (priv->chains, chain); - if (error) { - local = g_error_new (NM_AGENT_MANAGER_ERROR, - NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, - "Failed to request agent permissions: %s", - error->message); - g_dbus_method_invocation_take_error (context, local); - } else { - agent = nm_auth_chain_steal_data (chain, "agent"); - g_assert (agent); + agent = nm_auth_chain_steal_data (chain, "agent"); + nm_assert (agent); - result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED); - if (result == NM_AUTH_CALL_RESULT_YES) - nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, TRUE); + result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED); + if (result == NM_AUTH_CALL_RESULT_YES) + nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, TRUE); - result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN); - if (result == NM_AUTH_CALL_RESULT_YES) - nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, TRUE); + result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN); + if (result == NM_AUTH_CALL_RESULT_YES) + nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, TRUE); - priv->agent_version_id += 1; - sender = nm_secret_agent_get_dbus_owner (agent); - g_hash_table_insert (priv->agents, g_strdup (sender), agent); - _LOGI (agent, "agent registered"); - g_dbus_method_invocation_return_value (context, NULL); + priv->agent_version_id += 1; + sender = nm_secret_agent_get_dbus_owner (agent); + g_hash_table_insert (priv->agents, g_strdup (sender), agent); + _LOGI (agent, "agent registered"); + g_dbus_method_invocation_return_value (context, NULL); - /* Signal an agent was registered */ - g_signal_emit (self, signals[AGENT_REGISTERED], 0, agent); + /* Signal an agent was registered */ + g_signal_emit (self, signals[AGENT_REGISTERED], 0, agent); - /* Add this agent to any in-progress secrets requests */ - c_list_for_each (iter, &priv->requests) - request_add_agent (c_list_entry (iter, Request, lst_request), agent); - } + /* Add this agent to any in-progress secrets requests */ + c_list_for_each (iter, &priv->requests) + request_add_agent (c_list_entry (iter, Request, lst_request), agent); } static NMSecretAgent * @@ -1011,7 +1001,6 @@ _con_get_request_start_proceed (Request *req, gboolean include_system_secrets) static void _con_get_request_start_validated (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data) { @@ -1025,28 +1014,20 @@ _con_get_request_start_validated (NMAuthChain *chain, req->con.chain = NULL; - if (error) { - _LOGD (req->current, "agent "LOG_REQ_FMT" MODIFY check error: %s", - LOG_REQ_ARG (req), - error->message); - /* Try the next agent */ - request_next_agent (req); - } else { - /* If the agent obtained the 'modify' permission, we send all system secrets - * to it. If it didn't, we still ask it for secrets, but we don't send - * any system secrets. - */ - perm = nm_auth_chain_get_data (chain, "perm"); - g_assert (perm); - if (nm_auth_chain_get_result (chain, perm) == NM_AUTH_CALL_RESULT_YES) - req->con.current_has_modify = TRUE; + /* If the agent obtained the 'modify' permission, we send all system secrets + * to it. If it didn't, we still ask it for secrets, but we don't send + * any system secrets. + */ + perm = nm_auth_chain_get_data (chain, "perm"); + g_assert (perm); + if (nm_auth_chain_get_result (chain, perm) == NM_AUTH_CALL_RESULT_YES) + req->con.current_has_modify = TRUE; - _LOGD (req->current, "agent "LOG_REQ_FMT" MODIFY check result %s", - LOG_REQ_ARG (req), - req->con.current_has_modify ? "YES" : "NO"); + _LOGD (req->current, "agent "LOG_REQ_FMT" MODIFY check result %s", + LOG_REQ_ARG (req), + req->con.current_has_modify ? "YES" : "NO"); - _con_get_request_start_proceed (req, req->con.current_has_modify); - } + _con_get_request_start_proceed (req, req->con.current_has_modify); } static void @@ -1470,7 +1451,6 @@ nm_agent_manager_all_agents_have_capability (NMAgentManager *manager, static void agent_permissions_changed_done (NMAuthChain *chain, - GError *error, GDBusMethodInvocation *context, gpointer user_data) { @@ -1484,16 +1464,12 @@ agent_permissions_changed_done (NMAuthChain *chain, agent = nm_auth_chain_get_data (chain, "agent"); g_assert (agent); - if (error) - _LOGD (agent, "failed to request updated agent permissions"); - else { - _LOGD (agent, "updated agent permissions"); + _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; - if (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN) == NM_AUTH_CALL_RESULT_YES) - share_open = TRUE; - } + if (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED) == NM_AUTH_CALL_RESULT_YES) + share_protected = TRUE; + if (nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN) == NM_AUTH_CALL_RESULT_YES) + share_open = TRUE; nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_PROTECTED, share_protected); nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, share_open); diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 7e92e4f9ca..23ae87984d 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1134,7 +1134,6 @@ send_agent_owned_secrets (NMSettings *self, static void pk_add_cb (NMAuthChain *chain, - GError *chain_error, GDBusMethodInvocation *context, gpointer user_data) { @@ -1159,12 +1158,7 @@ pk_add_cb (NMAuthChain *chain, result = nm_auth_chain_get_result (chain, perm); - if (chain_error) { - error = g_error_new (NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_FAILED, - "Error checking authorization: %s", - chain_error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { error = g_error_new_literal (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_PERMISSION_DENIED, "Insufficient privileges."); @@ -1503,7 +1497,6 @@ impl_settings_reload_connections (NMDBusObject *obj, static void pk_hostname_cb (NMAuthChain *chain, - GError *chain_error, GDBusMethodInvocation *context, gpointer user_data) { @@ -1520,12 +1513,7 @@ pk_hostname_cb (NMAuthChain *chain, result = nm_auth_chain_get_result (chain, NM_AUTH_PERMISSION_SETTINGS_MODIFY_HOSTNAME); /* If our NMSettingsConnection is already gone, do nothing */ - if (chain_error) { - error = g_error_new (NM_SETTINGS_ERROR, - NM_SETTINGS_ERROR_FAILED, - "Error checking authorization: %s", - chain_error->message); - } else if (result != NM_AUTH_CALL_RESULT_YES) { + if (result != NM_AUTH_CALL_RESULT_YES) { error = g_error_new_literal (NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_PERMISSION_DENIED, "Insufficient privileges."); From 58ee66404d3c3fe626566b64b072ba140f3d0334 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 09:33:26 +0200 Subject: [PATCH 08/32] auth-chain: don't put permission results into regular user data NMAuthChain tracks arbitrary user-data pointers for convenience of the caller. Previously, it would also track the permission request result as such user-data. That is not optimal. - for one, we should keep the namespaces for user data (tags) and permissions separate. When the user requests a permission result with nm_auth_chain_get_result() then clearly no user-data is requested. - we already track permissions in a separate linked list. Previously, when the auth-call completes, we would destroy the entry in the linked list for requests and create an entry in the linked list for user-data. Possibly this was done in the past, because the user-data list was indexed by a hash table. So, in that case, we only would need to lookup the permission results in the indexed user-data hash, but never do any search of a linked list. That is no longer the case, because in practice the number of tracked user-data/permissions is fixed and small (at which point it's just more efficient to search a short linked list). - There is already distrinct API for getting user-data and permissions. By keeping the lists separate we don't need to search the list for entries of the wrong type (it's faster). - also assert that nm_auth_chain_get_result() indeed asks for a result that was previously requested. It makes no sense otherwise. --- src/nm-auth-utils.c | 131 +++++++++++++++++++++++++++++++------------- 1 file changed, 93 insertions(+), 38 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index ef61a92050..368cd42194 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -41,6 +41,8 @@ struct NMAuthChain { NMAuthChainResultFunc done_func; gpointer user_data; + guint num_pending_auth_calls; + bool is_destroyed:1; bool is_finishing:1; }; @@ -49,6 +51,7 @@ typedef struct { CList auth_call_lst; NMAuthChain *chain; NMAuthManagerCallId *call_id; + NMAuthCallResult result; char permission[]; } AuthCall; @@ -64,6 +67,20 @@ _ASSERT_call (AuthCall *call) nm_assert (call); nm_assert (call->chain); nm_assert (nm_c_list_contains_entry (&call->chain->auth_call_lst_head, call, auth_call_lst)); +#if NM_MORE_ASSERTS > 5 + { + AuthCall *auth_call; + guint n = 0; + + c_list_for_each_entry (auth_call, &call->chain->auth_call_lst_head, auth_call_lst) { + nm_assert ( auth_call->result == NM_AUTH_CALL_RESULT_UNKNOWN + || !auth_call->call_id); + if (auth_call->call_id) + n++; + } + nm_assert (n == call->chain->num_pending_auth_calls); + } +#endif } /*****************************************************************************/ @@ -71,12 +88,28 @@ _ASSERT_call (AuthCall *call) static void auth_call_free (AuthCall *call) { - if (call->call_id) - nm_auth_manager_check_authorization_cancel (call->call_id); + _ASSERT_call (call); + c_list_unlink_stale (&call->auth_call_lst); + if (call->call_id) { + call->chain->num_pending_auth_calls--; + nm_auth_manager_check_authorization_cancel (call->call_id); + } g_slice_free (AuthCall, call); } +static AuthCall * +_find_auth_call (NMAuthChain *self, const char *permission) +{ + AuthCall *auth_call; + + c_list_for_each_entry (auth_call, &self->auth_call_lst_head, auth_call_lst) { + if (nm_streq (auth_call->permission, permission)) + return auth_call; + } + return NULL; +} + /*****************************************************************************/ typedef struct { @@ -213,15 +246,26 @@ nm_auth_chain_set_data (NMAuthChain *self, NMAuthCallResult nm_auth_chain_get_result (NMAuthChain *self, const char *permission) { - ChainData *chain_data; + AuthCall *auth_call; g_return_val_if_fail (self, NM_AUTH_CALL_RESULT_UNKNOWN); g_return_val_if_fail (permission, NM_AUTH_CALL_RESULT_UNKNOWN); - chain_data = _get_data (self, permission); - return chain_data - ? (NMAuthCallResult) GPOINTER_TO_UINT (chain_data->data) - : NM_AUTH_CALL_RESULT_UNKNOWN; + /* it is a bug to request the result other than from the done_func() + * callback. You are not supposed to poll for the result but request + * it upon notification. */ + nm_assert (self->is_finishing); + + auth_call = _find_auth_call (self, permission); + + /* it is a bug to request a permission result that was not + * previously requested or which did not complete yet. */ + if (!auth_call) + g_return_val_if_reached (NM_AUTH_CALL_RESULT_UNKNOWN); + + nm_assert (!auth_call->call_id); + + return auth_call->result; } NMAuthSubject * @@ -234,31 +278,6 @@ nm_auth_chain_get_subject (NMAuthChain *self) /*****************************************************************************/ -static void -auth_call_complete (AuthCall *call) -{ - NMAuthChain *self; - - _ASSERT_call (call); - - self = call->chain; - - nm_assert (!self->is_finishing); - - auth_call_free (call); - - if (c_list_is_empty (&self->auth_call_lst_head)) { - /* we are on an idle-handler or a clean call-stack (non-reentrant). */ - nm_assert (!self->is_destroyed); - nm_assert (!self->is_finishing); - - self->is_finishing = TRUE; - self->done_func (self, self->context, self->user_data); - nm_assert (self->is_finishing); - _auth_chain_destroy (self); - } -} - static void pk_call_cb (NMAuthManager *auth_manager, NMAuthManagerCallId *call_id, @@ -267,23 +286,41 @@ pk_call_cb (NMAuthManager *auth_manager, GError *error, gpointer user_data) { + NMAuthChain *self; AuthCall *call; - NMAuthCallResult call_result; + + nm_assert (call_id); if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; call = user_data; + _ASSERT_call (call); nm_assert (call->call_id == call_id); + nm_assert (call->result == NM_AUTH_CALL_RESULT_UNKNOWN); + + self = call->chain; + + nm_assert (!self->is_destroyed); + nm_assert (!self->is_finishing); call->call_id = NULL; - call_result = nm_auth_call_result_eval (is_authorized, is_challenge, error); + call->result = nm_auth_call_result_eval (is_authorized, is_challenge, error); - nm_auth_chain_set_data (call->chain, call->permission, GUINT_TO_POINTER (call_result), NULL); + call->chain->num_pending_auth_calls--; - auth_call_complete (call); + _ASSERT_call (call); + + if (call->chain->num_pending_auth_calls == 0) { + /* we are on an idle-handler or a clean call-stack (non-reentrant) so it's safe + * to invoke the callback right away. */ + self->is_finishing = TRUE; + self->done_func (self, self->context, self->user_data); + nm_assert (self->is_finishing); + _auth_chain_destroy (self); + } } void @@ -303,18 +340,37 @@ nm_auth_chain_add_call (NMAuthChain *self, nm_assert ( nm_auth_subject_is_unix_process (self->subject) || nm_auth_subject_is_internal (self->subject)); + /* duplicate permissions are not supported, also because nm_auth_chain_get_result() + * can only return one-permission. */ + nm_assert (!_find_auth_call (self, permission)); + l_p_1 = strlen (permission) + 1; call = g_malloc (sizeof (AuthCall) + l_p_1); call->chain = self; call->call_id = NULL; + call->result = NM_AUTH_CALL_RESULT_UNKNOWN; memcpy (call->permission, permission, l_p_1); - c_list_link_tail (&self->auth_call_lst_head, &call->auth_call_lst); + + /* above we assert that no duplicate permissions are added. Still, track the + * new request to the front of the list so that it would shadow an earlier + * call. */ + c_list_link_front (&self->auth_call_lst_head, &call->auth_call_lst); + call->call_id = nm_auth_manager_check_authorization (auth_manager, self->subject, permission, allow_interaction, pk_call_cb, call); + + self->num_pending_auth_calls++; + + _ASSERT_call (call); + + /* we track auth-calls in a linked list. If we end up requesting too many permissions this + * becomes inefficient. If that ever happens, consider a more efficient data structure for + * a large number of requests. */ + nm_assert (self->num_pending_auth_calls < 25); } /*****************************************************************************/ @@ -343,7 +399,6 @@ nm_auth_chain_new_context (GDBusMethodInvocation *context, return chain; } -/* Requires an NMAuthSubject */ NMAuthChain * nm_auth_chain_new_subject (NMAuthSubject *subject, GDBusMethodInvocation *context, From 6c63799e4f2dc1336e6134d83c0d204c9b13c83c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 10:07:21 +0200 Subject: [PATCH 09/32] auth-chain: don't allow setting the same user-data twice We track the user-data in a linked list. Hence, when setting a user data we would need to search the list whether the tag already exists. This has an overhead and makes set-data() O(n). But we really don't need this. The NMAuthChain allows a simple way to attach user-data to the request. We don't need a full-blown g_object_set_data() (which btw is also just implemented as a linked list). --- src/nm-auth-utils.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 368cd42194..b1b3d7042e 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -218,26 +218,24 @@ nm_auth_chain_set_data (NMAuthChain *self, * necessary, revert the code to use GHashTable again. */ nm_assert (c_list_length (&self->data_lst_head) < 25); - chain_data = _get_data (self, tag); + /* The tag must not yet exist. Otherwise we'd have to first search the linked + * list for an existing entry. */ + nm_assert (!_get_data (self, tag)); - if (data == NULL) { - if (chain_data) - chain_data_free (chain_data); - return; - } - - if (chain_data) { - gpointer old_data = chain_data->data; - GDestroyNotify old_destroy = chain_data->destroy; - - chain_data->data = data; - chain_data->destroy = data_destroy; - if (old_destroy) - old_destroy (old_data); + if (!data) { + /* we don't track user data of %NULL. + * + * In the past this had also the meaning of removing a user-data. But since + * nm_auth_chain_set_data() does not allow being called more than once + * for the same tag, we don't need to remove anything. */ return; } chain_data = chain_data_new_stale (tag, data, data_destroy); + + /* we assert that no duplicate tags are added. But still, add the new + * element to the front, so that it would shadow the duplicate element + * in the list. */ c_list_link_front (&self->data_lst_head, &chain_data->data_lst); } From 9df2abea53065c88878f00a467811cd635d3c6a8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 10:31:18 +0200 Subject: [PATCH 10/32] auth-chain: don't clone the permission string for AuthChain Out of the 33 callers of nm_auth_chain_add_call(), the permission argument is: - 29 times a C string literal like NM_AUTH_PERMISSION_NETWORK_CONTROL. - 3 times assign a string that is in fact a static string (it's just not a string literal) - only NMManager's device_auth_request_cb() passes a permission of (possibly) non static origin. But it already duplicates the string for it's own purposes and attaches it as user-data to the NMAuthChain. There really is no need to duplicate the string. Replace nm_auth_chain_add_call() by a macro that ensures that the permission string is a C literal. Rename nm_auth_chain_add_call() to nm_auth_chain_add_call_unsafe() to indicate that the lifetime of the permission argument is now the responsibility of the caller. --- src/nm-auth-utils.c | 47 +++++++++++++++++++++++++-------- src/nm-auth-utils.h | 9 ++++--- src/nm-manager.c | 9 ++++--- src/settings/nm-agent-manager.c | 2 +- src/settings/nm-settings.c | 4 +-- 5 files changed, 51 insertions(+), 20 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index b1b3d7042e..2e61ad2060 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -51,8 +51,8 @@ typedef struct { CList auth_call_lst; NMAuthChain *chain; NMAuthManagerCallId *call_id; + const char *permission; NMAuthCallResult result; - char permission[]; } AuthCall; /*****************************************************************************/ @@ -66,6 +66,7 @@ _ASSERT_call (AuthCall *call) { nm_assert (call); nm_assert (call->chain); + nm_assert (call->permission && strlen (call->permission) > 0); nm_assert (nm_c_list_contains_entry (&call->chain->auth_call_lst_head, call, auth_call_lst)); #if NM_MORE_ASSERTS > 5 { @@ -321,14 +322,30 @@ pk_call_cb (NMAuthManager *auth_manager, } } +/** + * nm_auth_chain_add_call_unsafe: + * @self: the #NMAuthChain + * @permission: the permission string. This string is kept by reference + * and you must make sure that it's lifetime lasts until the NMAuthChain + * gets destroyed. That's why the function is "unsafe". Use + * nm_auth_chain_add_call() instead. + * @allow_interaction: flag + * + * It's "unsafe" because @permission is not copied. It's the callers responsibility + * that the permission string stays valid as long as NMAuthChain. + * + * If you can, use nm_auth_chain_add_call() instead! + * + * If you have a non-static string, you may attach the permission string as + * user-data via nm_auth_chain_set_data(). + */ void -nm_auth_chain_add_call (NMAuthChain *self, - const char *permission, - gboolean allow_interaction) +nm_auth_chain_add_call_unsafe (NMAuthChain *self, + const char *permission, + gboolean allow_interaction) { AuthCall *call; NMAuthManager *auth_manager = nm_auth_manager_get (); - gsize l_p_1; g_return_if_fail (self); g_return_if_fail (self->subject); @@ -342,12 +359,16 @@ nm_auth_chain_add_call (NMAuthChain *self, * can only return one-permission. */ nm_assert (!_find_auth_call (self, permission)); - l_p_1 = strlen (permission) + 1; - call = g_malloc (sizeof (AuthCall) + l_p_1); - call->chain = self; - call->call_id = NULL; - call->result = NM_AUTH_CALL_RESULT_UNKNOWN; - memcpy (call->permission, permission, l_p_1); + call = g_slice_new (AuthCall); + + *call = (AuthCall) { + .chain = self, + .call_id = NULL, + .result = NM_AUTH_CALL_RESULT_UNKNOWN, + + /* we don't clone the permission string. It's the callers responsiblity. */ + .permission = permission, + }; /* above we assert that no duplicate permissions are added. Still, track the * new request to the front of the list so that it would shadow an earlier @@ -463,6 +484,10 @@ _auth_chain_destroy (NMAuthChain *self) nm_clear_g_object (&self->subject); nm_clear_g_object (&self->context); + /* we must first destry all AuthCall instances before ChainData. The reason is + * that AuthData.permission is not cloned and the lifetime of the string must + * be ensured by the caller. A sensible thing to do for the caller is attach the + * permission string via nm_auth_chain_set_data(). Hence, first free the AuthCall. */ while ((call = c_list_first_entry (&self->auth_call_lst_head, AuthCall, auth_call_lst))) auth_call_free (call); diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index 58180bc4b1..e482af7f58 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -52,9 +52,12 @@ void nm_auth_chain_set_data (NMAuthChain *chain, NMAuthCallResult nm_auth_chain_get_result (NMAuthChain *chain, const char *permission); -void nm_auth_chain_add_call (NMAuthChain *chain, - const char *permission, - gboolean allow_interaction); +void nm_auth_chain_add_call_unsafe (NMAuthChain *chain, + const char *permission, + gboolean allow_interaction); + +#define nm_auth_chain_add_call(chain, permission, allow_interaction) \ + nm_auth_chain_add_call_unsafe ((chain), ""permission"", (allow_interaction)) void nm_auth_chain_destroy (NMAuthChain *chain); diff --git a/src/nm-manager.c b/src/nm-manager.c index b35761b2b3..0ce15892fa 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2385,6 +2385,7 @@ device_auth_request_cb (NMDevice *device, GError *error = NULL; NMAuthSubject *subject = NULL; NMAuthChain *chain; + char *permission_dup; /* Validate the caller */ subject = nm_auth_subject_new_unix_process_from_context (context); @@ -2413,12 +2414,14 @@ device_auth_request_cb (NMDevice *device, goto done; } + permission_dup = g_strdup (permission); + priv->auth_chains = g_slist_append (priv->auth_chains, chain); nm_auth_chain_set_data (chain, "device", g_object_ref (device), g_object_unref); - nm_auth_chain_set_data (chain, "requested-permission", g_strdup (permission), g_free); nm_auth_chain_set_data (chain, "callback", callback, NULL); nm_auth_chain_set_data (chain, "user-data", user_data, NULL); - nm_auth_chain_add_call (chain, permission, allow_interaction); + nm_auth_chain_set_data (chain, "requested-permission", permission_dup /* transfer ownership */, g_free); + nm_auth_chain_add_call_unsafe (chain, permission_dup, allow_interaction); done: if (error) @@ -6844,7 +6847,7 @@ nm_manager_dbus_set_property_handle (NMDBusObject *obj, chain = nm_auth_chain_new_subject (subject, invocation, _dbus_set_property_auth_cb, handle_data); priv->auth_chains = g_slist_append (priv->auth_chains, chain); - nm_auth_chain_add_call (chain, property_info->writable.permission, TRUE); + nm_auth_chain_add_call_unsafe (chain, property_info->writable.permission, TRUE); return; err: diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 73f59c0b3c..a9fc56d3ff 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -1073,7 +1073,7 @@ _con_get_request_start (Request *req) perm = NM_AUTH_PERMISSION_SETTINGS_MODIFY_SYSTEM; nm_auth_chain_set_data (req->con.chain, "perm", (gpointer) perm, NULL); - nm_auth_chain_add_call (req->con.chain, perm, TRUE); + nm_auth_chain_add_call_unsafe (req->con.chain, perm, TRUE); } else { _LOGD (NULL, "("LOG_REQ_FMT") requesting user-owned secrets from agent %s", LOG_REQ_ARG (req), agent_dbus_owner); diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 23ae87984d..2476caa117 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1274,7 +1274,7 @@ nm_settings_add_connection_dbus (NMSettings *self, * request affects more than just the caller, require 'modify.system'. */ s_con = nm_connection_get_setting_connection (connection); - g_assert (s_con); + nm_assert (s_con); if (nm_setting_connection_get_num_permissions (s_con) == 1) perm = NM_AUTH_PERMISSION_SETTINGS_MODIFY_OWN; else @@ -1296,7 +1296,7 @@ nm_settings_add_connection_dbus (NMSettings *self, nm_auth_chain_set_data (chain, "callback-data", user_data, NULL); nm_auth_chain_set_data (chain, "subject", g_object_ref (subject), g_object_unref); nm_auth_chain_set_data (chain, "save-to-disk", GUINT_TO_POINTER (save_to_disk), NULL); - nm_auth_chain_add_call (chain, perm, TRUE); + nm_auth_chain_add_call_unsafe (chain, perm, TRUE); return; done: From 04fafd582b87328be0087290feb2d9c9f59c8bbd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 6 May 2019 09:53:26 +0200 Subject: [PATCH 11/32] manager: use "perm" name for nm_auth_chain_set_data() tag in device_auth_request_cb() There are two similar cases where the caller attaches the requested permission to the auth-chain. There the tag "perm" is used. Rename for consistency. --- src/nm-manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/nm-manager.c b/src/nm-manager.c index 0ce15892fa..90bc69bfbb 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -2344,7 +2344,7 @@ device_auth_done_cb (NMAuthChain *chain, priv->auth_chains = g_slist_remove (priv->auth_chains, chain); - permission = nm_auth_chain_get_data (chain, "requested-permission"); + permission = nm_auth_chain_get_data (chain, "perm"); nm_assert (permission); callback = nm_auth_chain_get_data (chain, "callback"); nm_assert (callback); @@ -2420,7 +2420,7 @@ device_auth_request_cb (NMDevice *device, nm_auth_chain_set_data (chain, "device", g_object_ref (device), g_object_unref); nm_auth_chain_set_data (chain, "callback", callback, NULL); nm_auth_chain_set_data (chain, "user-data", user_data, NULL); - nm_auth_chain_set_data (chain, "requested-permission", permission_dup /* transfer ownership */, g_free); + nm_auth_chain_set_data (chain, "perm", permission_dup /* transfer ownership */, g_free); nm_auth_chain_add_call_unsafe (chain, permission_dup, allow_interaction); done: From 5a3ffffe7453bf0fcde97e14f0cc7a2d85e8bce8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 6 May 2019 10:04:16 +0200 Subject: [PATCH 12/32] auth-chain: don't copy ChainData tag All callers pass a C string literal as the user-data tag of NMAuthChain. It makes little sense otherwise because you usually know which user data you need in advance. So don't bother with copying the string. Just reference the defacto static string. Rename nm_auth_chain_set_data() to nm_auth_chain_set_data_unsafe() to indicate that the lifetime of the tag string is now the caller's responsibility. The nm_auth_chain_set_data() macro now ensures that the tag argument is a C string literal. In fact, all callers that we had did that already. --- src/nm-auth-utils.c | 52 ++++++++++++++++++++++----------------------- src/nm-auth-utils.h | 11 ++++++---- 2 files changed, 33 insertions(+), 30 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 2e61ad2060..81ea66a2ab 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -115,37 +115,18 @@ _find_auth_call (NMAuthChain *self, const char *permission) typedef struct { CList data_lst; + const char *tag; gpointer data; GDestroyNotify destroy; - char tag[]; } ChainData; -static ChainData * -chain_data_new_stale (const char *tag, - gpointer data, - GDestroyNotify destroy) -{ - ChainData *chain_data; - gsize l_p_1; - - nm_assert (tag); - nm_assert (data); - - l_p_1 = strlen (tag) + 1; - chain_data = g_malloc (sizeof (ChainData) + l_p_1); - chain_data->data = data; - chain_data->destroy = destroy; - memcpy (&chain_data->tag[0], tag, l_p_1); - return chain_data; -} - static void chain_data_free (ChainData *chain_data) { c_list_unlink_stale (&chain_data->data_lst); if (chain_data->destroy) chain_data->destroy (chain_data->data); - g_free (chain_data); + g_slice_free (ChainData, chain_data); } static ChainData * @@ -204,11 +185,25 @@ nm_auth_chain_steal_data (NMAuthChain *self, const char *tag) return value; } +/** + * nm_auth_chain_set_data_unsafe: + * @self: the #NMAuthChain + * @tag: the tag for referencing the attached data. + * @data: the data to attach. If %NULL, this call has no effect + * and nothing is attached. + * @data_destroy: (allow-none): the destroy function for the data pointer. + * + * @tag string is not cloned and must outlife @self. That is why + * the function is "unsafe". Use nm_auth_chain_set_data() with a C literal + * instead. + * + * It is a bug to add the same tag more than once. + */ void -nm_auth_chain_set_data (NMAuthChain *self, - const char *tag, - gpointer data, - GDestroyNotify data_destroy) +nm_auth_chain_set_data_unsafe (NMAuthChain *self, + const char *tag, + gpointer data, + GDestroyNotify data_destroy) { ChainData *chain_data; @@ -232,7 +227,12 @@ nm_auth_chain_set_data (NMAuthChain *self, return; } - chain_data = chain_data_new_stale (tag, data, data_destroy); + chain_data = g_slice_new (ChainData); + *chain_data = (ChainData) { + .tag = tag, + .data = data, + .destroy = data_destroy, + }; /* we assert that no duplicate tags are added. But still, add the new * element to the front, so that it would shadow the duplicate element diff --git a/src/nm-auth-utils.h b/src/nm-auth-utils.h index e482af7f58..ebc7d0a9d1 100644 --- a/src/nm-auth-utils.h +++ b/src/nm-auth-utils.h @@ -44,10 +44,13 @@ gpointer nm_auth_chain_get_data (NMAuthChain *chain, const char *tag); gpointer nm_auth_chain_steal_data (NMAuthChain *chain, const char *tag); -void nm_auth_chain_set_data (NMAuthChain *chain, - const char *tag, - gpointer data, - GDestroyNotify data_destroy); +void nm_auth_chain_set_data_unsafe (NMAuthChain *chain, + const char *tag, + gpointer data, + GDestroyNotify data_destroy); + +#define nm_auth_chain_set_data(chain, tag, data, data_destroy) \ + nm_auth_chain_set_data_unsafe ((chain), ""tag"", (data), (data_destroy)) NMAuthCallResult nm_auth_chain_get_result (NMAuthChain *chain, const char *permission); From 4e24ebcc7b04a11cdd1149086a6d9eb7da8d18a8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:00:31 +0200 Subject: [PATCH 13/32] core: minor cleanup --- src/nm-auth-manager.c | 12 +++++++----- src/nm-auth-utils.c | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 09a217ea11..bae437b6c1 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -365,11 +365,13 @@ nm_auth_manager_check_authorization (NMAuthManager *self, ? POLKIT_CHECK_AUTHORIZATION_FLAGS_ALLOW_USER_INTERACTION : POLKIT_CHECK_AUTHORIZATION_FLAGS_NONE; - call_id = g_slice_new0 (NMAuthManagerCallId); - call_id->self = g_object_ref (self); - call_id->callback = callback; - call_id->user_data = user_data; - call_id->call_numid = ++priv->call_numid_counter; + call_id = g_slice_new (NMAuthManagerCallId); + *call_id = (NMAuthManagerCallId) { + .self = g_object_ref (self), + .callback = callback, + .user_data = user_data, + .call_numid = ++priv->call_numid_counter, + }; c_list_link_tail (&priv->calls_lst_head, &call_id->calls_lst); if (!priv->polkit_enabled) { diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index 81ea66a2ab..454a6621c1 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -345,7 +345,6 @@ nm_auth_chain_add_call_unsafe (NMAuthChain *self, gboolean allow_interaction) { AuthCall *call; - NMAuthManager *auth_manager = nm_auth_manager_get (); g_return_if_fail (self); g_return_if_fail (self->subject); @@ -375,7 +374,7 @@ nm_auth_chain_add_call_unsafe (NMAuthChain *self, * call. */ c_list_link_front (&self->auth_call_lst_head, &call->auth_call_lst); - call->call_id = nm_auth_manager_check_authorization (auth_manager, + call->call_id = nm_auth_manager_check_authorization (nm_auth_manager_get (), self->subject, permission, allow_interaction, From 49ed2079abd629d516e3227ce2aa5fffd5b46fc8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 13:43:19 +0200 Subject: [PATCH 14/32] core/dbus: let NMDBusManager create a D-Bus connection in "configure-and-quit=true" mode Note that various components (NMFirewallManager, NMAuthManager, NMConnectivity, etc.pp) all request their own GDBusConnection from glib's G_BUS_TYPE_SYSTEM singleton. In the future, let them instead use the D-Bus connection that NMDBusManager already has. - NMDBusManager also uses g_bus_get(G_BUS_TYPE_SYSTEM), so in practice this is just the same GDBusConnection instance. - if it would not be the same GDBusConnection instance, it would be more correct/logical to use the one that NMDBusManager uses. - NMDBusManager already aquired the GDBusConnection synchronously and it's ready for use. On the other hand, g_bus_get()/g_bus_get_sync() has the notion that getting the singleton cannot be done without waiting/blocking. So at least it involves locking or even dispatching the async reply on D-Bus. - in "configure-and-quit=initrd" we really don't have D-Bus available. NMDBusManager should control whether the other components use D-Bus or not. For example, NMFirewallManager should not ask glib for a G_BUS_TYPE_SYSTEM singleton only to later find out that it doesn't work. So if these components would reuse NMDBusManager's GDBusConnection, then it must have the connection also in regular "configure-and-quit=true" mode. In this case, we are in late boot and want do connectivity checking and talk to firewalld. --- src/main.c | 39 +++++++++++++++++---- src/nm-connectivity.c | 2 +- src/nm-dbus-manager.c | 79 ++++++++++++++++++++++++++----------------- src/nm-dbus-manager.h | 3 +- 4 files changed, 83 insertions(+), 40 deletions(-) diff --git a/src/main.c b/src/main.c index 02a4210532..6b356e16c2 100644 --- a/src/main.c +++ b/src/main.c @@ -213,6 +213,35 @@ do_early_setup (int *argc, char **argv[], NMConfigCmdLineOptions *config_cli) global_opt.pidfile = global_opt.pidfile ?: g_strdup(NM_DEFAULT_PID_FILE); } +static gboolean +_dbus_manager_init (NMConfig *config) +{ + NMDBusManager *busmgr; + NMConfigConfigureAndQuitType c_a_q_type; + + busmgr = nm_dbus_manager_get (); + + c_a_q_type = nm_config_get_configure_and_quit (config); + + if (c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_DISABLED) + return nm_dbus_manager_acquire_bus (busmgr, TRUE); + + if (c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_ENABLED) { + /* D-Bus is useless in configure and quit mode -- we're eventually dropping + * off and potential clients would have no way of knowing whether we're + * finished already or didn't start yet. + * + * But we still create a nm_dbus_manager_get_dbus_connection() D-Bus connection + * so that we can talk to other services like firewalld. */ + return nm_dbus_manager_acquire_bus (busmgr, FALSE); + } + + nm_assert (c_a_q_type == NM_CONFIG_CONFIGURE_AND_QUIT_INITRD); + /* in initrd we don't have D-Bus at all. Don't even try to get the G_BUS_TYPE_SYSTEM + * connection. And of course don't claim the D-Bus name. */ + return TRUE; +} + /* * main * @@ -399,15 +428,11 @@ main (int argc, char *argv[]) NM_CONFIG_KEYFILE_KEY_MAIN_AUTH_POLKIT, NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT_BOOL)); - if (!nm_config_get_configure_and_quit (config)) { - /* D-Bus is useless in configure and quit mode -- we're eventually dropping - * off and potential clients would have no way of knowing whether we're - * finished already or didn't start yet. */ - if (!nm_dbus_manager_acquire_bus (nm_dbus_manager_get ())) - goto done_no_manager; - } + if (!_dbus_manager_init (config)) + goto done_no_manager; manager = nm_manager_setup (); + nm_dbus_manager_start (nm_dbus_manager_get(), nm_manager_dbus_set_property_handle, manager); diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 2816e76afc..de3d8b4dff 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -876,7 +876,7 @@ nm_connectivity_check_start (NMConnectivity *self, cb_data->concheck.resolve_cancellable = g_cancellable_new (); - g_dbus_connection_call (nm_dbus_manager_get_dbus_connection (nm_dbus_manager_get ()), + g_dbus_connection_call (dbus_connection, "org.freedesktop.resolve1", "/org/freedesktop/resolve1", "org.freedesktop.resolve1.Manager", diff --git a/src/nm-dbus-manager.c b/src/nm-dbus-manager.c index 1a455c4280..6a61079d6a 100644 --- a/src/nm-dbus-manager.c +++ b/src/nm-dbus-manager.c @@ -471,6 +471,9 @@ _bus_get_unix_pid (NMDBusManager *self, guint32 unix_pid = G_MAXUINT32; gs_unref_variant GVariant *ret = NULL; + if (!priv->main_dbus_connection) + return FALSE; + ret = g_dbus_connection_call_sync (priv->main_dbus_connection, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, @@ -500,6 +503,9 @@ _bus_get_unix_user (NMDBusManager *self, guint32 unix_uid = G_MAXUINT32; gs_unref_variant GVariant *ret = NULL; + if (!priv->main_dbus_connection) + return FALSE; + ret = g_dbus_connection_call_sync (priv->main_dbus_connection, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, @@ -1024,6 +1030,7 @@ _obj_register (NMDBusManager *self, nm_assert (c_list_is_empty (&obj->internal.registration_lst_head)); nm_assert (priv->main_dbus_connection); + nm_assert (priv->objmgr_registration_id != 0); nm_assert (priv->started); n_klasses = 0; @@ -1119,15 +1126,10 @@ _obj_unregister (NMDBusManager *self, GVariantBuilder builder; nm_assert (NM_IS_DBUS_OBJECT (obj)); - - if (!priv->main_dbus_connection) { - /* nothing to do for the moment. */ - nm_assert (c_list_is_empty (&obj->internal.registration_lst_head)); - return; - } - + nm_assert (priv->main_dbus_connection); + nm_assert (priv->objmgr_registration_id != 0); + nm_assert (priv->started); nm_assert (!c_list_is_empty (&obj->internal.registration_lst_head)); - nm_assert (priv->objmgr_registration_id); g_variant_builder_init (&builder, G_VARIANT_TYPE ("as")); @@ -1202,7 +1204,7 @@ _nm_dbus_manager_obj_export (NMDBusObject *obj) nm_assert_not_reached (); c_list_link_tail (&priv->objects_lst_head, &obj->internal.objects_lst); - if (priv->main_dbus_connection && priv->started) + if (priv->started) _obj_register (self, obj); } @@ -1223,7 +1225,10 @@ _nm_dbus_manager_obj_unexport (NMDBusObject *obj) nm_assert (&obj->internal == g_hash_table_lookup (priv->objects_by_path, &obj->internal)); nm_assert (c_list_contains (&priv->objects_lst_head, &obj->internal.objects_lst)); - _obj_unregister (self, obj); + if (priv->started) + _obj_unregister (self, obj); + else + nm_assert (c_list_is_empty (&obj->internal.registration_lst_head)); if (!g_hash_table_remove (priv->objects_by_path, &obj->internal)) nm_assert_not_reached (); @@ -1249,6 +1254,16 @@ _nm_dbus_manager_obj_notify (NMDBusObject *obj, nm_assert (NM_IS_DBUS_MANAGER (obj->internal.bus_manager)); nm_assert (!c_list_is_empty (&obj->internal.objects_lst)); + self = obj->internal.bus_manager; + priv = NM_DBUS_MANAGER_GET_PRIVATE (self); + + nm_assert (!priv->started || priv->objmgr_registration_id != 0); + nm_assert (priv->objmgr_registration_id == 0 || priv->main_dbus_connection); + nm_assert (c_list_is_empty (&obj->internal.registration_lst_head) != priv->started); + + if (G_UNLIKELY (!priv->started)) + return; + c_list_for_each_entry (reg_data, &obj->internal.registration_lst_head, registration_lst) { if (_reg_data_get_interface_info (reg_data)->legacy_property_changed) { any_legacy_signals = TRUE; @@ -1256,9 +1271,6 @@ _nm_dbus_manager_obj_notify (NMDBusObject *obj, } } - self = obj->internal.bus_manager; - priv = NM_DBUS_MANAGER_GET_PRIVATE (self); - /* do a naive search for the matching NMDBusPropertyInfoExtended infos. Since the number of * (interfaces x properties) is static and possibly small, this naive search is effectively * O(1). We might wanna introduce some index to lookup the properties in question faster. @@ -1401,7 +1413,7 @@ _nm_dbus_manager_obj_emit_signal (NMDBusObject *obj, self = obj->internal.bus_manager; priv = NM_DBUS_MANAGER_GET_PRIVATE (self); - if (!priv->main_dbus_connection || !priv->started) { + if (!priv->started) { nm_g_variant_unref_floating (args); return; } @@ -1565,9 +1577,12 @@ nm_dbus_manager_start (NMDBusManager *self, NMDBusObject *obj; g_return_if_fail (NM_IS_DBUS_MANAGER (self)); + priv = NM_DBUS_MANAGER_GET_PRIVATE (self); - if (!priv->main_dbus_connection) { + nm_assert (!priv->started); + + if (priv->objmgr_registration_id == 0) { /* Do nothing. We're presumably in the configure-and-quit mode. */ return; } @@ -1581,12 +1596,12 @@ nm_dbus_manager_start (NMDBusManager *self, } gboolean -nm_dbus_manager_acquire_bus (NMDBusManager *self) +nm_dbus_manager_acquire_bus (NMDBusManager *self, + gboolean request_name) { NMDBusManagerPrivate *priv; gs_free_error GError *error = NULL; gs_unref_variant GVariant *ret = NULL; - gs_unref_object GDBusConnection *connection = NULL; guint32 result; guint registration_id; @@ -1599,17 +1614,22 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) * acquire the name despite connecting to the bus successfully. * It means that something is gravely broken -- such as another NetworkManager * instance running. */ - connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, - NULL, - &error); - if (!connection) { - _LOGI ("cannot connect to D-Bus: %s", error->message); + priv->main_dbus_connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, + NULL, + &error); + if (!priv->main_dbus_connection) { + _LOGE ("cannot connect to D-Bus: %s", error->message); return FALSE; } - g_dbus_connection_set_exit_on_close (connection, FALSE); + g_dbus_connection_set_exit_on_close (priv->main_dbus_connection, FALSE); - registration_id = g_dbus_connection_register_object (connection, + if (!request_name) { + _LOGD ("D-Bus connection created"); + return TRUE; + } + + registration_id = g_dbus_connection_register_object (priv->main_dbus_connection, OBJECT_MANAGER_SERVER_BASE_PATH, NM_UNCONST_PTR (GDBusInterfaceInfo, &interface_info_objmgr), &dbus_vtable_objmgr, @@ -1621,7 +1641,7 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) return FALSE; } - ret = g_dbus_connection_call_sync (connection, + ret = g_dbus_connection_call_sync (priv->main_dbus_connection, DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, @@ -1634,13 +1654,10 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) -1, NULL, &error); - if (!ret) - return FALSE; - if (!ret) { _LOGE ("fatal failure to acquire D-Bus service \"%s"": %s", NM_DBUS_SERVICE, error->message); - g_dbus_connection_unregister_object(connection, registration_id); + g_dbus_connection_unregister_object (priv->main_dbus_connection, registration_id); return FALSE; } @@ -1648,12 +1665,11 @@ nm_dbus_manager_acquire_bus (NMDBusManager *self) if (result != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) { _LOGE ("fatal failure to acquire D-Bus service \"%s\" (%u). Service already taken", NM_DBUS_SERVICE, (guint) result); - g_dbus_connection_unregister_object(connection, registration_id); + g_dbus_connection_unregister_object (priv->main_dbus_connection, registration_id); return FALSE; } priv->objmgr_registration_id = registration_id; - priv->main_dbus_connection = g_steal_pointer (&connection); _LOGI ("acquired D-Bus service \"%s\"", NM_DBUS_SERVICE); @@ -1690,6 +1706,7 @@ nm_dbus_manager_init (NMDBusManager *self) c_list_init (&priv->private_servers_lst_head); c_list_init (&priv->objects_lst_head); + priv->objects_by_path = g_hash_table_new ((GHashFunc) _objects_by_path_hash, (GEqualFunc) _objects_by_path_equal); c_list_init (&priv->caller_info_lst_head); diff --git a/src/nm-dbus-manager.h b/src/nm-dbus-manager.h index 2de6ff0ba7..7a510e6f40 100644 --- a/src/nm-dbus-manager.h +++ b/src/nm-dbus-manager.h @@ -49,7 +49,8 @@ typedef void (*NMDBusManagerSetPropertyHandler) (NMDBusObject *obj, GVariant *value, gpointer user_data); -gboolean nm_dbus_manager_acquire_bus (NMDBusManager *self); +gboolean nm_dbus_manager_acquire_bus (NMDBusManager *self, + gboolean request_name); GDBusConnection *nm_dbus_manager_get_dbus_connection (NMDBusManager *self); From 143b6e41afae1709b8a7fc43bcb3c4df22707240 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 09:41:53 +0200 Subject: [PATCH 15/32] core: add NM_MAIN_DBUS_CONNECTION_GET macro --- src/nm-dbus-manager.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/nm-dbus-manager.h b/src/nm-dbus-manager.h index 7a510e6f40..2cd1b19d73 100644 --- a/src/nm-dbus-manager.h +++ b/src/nm-dbus-manager.h @@ -54,6 +54,8 @@ gboolean nm_dbus_manager_acquire_bus (NMDBusManager *self, GDBusConnection *nm_dbus_manager_get_dbus_connection (NMDBusManager *self); +#define NM_MAIN_DBUS_CONNECTION_GET (nm_dbus_manager_get_dbus_connection (nm_dbus_manager_get ())) + void nm_dbus_manager_start (NMDBusManager *self, NMDBusManagerSetPropertyHandler set_property_handler, gpointer set_property_handler_data); From 1e01c5fec908f65a9b670a49efa46f04fc40ae3e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 09:37:37 +0200 Subject: [PATCH 16/32] core: use NM_MAIN_DBUS_CONNECTION_GET macro We will use the D-Bus connection of our NMDBusManager singleton more. Use a macro. - it's shorter to type and it's one distinct word. - the name indicates what this is: the main D-Bus connection singleton. By searching for this name we can find all users that care about using this singleton. --- src/dns/nm-dns-systemd-resolved.c | 2 +- src/nm-connectivity.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 5d262ba3c7..27f106b763 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -527,7 +527,7 @@ nm_dns_systemd_resolved_init (NMDnsSystemdResolved *self) c_list_init (&priv->request_queue_lst_head); - priv->dbus_connection = nm_g_object_ref (nm_dbus_manager_get_dbus_connection (nm_dbus_manager_get ())); + priv->dbus_connection = nm_g_object_ref (NM_MAIN_DBUS_CONNECTION_GET); if (!priv->dbus_connection) { _LOGD ("no D-Bus connection"); return; diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index de3d8b4dff..a842a4cda7 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -863,7 +863,7 @@ nm_connectivity_check_start (NMConnectivity *self, if (has_systemd_resolved) { GDBusConnection *dbus_connection; - dbus_connection = nm_dbus_manager_get_dbus_connection (nm_dbus_manager_get ()); + dbus_connection = NM_MAIN_DBUS_CONNECTION_GET; if (!dbus_connection) { /* we have no D-Bus connection? That might happen in configure and quit mode. * From 40fb6652a2b76f1eef45e4b9dcfd963062dd0863 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:03:35 +0200 Subject: [PATCH 17/32] auth-manager: re-use D-Bus connection from NMDBusManager First of all, NMDBusManager takes the system D-Bus connection synchronously, so we should avoid API that is asynchronous and first needs to get glib's G_BUS_TYPE_SYSTEM instance. Also, the only reason why NMDBusManager might not have a D-Bus connection is in "initrd" configure-and-quit mode. In that mode we also don't need polkit. --- src/main.c | 6 +++--- src/nm-auth-manager.c | 44 ++++++++++++++++++++++++++++++------------- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/src/main.c b/src/main.c index 6b356e16c2..200cff276c 100644 --- a/src/main.c +++ b/src/main.c @@ -423,14 +423,14 @@ main (int argc, char *argv[]) NM_UTILS_KEEP_ALIVE (config, nm_netns_get (), "NMConfig-depends-on-NMNetns"); + if (!_dbus_manager_init (config)) + goto done_no_manager; + nm_auth_manager_setup (nm_config_data_get_value_boolean (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUP_MAIN, NM_CONFIG_KEYFILE_KEY_MAIN_AUTH_POLKIT, NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT_BOOL)); - if (!_dbus_manager_init (config)) - goto done_no_manager; - manager = nm_manager_setup (); nm_dbus_manager_start (nm_dbus_manager_get(), diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index bae437b6c1..0db7e599ca 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -25,6 +25,7 @@ #include "c-list/src/c-list.h" #include "nm-errors.h" #include "nm-core-internal.h" +#include "nm-dbus-manager.h" #include "NetworkManagerUtils.h" #define POLKIT_SERVICE "org.freedesktop.PolicyKit1" @@ -628,23 +629,40 @@ constructed (GObject *object) { NMAuthManager *self = NM_AUTH_MANAGER (object); NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); + GDBusConnection *dbus_connection; + NMLogLevel logl = LOGL_DEBUG; + const char *create_message; G_OBJECT_CLASS (nm_auth_manager_parent_class)->constructed (object); - _LOGD ("create auth-manager: polkit %s", priv->polkit_enabled ? "enabled" : "disabled"); - - if (priv->polkit_enabled) { - priv->new_proxy_cancellable = g_cancellable_new (); - g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, - NULL, - POLKIT_SERVICE, - POLKIT_OBJECT_PATH, - POLKIT_INTERFACE, - priv->new_proxy_cancellable, - _dbus_new_proxy_cb, - self); + if (!priv->polkit_enabled) { + create_message = "polkit disabled"; + goto out; } + + dbus_connection = NM_MAIN_DBUS_CONNECTION_GET; + if (!dbus_connection) { + priv->polkit_enabled = FALSE; + /* This warrants an info level message. */ + logl = LOGL_INFO; + create_message = "D-Bus connection not available. Polkit is disabled and all requests are authenticated."; + goto out; + } + + priv->new_proxy_cancellable = g_cancellable_new (); + g_dbus_proxy_new (dbus_connection, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, + POLKIT_SERVICE, + POLKIT_OBJECT_PATH, + POLKIT_INTERFACE, + priv->new_proxy_cancellable, + _dbus_new_proxy_cb, + self); + create_message = "polkit enabled"; + +out: + _NMLOG (logl, "create auth-manager: %s", create_message); } NMAuthManager * From a381b3999efa86443fb150a12d74176fb07a27f6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 09:29:43 +0200 Subject: [PATCH 18/32] core/dbus: aquire D-Bus name earlier before initializing NMPlatform/NMNetns singletons Aquiring the bus early tells systemd that NetworkManager is started. Do that even before setting up/creating the singletons for NMPlatform and NMNetns. This is a trick so that NetworkManager is considered earlier to be started. But it's right, because we can and should create the D-Bus socket as early as possible to let other services (that order After=network.target) can already start too. Of course, NetworkManager is not yet fully running and it will take a while longer until it actually replies on D-Bus. But the requests are not lost and services that talk to NetworkManager that early can in the meantime to other startup actions. --- src/main.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main.c b/src/main.c index 200cff276c..4737d2a164 100644 --- a/src/main.c +++ b/src/main.c @@ -418,14 +418,13 @@ main (int argc, char *argv[]) #endif ); - /* Set up platform interaction layer */ + if (!_dbus_manager_init (config)) + goto done_no_manager; + nm_linux_platform_setup (); NM_UTILS_KEEP_ALIVE (config, nm_netns_get (), "NMConfig-depends-on-NMNetns"); - if (!_dbus_manager_init (config)) - goto done_no_manager; - nm_auth_manager_setup (nm_config_data_get_value_boolean (nm_config_get_data_orig (config), NM_CONFIG_KEYFILE_GROUP_MAIN, NM_CONFIG_KEYFILE_KEY_MAIN_AUTH_POLKIT, From 655e6bb1e320f1ac86b4c92e5d7822ead29c257f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:24:07 +0200 Subject: [PATCH 19/32] shared: add "shared/nm-glib-aux/nm-dbus-aux.h" --- Makefile.am | 2 ++ shared/meson.build | 3 ++- shared/nm-glib-aux/nm-dbus-aux.c | 24 ++++++++++++++++++++++++ shared/nm-glib-aux/nm-dbus-aux.h | 26 ++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 shared/nm-glib-aux/nm-dbus-aux.c create mode 100644 shared/nm-glib-aux/nm-dbus-aux.h diff --git a/Makefile.am b/Makefile.am index 1de743030f..3fa232b84c 100644 --- a/Makefile.am +++ b/Makefile.am @@ -335,6 +335,8 @@ shared_nm_glib_aux_libnm_glib_aux_la_CPPFLAGS = \ shared_nm_glib_aux_libnm_glib_aux_la_SOURCES = \ shared/nm-glib-aux/nm-c-list.h \ + shared/nm-glib-aux/nm-dbus-aux.c \ + shared/nm-glib-aux/nm-dbus-aux.h \ shared/nm-glib-aux/nm-dedup-multi.c \ shared/nm-glib-aux/nm-dedup-multi.h \ shared/nm-glib-aux/nm-enum-utils.c \ diff --git a/shared/meson.build b/shared/meson.build index a63068eca5..65fdfcabc7 100644 --- a/shared/meson.build +++ b/shared/meson.build @@ -139,7 +139,8 @@ shared_nm_glib_aux_c_args = [ shared_nm_glib_aux = static_library( 'nm-utils-base', - sources: files('nm-glib-aux/nm-dedup-multi.c', + sources: files('nm-glib-aux/nm-dbus-aux.c', + 'nm-glib-aux/nm-dedup-multi.c', 'nm-glib-aux/nm-enum-utils.c', 'nm-glib-aux/nm-errno.c', 'nm-glib-aux/nm-hash-utils.c', diff --git a/shared/nm-glib-aux/nm-dbus-aux.c b/shared/nm-glib-aux/nm-dbus-aux.c new file mode 100644 index 0000000000..02b466aab2 --- /dev/null +++ b/shared/nm-glib-aux/nm-dbus-aux.c @@ -0,0 +1,24 @@ +/* NetworkManager -- Network link manager + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + * (C) Copyright 2019 Red Hat, Inc. + */ + +#include "nm-default.h" + +#include "nm-dbus-aux.h" + diff --git a/shared/nm-glib-aux/nm-dbus-aux.h b/shared/nm-glib-aux/nm-dbus-aux.h new file mode 100644 index 0000000000..f0cdc19768 --- /dev/null +++ b/shared/nm-glib-aux/nm-dbus-aux.h @@ -0,0 +1,26 @@ +/* NetworkManager -- Network link manager + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, + * Boston, MA 02110-1301 USA. + * + * (C) Copyright 2019 Red Hat, Inc. + */ + +#ifndef __NM_DBUS_AUX_H__ +#define __NM_DBUS_AUX_H__ + +/*****************************************************************************/ + +#endif /* __NM_DBUS_AUX_H__ */ From 8ffa75685e9fd60c6797255db7d1ef4fb68aad78 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:21:23 +0200 Subject: [PATCH 20/32] shared: add nm_dbus_connection_signal_subscribe_name_owner_changed() helper ... and nm_dbus_connection_call_get_name_owner(). We are going to use GDBusConnection more instead of GDBusProxy. Hence, these two functions are the standard repertoire and used over and over. Their arguments are complicated enough to warrant a small helper. --- shared/nm-glib-aux/nm-dbus-aux.c | 45 ++++++++++++++++++++++++++++++++ shared/nm-glib-aux/nm-dbus-aux.h | 35 +++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/shared/nm-glib-aux/nm-dbus-aux.c b/shared/nm-glib-aux/nm-dbus-aux.c index 02b466aab2..083c4feec7 100644 --- a/shared/nm-glib-aux/nm-dbus-aux.c +++ b/shared/nm-glib-aux/nm-dbus-aux.c @@ -22,3 +22,48 @@ #include "nm-dbus-aux.h" +/*****************************************************************************/ + +static void +_nm_dbus_connection_call_get_name_owner_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) +{ + gs_unref_variant GVariant *ret = NULL; + gs_free_error GError *error = NULL; + const char *owner = NULL; + gpointer orig_user_data; + NMDBusConnectionCallGetNameOwnerCb callback; + + nm_utils_user_data_unpack (user_data, &orig_user_data, &callback); + + ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), res, &error); + if (ret) + g_variant_get (ret, "(&s)", &owner); + + callback (owner, error, orig_user_data); +} + +void +nm_dbus_connection_call_get_name_owner (GDBusConnection *dbus_connection, + const char *service_name, + int timeout_msec, + GCancellable *cancellable, + NMDBusConnectionCallGetNameOwnerCb callback, + gpointer user_data) +{ + nm_assert (callback); + + g_dbus_connection_call (dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "GetNameOwner", + g_variant_new ("(s)", service_name), + G_VARIANT_TYPE ("(s)"), + G_DBUS_CALL_FLAGS_NONE, + timeout_msec, + cancellable, + _nm_dbus_connection_call_get_name_owner_cb, + nm_utils_user_data_pack (user_data, callback)); +} diff --git a/shared/nm-glib-aux/nm-dbus-aux.h b/shared/nm-glib-aux/nm-dbus-aux.h index f0cdc19768..c888351b69 100644 --- a/shared/nm-glib-aux/nm-dbus-aux.h +++ b/shared/nm-glib-aux/nm-dbus-aux.h @@ -21,6 +21,41 @@ #ifndef __NM_DBUS_AUX_H__ #define __NM_DBUS_AUX_H__ +#include "nm-std-aux/nm-dbus-compat.h" + +/*****************************************************************************/ + +static inline guint +nm_dbus_connection_signal_subscribe_name_owner_changed (GDBusConnection *dbus_connection, + const char *service_name, + GDBusSignalCallback callback, + gpointer user_data, + GDestroyNotify user_data_free_func) + +{ + return g_dbus_connection_signal_subscribe (dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_INTERFACE_DBUS, + "NameOwnerChanged", + DBUS_PATH_DBUS, + service_name, + G_DBUS_SIGNAL_FLAGS_NONE, + callback, + user_data, + user_data_free_func); +} + +typedef void (*NMDBusConnectionCallGetNameOwnerCb) (const char *name_owner, + GError *error, + gpointer user_data); + +void nm_dbus_connection_call_get_name_owner (GDBusConnection *dbus_connection, + const char *service_name, + int timeout_msec, + GCancellable *cancellable, + NMDBusConnectionCallGetNameOwnerCb callback, + gpointer user_data); + /*****************************************************************************/ #endif /* __NM_DBUS_AUX_H__ */ From b9e2fcccf710237e44437fa37e278eb47a173484 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:30:29 +0200 Subject: [PATCH 21/32] all: use nm_dbus_connection_signal_subscribe_name_owner_changed() ... and nm_dbus_connection_call_get_name_owner(). --- libnm/nm-vpn-service-plugin.c | 16 +++----- src/dns/nm-dns-systemd-resolved.c | 49 ++++++++---------------- src/nm-keep-alive.c | 62 +++++++++++-------------------- src/settings/nm-secret-agent.c | 16 +++----- 4 files changed, 50 insertions(+), 93 deletions(-) diff --git a/libnm/nm-vpn-service-plugin.c b/libnm/nm-vpn-service-plugin.c index abd02f1803..3bc49f58cc 100644 --- a/libnm/nm-vpn-service-plugin.c +++ b/libnm/nm-vpn-service-plugin.c @@ -27,6 +27,7 @@ #include #include "nm-glib-aux/nm-secret-utils.h" +#include "nm-glib-aux/nm-dbus-aux.h" #include "nm-enum-types.h" #include "nm-utils.h" #include "nm-connection.h" @@ -499,16 +500,11 @@ watch_peer (NMVpnServicePlugin *plugin, GDBusConnection *connection = g_dbus_method_invocation_get_connection (context); const char *peer = g_dbus_message_get_sender (g_dbus_method_invocation_get_message (context)); - return g_dbus_connection_signal_subscribe (connection, - "org.freedesktop.DBus", - "org.freedesktop.DBus", - "NameOwnerChanged", - "/org/freedesktop/DBus", - peer, - G_DBUS_SIGNAL_FLAGS_NONE, - peer_vanished, - plugin, - NULL); + return nm_dbus_connection_signal_subscribe_name_owner_changed (connection, + peer, + peer_vanished, + plugin, + NULL); } static void diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 27f106b763..33387a6913 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -32,6 +32,7 @@ #include #include "nm-glib-aux/nm-c-list.h" +#include "nm-glib-aux/nm-dbus-aux.h" #include "nm-core-internal.h" #include "platform/nm-platform.h" #include "nm-utils.h" @@ -474,24 +475,17 @@ name_owner_changed_cb (GDBusConnection *connection, } static void -get_name_owner_cb (GObject *source, - GAsyncResult *res, +get_name_owner_cb (const char *name_owner, + GError *error, gpointer user_data) { NMDnsSystemdResolved *self; NMDnsSystemdResolvedPrivate *priv; - gs_unref_variant GVariant *ret = NULL; - gs_free_error GError *error = NULL; - const char *owner = NULL; - ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), res, &error); - if ( !ret + if ( !name_owner && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; - if (ret) - g_variant_get (ret, "(&s)", &owner); - self = user_data; priv = NM_DNS_SYSTEMD_RESOLVED_GET_PRIVATE (self); @@ -499,7 +493,7 @@ get_name_owner_cb (GObject *source, priv->dbus_initied = TRUE; - name_owner_changed (self, owner); + name_owner_changed (self, name_owner); } /*****************************************************************************/ @@ -533,29 +527,18 @@ nm_dns_systemd_resolved_init (NMDnsSystemdResolved *self) return; } - priv->name_owner_changed_id = g_dbus_connection_signal_subscribe (priv->dbus_connection, - DBUS_SERVICE_DBUS, - DBUS_INTERFACE_DBUS, - "NameOwnerChanged", - DBUS_PATH_DBUS, - SYSTEMD_RESOLVED_DBUS_SERVICE, - G_DBUS_SIGNAL_FLAGS_NONE, - name_owner_changed_cb, - self, - NULL); + priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection, + SYSTEMD_RESOLVED_DBUS_SERVICE, + name_owner_changed_cb, + self, + NULL); priv->cancellable = g_cancellable_new (); - g_dbus_connection_call (priv->dbus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS, - "GetNameOwner", - g_variant_new ("(s)", SYSTEMD_RESOLVED_DBUS_SERVICE), - G_VARIANT_TYPE ("(s)"), - G_DBUS_CALL_FLAGS_NONE, - -1, - priv->cancellable, - get_name_owner_cb, - self); + nm_dbus_connection_call_get_name_owner (priv->dbus_connection, + SYSTEMD_RESOLVED_DBUS_SERVICE, + -1, + priv->cancellable, + get_name_owner_cb, + self); } NMDnsPlugin * diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index cfec138a30..4c8a480f62 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -24,6 +24,7 @@ #include "nm-keep-alive.h" #include "settings/nm-settings-connection.h" +#include "nm-glib-aux/nm-dbus-aux.h" /*****************************************************************************/ @@ -211,32 +212,24 @@ nm_keep_alive_set_settings_connection_watch_visible (NMKeepAlive *self, /*****************************************************************************/ static void -get_name_owner_cb (GObject *source_object, - GAsyncResult *res, +get_name_owner_cb (const char *name_owner, + GError *error, gpointer user_data) { - NMKeepAlive *self = user_data; + NMKeepAlive *self; NMKeepAlivePrivate *priv; - gs_free_error GError *error = NULL; - gs_unref_variant GVariant *result = NULL; - const char *name_owner; - result = g_dbus_connection_call_finish ((GDBusConnection *) source_object, - res, - &error); - if ( !result + if ( !name_owner && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) return; - if (result) { - g_variant_get (result, "(&s)", &name_owner); + self = user_data; + priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - priv = NM_KEEP_ALIVE_GET_PRIVATE (self); - - if (nm_streq (name_owner, priv->dbus_client)) { - /* all good, the name is confirmed. */ - return; - } + if ( name_owner + && nm_streq (name_owner, priv->dbus_client)) { + /* all good, the name is confirmed. */ + return; } _LOGD ("DBus client for keep alive is not on the bus"); @@ -259,18 +252,12 @@ _is_alive_dbus_client (NMKeepAlive *self) priv->dbus_client_confirmed = TRUE; priv->dbus_client_confirm_cancellable = g_cancellable_new (); - g_dbus_connection_call (priv->dbus_connection, - "org.freedesktop.DBus", - "/org/freedesktop/DBus", - "org.freedesktop.DBus", - "GetNameOwner", - g_variant_new ("(s)", priv->dbus_client), - G_VARIANT_TYPE ("(s)"), - G_DBUS_CALL_FLAGS_NONE, - -1, - priv->dbus_client_confirm_cancellable, - get_name_owner_cb, - self); + nm_dbus_connection_call_get_name_owner (priv->dbus_connection, + priv->dbus_client, + -1, + priv->dbus_client_confirm_cancellable, + get_name_owner_cb, + self); } return TRUE; } @@ -336,16 +323,11 @@ nm_keep_alive_set_dbus_client_watch (NMKeepAlive *self, priv->dbus_client_watching = TRUE; priv->dbus_client_confirmed = FALSE; priv->dbus_connection = g_object_ref (connection); - priv->subscription_id = g_dbus_connection_signal_subscribe (connection, - "org.freedesktop.DBus", - "org.freedesktop.DBus", - "NameOwnerChanged", - "/org/freedesktop/DBus", - priv->dbus_client, - G_DBUS_SIGNAL_FLAGS_NONE, - name_owner_changed_cb, - self, - NULL); + priv->subscription_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection, + priv->dbus_client, + name_owner_changed_cb, + self, + NULL); } else priv->dbus_client_watching = FALSE; diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 836ab21d4a..7af40b36ee 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -25,6 +25,7 @@ #include #include +#include "nm-glib-aux/nm-dbus-aux.h" #include "nm-dbus-interface.h" #include "nm-dbus-manager.h" #include "nm-core-internal.h" @@ -749,16 +750,11 @@ nm_secret_agent_new (GDBusMethodInvocation *context, 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); + priv->on_disconnected_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->connection, + priv->dbus_owner, + _on_disconnected_name_owner_changed, + self, + NULL); } return self; From f7fff62067ab0d51c810aa99706b73334c5af644 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:45:13 +0200 Subject: [PATCH 22/32] shared: add nm_clear_g_dbus_connection_signal() helper --- shared/nm-glib-aux/nm-dbus-aux.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/shared/nm-glib-aux/nm-dbus-aux.h b/shared/nm-glib-aux/nm-dbus-aux.h index c888351b69..5f112002b2 100644 --- a/shared/nm-glib-aux/nm-dbus-aux.h +++ b/shared/nm-glib-aux/nm-dbus-aux.h @@ -25,6 +25,23 @@ /*****************************************************************************/ +static inline gboolean +nm_clear_g_dbus_connection_signal (GDBusConnection *dbus_connection, + guint *id) +{ + guint v; + + if ( id + && (v = *id)) { + *id = 0; + g_dbus_connection_signal_unsubscribe (dbus_connection, v); + return TRUE; + } + return FALSE; +} + +/*****************************************************************************/ + static inline guint nm_dbus_connection_signal_subscribe_name_owner_changed (GDBusConnection *dbus_connection, const char *service_name, From 309271ac176bda188ac739e7d38ea595e52b19a4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 11:53:55 +0200 Subject: [PATCH 23/32] all: use nm_clear_g_dbus_connection_signal() helper I also like this because it's non-obvious that subscription IDs from GDBusConnection are "guint" (contrary to signal handler IDs which are "gulong"). So, by using this API you get a compiler error when using the wrong type. In the past, when switching to nm_clear_g_signal_handler() this uncovered multiple bugs where the wrong type was used to hold the ID. --- libnm/nm-vpn-service-plugin.c | 7 ++---- src/dns/nm-dns-systemd-resolved.c | 6 ++--- src/nm-keep-alive.c | 2 +- src/settings/nm-secret-agent.c | 38 +++++++++++++++---------------- 4 files changed, 24 insertions(+), 29 deletions(-) diff --git a/libnm/nm-vpn-service-plugin.c b/libnm/nm-vpn-service-plugin.c index 3bc49f58cc..31cda37671 100644 --- a/libnm/nm-vpn-service-plugin.c +++ b/libnm/nm-vpn-service-plugin.c @@ -1186,11 +1186,8 @@ state_changed (NMVpnServicePlugin *plugin, NMVpnServiceState state) nm_vpn_service_plugin_emit_quit (plugin); else schedule_quit_timer (plugin); - if (priv->peer_watch_id) { - g_dbus_connection_signal_unsubscribe (nm_vpn_service_plugin_get_connection (plugin), - priv->peer_watch_id); - priv->peer_watch_id = 0; - } + nm_clear_g_dbus_connection_signal (nm_vpn_service_plugin_get_connection (plugin), + &priv->peer_watch_id); break; default: /* Clean up all timers we might have set up. */ diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index 33387a6913..f4d742844b 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -555,10 +555,8 @@ dispose (GObject *object) free_pending_updates (self); - if (priv->name_owner_changed_id != 0) { - g_dbus_connection_signal_unsubscribe (priv->dbus_connection, - nm_steal_int (&priv->name_owner_changed_id)); - } + nm_clear_g_dbus_connection_signal (priv->dbus_connection, + &priv->name_owner_changed_id); nm_clear_g_cancellable (&priv->cancellable); diff --git a/src/nm-keep-alive.c b/src/nm-keep-alive.c index 4c8a480f62..1390971c93 100644 --- a/src/nm-keep-alive.c +++ b/src/nm-keep-alive.c @@ -276,7 +276,7 @@ cleanup_dbus_watch (NMKeepAlive *self) nm_clear_g_free (&priv->dbus_client); if (priv->dbus_connection) { g_dbus_connection_signal_unsubscribe (priv->dbus_connection, - priv->subscription_id); + nm_steal_int (&priv->subscription_id)); g_clear_object (&priv->dbus_connection); } } diff --git a/src/settings/nm-secret-agent.c b/src/settings/nm-secret-agent.c index 7af40b36ee..97ffbfeef3 100644 --- a/src/settings/nm-secret-agent.c +++ b/src/settings/nm-secret-agent.c @@ -55,7 +55,10 @@ typedef struct { NMDBusManager *bus_mgr; GDBusConnection *connection; CList requests; - gulong on_disconnected_id; + union { + gulong obj_signal; + guint dbus_signal; + } on_disconnected_id; bool connection_is_private:1; } NMSecretAgentPrivate; @@ -615,15 +618,12 @@ nm_secret_agent_delete_secrets (NMSecretAgent *self, static void _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; + if (priv->connection_is_private) { + nm_clear_g_signal_handler (priv->bus_mgr, + &priv->on_disconnected_id.obj_signal); + } else { + nm_clear_g_dbus_connection_signal (priv->connection, + &priv->on_disconnected_id.dbus_signal); } g_clear_object (&priv->connection); @@ -745,16 +745,16 @@ nm_secret_agent_new (GDBusMethodInvocation *context, /* 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_DBUS_MANAGER_PRIVATE_CONNECTION_DISCONNECTED, - G_CALLBACK (_on_disconnected_private_connection), - self); + priv->on_disconnected_id.obj_signal = g_signal_connect (priv->bus_mgr, + NM_DBUS_MANAGER_PRIVATE_CONNECTION_DISCONNECTED, + G_CALLBACK (_on_disconnected_private_connection), + self); } else { - priv->on_disconnected_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->connection, - priv->dbus_owner, - _on_disconnected_name_owner_changed, - self, - NULL); + priv->on_disconnected_id.dbus_signal = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->connection, + priv->dbus_owner, + _on_disconnected_name_owner_changed, + self, + NULL); } return self; From 654faa4d38f22444d4b72be9317f1f2eecb8bd4d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 11:51:01 +0200 Subject: [PATCH 24/32] shared: add nm_dbus_connection_call_start_service_by_name() helper --- shared/nm-glib-aux/nm-dbus-aux.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/shared/nm-glib-aux/nm-dbus-aux.h b/shared/nm-glib-aux/nm-dbus-aux.h index 5f112002b2..271a7d9c91 100644 --- a/shared/nm-glib-aux/nm-dbus-aux.h +++ b/shared/nm-glib-aux/nm-dbus-aux.h @@ -42,6 +42,30 @@ nm_clear_g_dbus_connection_signal (GDBusConnection *dbus_connection, /*****************************************************************************/ +static inline void +nm_dbus_connection_call_start_service_by_name (GDBusConnection *dbus_connection, + const char *name, + int timeout_msec, + GCancellable *cancellable, + GAsyncReadyCallback callback, + gpointer user_data) +{ + g_dbus_connection_call (dbus_connection, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "StartServiceByName", + g_variant_new ("(su)", name, 0u), + G_VARIANT_TYPE ("(u)"), + G_DBUS_CALL_FLAGS_NONE, + timeout_msec, + cancellable, + callback, + user_data); +} + +/*****************************************************************************/ + static inline guint nm_dbus_connection_signal_subscribe_name_owner_changed (GDBusConnection *dbus_connection, const char *service_name, From 458a5e6531224e2234bdeef3a988d6d0db9ee947 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 11:52:15 +0200 Subject: [PATCH 25/32] src: use nm_dbus_connection_call_start_service_by_name() --- src/dns/nm-dns-systemd-resolved.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/dns/nm-dns-systemd-resolved.c b/src/dns/nm-dns-systemd-resolved.c index f4d742844b..9c011d4993 100644 --- a/src/dns/nm-dns-systemd-resolved.c +++ b/src/dns/nm-dns-systemd-resolved.c @@ -305,18 +305,12 @@ send_updates (NMDnsSystemdResolved *self) _LOGT ("send-updates: no name owner. Try start service..."); priv->try_start_blocked = TRUE; - g_dbus_connection_call (priv->dbus_connection, - DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, - DBUS_INTERFACE_DBUS, - "StartServiceByName", - g_variant_new ("(su)", SYSTEMD_RESOLVED_DBUS_SERVICE, 0u), - G_VARIANT_TYPE ("(u)"), - G_DBUS_CALL_FLAGS_NONE, - -1, - NULL, - NULL, - NULL); + nm_dbus_connection_call_start_service_by_name (priv->dbus_connection, + SYSTEMD_RESOLVED_DBUS_SERVICE, + -1, + NULL, + NULL, + NULL); return; } From 6153cb000098ca1f385989e9cfced91f7915eb4f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 12:46:17 +0200 Subject: [PATCH 26/32] auth-manager: drop GDBusProxy and use GDBusConnection directly Aside avoiding the unnecessary overhead of GDBusProxy, this simplifies NMAuthManager because the instance is ready from the start to use D-Bus. Previously, in the early phase requests needed to be queued until GDBusProxy could be created asynchronously. Now, there is nothing asynchronous involved during construction of the NMAuthManager (and of course there are no blocking calls). --- src/nm-auth-manager.c | 336 ++++++++++++++++++++---------------------- 1 file changed, 160 insertions(+), 176 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 0db7e599ca..c85a4b819b 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -23,6 +23,7 @@ #include "nm-auth-manager.h" #include "c-list/src/c-list.h" +#include "nm-glib-aux/nm-dbus-aux.h" #include "nm-errors.h" #include "nm-core-internal.h" #include "nm-dbus-manager.h" @@ -50,13 +51,15 @@ static guint signals[LAST_SIGNAL] = {0}; typedef struct { CList calls_lst_head; - GDBusProxy *proxy; - GCancellable *new_proxy_cancellable; - GCancellable *cancel_cancellable; + GDBusConnection *dbus_connection; + GCancellable *shutdown_cancellable; guint64 call_numid_counter; - bool polkit_enabled:1; + guint name_owner_changed_id; + guint changed_signal_id; + bool dbus_has_owner:1; bool disposing:1; bool shutting_down:1; + bool polkit_enabled_construct_only:1; } NMAuthManagerPrivate; struct _NMAuthManager { @@ -114,7 +117,7 @@ nm_auth_manager_get_polkit_enabled (NMAuthManager *self) { g_return_val_if_fail (NM_IS_AUTH_MANAGER (self), FALSE); - return NM_AUTH_MANAGER_GET_PRIVATE (self)->polkit_enabled; + return NM_AUTH_MANAGER_GET_PRIVATE (self)->dbus_connection != NULL; } /*****************************************************************************/ @@ -132,7 +135,6 @@ typedef enum { struct _NMAuthManagerCallId { CList calls_lst; NMAuthManager *self; - GVariant *dbus_parameters; GCancellable *dbus_cancellable; NMAuthManagerCheckAuthorizationCallback callback; gpointer user_data; @@ -142,7 +144,7 @@ struct _NMAuthManagerCallId { }; #define cancellation_id_to_str_a(call_numid) \ - nm_sprintf_bufa (NM_STRLEN (CANCELLATION_ID_PREFIX) + 20, \ + nm_sprintf_bufa (NM_STRLEN (CANCELLATION_ID_PREFIX) + 60, \ CANCELLATION_ID_PREFIX"%"G_GUINT64_FORMAT, \ (call_numid)) @@ -151,8 +153,6 @@ _call_id_free (NMAuthManagerCallId *call_id) { c_list_unlink (&call_id->calls_lst); nm_clear_g_source (&call_id->idle_id); - if (call_id->dbus_parameters) - g_variant_unref (g_steal_pointer (&call_id->dbus_parameters)); if (call_id->dbus_cancellable) { /* we have a pending D-Bus call. We keep the call-id instance alive @@ -183,7 +183,7 @@ _call_id_invoke_callback (NMAuthManagerCallId *call_id, } static void -cancel_check_authorization_cb (GObject *proxy, +cancel_check_authorization_cb (GObject *source, GAsyncResult *res, gpointer user_data) { @@ -191,7 +191,7 @@ cancel_check_authorization_cb (GObject *proxy, gs_unref_variant GVariant *value = NULL; gs_free_error GError *error= NULL; - value = g_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), res, &error); + value = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), res, &error); if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) _LOG2T (call_id, "cancel request was cancelled"); else if (error) @@ -225,18 +225,18 @@ _call_check_authorize_cb (GObject *proxy, self = call_id->self; priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - value = _nm_dbus_proxy_call_finish (G_DBUS_PROXY (proxy), res, G_VARIANT_TYPE ("((bba{ss}))"), &error); + value = g_dbus_connection_call_finish (G_DBUS_CONNECTION (proxy), res, &error); if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) { /* call_id was cancelled externally, but _call_id_free() kept call_id * alive (and it has still the reference on @self. */ - if (!priv->cancel_cancellable) { + if (!priv->shutdown_cancellable) { /* we do a forced shutdown. There is no more time for cancelling... */ _call_id_free (call_id); /* this shouldn't really happen, because: * _call_check_authorize() only scheduled the D-Bus request at a time when - * cancel_cancellable was still set. It means, somebody called force-shutdown + * shutdown_cancellable was still set. It means, somebody called force-shutdown * after call-id was schedule. * force-shutdown should only be called after: * - cancel all pending requests @@ -245,15 +245,19 @@ _call_check_authorize_cb (GObject *proxy, g_return_if_reached (); } - g_dbus_proxy_call (priv->proxy, - "CancelCheckAuthorization", - g_variant_new ("(s)", - cancellation_id_to_str_a (call_id->call_numid)), - G_DBUS_CALL_FLAGS_NONE, - CANCELLATION_TIMEOUT_MS, - priv->cancel_cancellable, - cancel_check_authorization_cb, - call_id); + g_dbus_connection_call (priv->dbus_connection, + POLKIT_SERVICE, + POLKIT_OBJECT_PATH, + POLKIT_INTERFACE, + "CancelCheckAuthorization", + g_variant_new ("(s)", + cancellation_id_to_str_a (call_id->call_numid)), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + CANCELLATION_TIMEOUT_MS, + priv->shutdown_cancellable, + cancel_check_authorization_cb, + call_id); return; } @@ -271,30 +275,6 @@ _call_check_authorize_cb (GObject *proxy, _call_id_invoke_callback (call_id, is_authorized, is_challenge, error); } -static void -_call_check_authorize (NMAuthManagerCallId *call_id) -{ - NMAuthManager *self = call_id->self; - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - - nm_assert (call_id->dbus_parameters); - nm_assert (g_variant_is_floating (call_id->dbus_parameters)); - nm_assert (!call_id->dbus_cancellable); - - call_id->dbus_cancellable = g_cancellable_new (); - - nm_assert (priv->cancel_cancellable); - - g_dbus_proxy_call (priv->proxy, - "CheckAuthorization", - g_steal_pointer (&call_id->dbus_parameters), - G_DBUS_CALL_FLAGS_NONE, - G_MAXINT, /* no timeout */ - call_id->dbus_cancellable, - _call_check_authorize_cb, - call_id); -} - static gboolean _call_on_idle (gpointer user_data) { @@ -345,9 +325,6 @@ nm_auth_manager_check_authorization (NMAuthManager *self, NMAuthManagerPrivate *priv; PolkitCheckAuthorizationFlags flags; char subject_buf[64]; - GVariantBuilder builder; - GVariant *subject_value; - GVariant *details_value; NMAuthManagerCallId *call_id; g_return_val_if_fail (NM_IS_AUTH_MANAGER (self), NULL); @@ -375,7 +352,7 @@ nm_auth_manager_check_authorization (NMAuthManager *self, }; c_list_link_tail (&priv->calls_lst_head, &call_id->calls_lst); - if (!priv->polkit_enabled) { + if (!priv->dbus_connection) { _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (succeeding due to polkit authorization disabled)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); call_id->idle_reason = IDLE_REASON_AUTHORIZED; call_id->idle_id = g_idle_add (_call_on_idle, call_id); @@ -387,12 +364,12 @@ nm_auth_manager_check_authorization (NMAuthManager *self, _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (succeeding for root)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); call_id->idle_reason = IDLE_REASON_AUTHORIZED; call_id->idle_id = g_idle_add (_call_on_idle, call_id); - } else if ( !priv->proxy - && !priv->new_proxy_cancellable) { - _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (failing due to invalid DBUS proxy)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); - call_id->idle_reason = IDLE_REASON_NO_DBUS; - call_id->idle_id = g_idle_add (_call_on_idle, call_id); } else { + GVariant *parameters; + GVariantBuilder builder; + GVariant *subject_value; + GVariant *details_value; + subject_value = nm_auth_subject_unix_process_to_polkit_gvariant (subject); nm_assert (g_variant_is_floating (subject_value)); @@ -400,18 +377,31 @@ nm_auth_manager_check_authorization (NMAuthManager *self, g_variant_builder_init (&builder, G_VARIANT_TYPE ("a{ss}")); details_value = g_variant_builder_end (&builder); - call_id->dbus_parameters = g_variant_new ("(@(sa{sv})s@a{ss}us)", - subject_value, - action_id, - details_value, - (guint32) flags, - cancellation_id_to_str_a (call_id->call_numid)); - if (!priv->proxy) { - _LOG2T (call_id, "CheckAuthorization(%s), subject=%s (wait for proxy)", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); - } else { - _LOG2T (call_id, "CheckAuthorization(%s), subject=%s", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); - _call_check_authorize (call_id); - } + parameters = g_variant_new ("(@(sa{sv})s@a{ss}us)", + subject_value, + action_id, + details_value, + (guint32) flags, + cancellation_id_to_str_a (call_id->call_numid)); + + _LOG2T (call_id, "CheckAuthorization(%s), subject=%s", action_id, nm_auth_subject_to_string (subject, subject_buf, sizeof (subject_buf))); + + call_id->dbus_cancellable = g_cancellable_new (); + + nm_assert (priv->shutdown_cancellable); + + g_dbus_connection_call (priv->dbus_connection, + POLKIT_SERVICE, + POLKIT_OBJECT_PATH, + POLKIT_INTERFACE, + "CheckAuthorization", + parameters, + G_VARIANT_TYPE ("((bba{ss}))"), + G_DBUS_CALL_FLAGS_NONE, + G_MAXINT, /* no timeout */ + call_id->dbus_cancellable, + _call_check_authorize_cb, + call_id); } return call_id; @@ -450,108 +440,85 @@ _emit_changed_signal (NMAuthManager *self) } static void -_log_name_owner (NMAuthManager *self, char **out_name_owner) +name_owner_changed (NMAuthManager *self, + const char *owner, + gboolean force_changed_signal) { NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - gs_free char *name_owner = NULL; + gboolean had_owner; - name_owner = g_dbus_proxy_get_name_owner (priv->proxy); - if (name_owner) - _LOGD ("dbus name owner: '%s'", name_owner); + owner = nm_str_not_empty (owner); + + if (!owner) + _LOGT ("D-Bus name for PolKit has no owner"); else - _LOGD ("dbus name owner: none"); + _LOGT ("D-Bus name for PolKit has owner %s", owner); - NM_SET_OUT (out_name_owner, g_steal_pointer (&name_owner)); -} + had_owner = priv->dbus_has_owner; + priv->dbus_has_owner = !!owner; -static void -_dbus_on_name_owner_notify_cb (GObject *object, - GParamSpec *pspec, - gpointer user_data) -{ - NMAuthManager *self = user_data; - gs_free char *name_owner = NULL; - - nm_assert (NM_AUTH_MANAGER_GET_PRIVATE (self)->proxy == (GDBusProxy *) object); - - _log_name_owner (self, &name_owner); - if (!name_owner) { - /* when the name disappears, we also want to raise a emit signal. - * When it appears, we raise one already. */ - _emit_changed_signal (self); + if (!priv->dbus_has_owner) { + if ( had_owner + || force_changed_signal) { + /* when the name disappears, we also want to raise a emit signal. + * When it appears, we raise one already. */ + _emit_changed_signal (self); + } } } static void -_dbus_on_changed_signal_cb (GDBusProxy *proxy, - gpointer user_data) +name_owner_changed_cb (GDBusConnection *connection, + const char *sender_name, + const char *object_path, + const char *interface_name, + const char *signal_name, + GVariant *parameters, + gpointer user_data) +{ + const char *new_owner; + + if (!g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)"))) + return; + + g_variant_get (parameters, + "(&s&s&s)", + NULL, + NULL, + &new_owner); + + name_owner_changed (user_data, new_owner, FALSE); +} + +static void +get_name_owner_cb (const char *name_owner, + GError *error, + gpointer user_data) +{ + if ( !name_owner + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + /* until now the name-owner state was undecided. Pass TRUE to name_owner_changed() + * so we force emitting a a changed signal (iff we have no name owner). */ + name_owner_changed (user_data, name_owner, TRUE); +} + +static void +changed_signal_cb (GDBusConnection *connection, + const char *sender_name, + const char *object_path, + const char *interface_name, + const char *signal_name, + GVariant *parameters, + gpointer user_data) { NMAuthManager *self = user_data; - nm_assert (NM_AUTH_MANAGER_GET_PRIVATE (self)->proxy == proxy); - _LOGD ("dbus signal: \"Changed\""); _emit_changed_signal (self); } -static void -_dbus_new_proxy_cb (GObject *source_object, - GAsyncResult *res, - gpointer user_data) -{ - NMAuthManager *self; - NMAuthManagerPrivate *priv; - gs_free_error GError *error = NULL; - GDBusProxy *proxy; - NMAuthManagerCallId *call_id; - - proxy = g_dbus_proxy_new_for_bus_finish (res, &error); - - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - return; - - self = user_data; - priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - - priv->proxy = proxy; - g_clear_object (&priv->new_proxy_cancellable); - - if (!priv->proxy) { - _LOGE ("could not create polkit proxy: %s", error->message); - -again: - c_list_for_each_entry (call_id, &priv->calls_lst_head, calls_lst) { - if (call_id->dbus_parameters) { - _LOG2T (call_id, "completed: failed due to no D-Bus proxy after startup"); - _call_id_invoke_callback (call_id, FALSE, FALSE, error); - goto again; - } - } - return; - } - - priv->cancel_cancellable = g_cancellable_new (); - - g_signal_connect (priv->proxy, - "notify::g-name-owner", - G_CALLBACK (_dbus_on_name_owner_notify_cb), - self); - _nm_dbus_signal_connect (priv->proxy, "Changed", NULL, - G_CALLBACK (_dbus_on_changed_signal_cb), - self); - - _log_name_owner (self, NULL); - - c_list_for_each_entry (call_id, &priv->calls_lst_head, calls_lst) { - if (call_id->dbus_parameters) { - _LOG2T (call_id, "CheckAuthorization invoke now"); - _call_check_authorize (call_id); - } - } - - _emit_changed_signal (self); -} - /*****************************************************************************/ NMAuthManager * @@ -593,7 +560,7 @@ nm_auth_manager_force_shutdown (NMAuthManager *self) */ priv->shutting_down = TRUE; - nm_clear_g_cancellable (&priv->cancel_cancellable); + nm_clear_g_cancellable (&priv->shutdown_cancellable); } /*****************************************************************************/ @@ -606,7 +573,7 @@ set_property (GObject *object, guint prop_id, const GValue *value, GParamSpec *p switch (prop_id) { case PROP_POLKIT_ENABLED: /* construct-only */ - priv->polkit_enabled = !!g_value_get_boolean (value); + priv->polkit_enabled_construct_only = !!g_value_get_boolean (value); break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); @@ -629,36 +596,51 @@ constructed (GObject *object) { NMAuthManager *self = NM_AUTH_MANAGER (object); NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - GDBusConnection *dbus_connection; NMLogLevel logl = LOGL_DEBUG; const char *create_message; G_OBJECT_CLASS (nm_auth_manager_parent_class)->constructed (object); - if (!priv->polkit_enabled) { + if (!priv->polkit_enabled_construct_only) { create_message = "polkit disabled"; goto out; } - dbus_connection = NM_MAIN_DBUS_CONNECTION_GET; - if (!dbus_connection) { - priv->polkit_enabled = FALSE; + priv->dbus_connection = nm_g_object_ref (NM_MAIN_DBUS_CONNECTION_GET); + + if (!priv->dbus_connection) { /* This warrants an info level message. */ logl = LOGL_INFO; create_message = "D-Bus connection not available. Polkit is disabled and all requests are authenticated."; goto out; } - priv->new_proxy_cancellable = g_cancellable_new (); - g_dbus_proxy_new (dbus_connection, - G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, - NULL, - POLKIT_SERVICE, - POLKIT_OBJECT_PATH, - POLKIT_INTERFACE, - priv->new_proxy_cancellable, - _dbus_new_proxy_cb, - self); + priv->shutdown_cancellable = g_cancellable_new (); + + priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection, + POLKIT_SERVICE, + name_owner_changed_cb, + self, + NULL); + + nm_dbus_connection_call_get_name_owner (priv->dbus_connection, + POLKIT_SERVICE, + -1, + priv->shutdown_cancellable, + get_name_owner_cb, + self); + + priv->changed_signal_id = g_dbus_connection_signal_subscribe (priv->dbus_connection, + POLKIT_SERVICE, + POLKIT_INTERFACE, + "Changed", + POLKIT_OBJECT_PATH, + NULL, + G_DBUS_SIGNAL_FLAGS_NONE, + changed_signal_cb, + self, + NULL); + create_message = "polkit enabled"; out: @@ -697,15 +679,17 @@ dispose (GObject *object) priv->disposing = TRUE; - nm_clear_g_cancellable (&priv->new_proxy_cancellable); - nm_clear_g_cancellable (&priv->cancel_cancellable); + nm_clear_g_cancellable (&priv->shutdown_cancellable); - if (priv->proxy) { - g_signal_handlers_disconnect_by_data (priv->proxy, self); - g_clear_object (&priv->proxy); - } + nm_clear_g_dbus_connection_signal (priv->dbus_connection, + &priv->name_owner_changed_id); + + nm_clear_g_dbus_connection_signal (priv->dbus_connection, + &priv->changed_signal_id); G_OBJECT_CLASS (nm_auth_manager_parent_class)->dispose (object); + + g_clear_object (&priv->dbus_connection); } static void From cbdb4981973420c51144a637c0cdc97fc74e5167 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 May 2019 13:10:14 +0200 Subject: [PATCH 27/32] auth-manager: don't watch polkit's D-Bus name and don't emit change signal when NameOwner disconnects PolicyKit is a D-Bus activatable service. I don't think it exits on idle (but maybe it does, it certainly should). Anyway, NetworkManager was watching the NameOwner of polkit and if the name was lost(!) it would emit a NM_AUTH_MANAGER_SIGNAL_CHANGED, which causes the internal code to re-authenticate right away. That means, if you stop policy kit, NetworkManager will ask it right away and D-Bus activate it. This is not right. In fact, we don't have to care about the name owner at all. Whenever we make a request, we just make it and D-Bus activate the service as needed. If polkit starts, it emits a Changed signal that we watch on D-Bus. That is the only moment when we should actually emit NM_AUTH_MANAGER_SIGNAL_CHANGED, not when polkit disconnects. --- src/nm-auth-manager.c | 92 +------------------------------------------ 1 file changed, 1 insertion(+), 91 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index c85a4b819b..6d8aa96104 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -54,9 +54,7 @@ typedef struct { GDBusConnection *dbus_connection; GCancellable *shutdown_cancellable; guint64 call_numid_counter; - guint name_owner_changed_id; guint changed_signal_id; - bool dbus_has_owner:1; bool disposing:1; bool shutting_down:1; bool polkit_enabled_construct_only:1; @@ -432,78 +430,6 @@ nm_auth_manager_check_authorization_cancel (NMAuthManagerCallId *call_id) /*****************************************************************************/ -static void -_emit_changed_signal (NMAuthManager *self) -{ - _LOGD ("emit changed signal"); - g_signal_emit (self, signals[CHANGED_SIGNAL], 0); -} - -static void -name_owner_changed (NMAuthManager *self, - const char *owner, - gboolean force_changed_signal) -{ - NMAuthManagerPrivate *priv = NM_AUTH_MANAGER_GET_PRIVATE (self); - gboolean had_owner; - - owner = nm_str_not_empty (owner); - - if (!owner) - _LOGT ("D-Bus name for PolKit has no owner"); - else - _LOGT ("D-Bus name for PolKit has owner %s", owner); - - had_owner = priv->dbus_has_owner; - priv->dbus_has_owner = !!owner; - - if (!priv->dbus_has_owner) { - if ( had_owner - || force_changed_signal) { - /* when the name disappears, we also want to raise a emit signal. - * When it appears, we raise one already. */ - _emit_changed_signal (self); - } - } -} - -static void -name_owner_changed_cb (GDBusConnection *connection, - const char *sender_name, - const char *object_path, - const char *interface_name, - const char *signal_name, - GVariant *parameters, - gpointer user_data) -{ - const char *new_owner; - - if (!g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)"))) - return; - - g_variant_get (parameters, - "(&s&s&s)", - NULL, - NULL, - &new_owner); - - name_owner_changed (user_data, new_owner, FALSE); -} - -static void -get_name_owner_cb (const char *name_owner, - GError *error, - gpointer user_data) -{ - if ( !name_owner - && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - return; - - /* until now the name-owner state was undecided. Pass TRUE to name_owner_changed() - * so we force emitting a a changed signal (iff we have no name owner). */ - name_owner_changed (user_data, name_owner, TRUE); -} - static void changed_signal_cb (GDBusConnection *connection, const char *sender_name, @@ -516,7 +442,7 @@ changed_signal_cb (GDBusConnection *connection, NMAuthManager *self = user_data; _LOGD ("dbus signal: \"Changed\""); - _emit_changed_signal (self); + g_signal_emit (self, signals[CHANGED_SIGNAL], 0); } /*****************************************************************************/ @@ -617,19 +543,6 @@ constructed (GObject *object) priv->shutdown_cancellable = g_cancellable_new (); - priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection, - POLKIT_SERVICE, - name_owner_changed_cb, - self, - NULL); - - nm_dbus_connection_call_get_name_owner (priv->dbus_connection, - POLKIT_SERVICE, - -1, - priv->shutdown_cancellable, - get_name_owner_cb, - self); - priv->changed_signal_id = g_dbus_connection_signal_subscribe (priv->dbus_connection, POLKIT_SERVICE, POLKIT_INTERFACE, @@ -681,9 +594,6 @@ dispose (GObject *object) nm_clear_g_cancellable (&priv->shutdown_cancellable); - nm_clear_g_dbus_connection_signal (priv->dbus_connection, - &priv->name_owner_changed_id); - nm_clear_g_dbus_connection_signal (priv->dbus_connection, &priv->changed_signal_id); From 83476a3fb64efa0b04630469aa901f99e4de121a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 09:51:09 +0200 Subject: [PATCH 28/32] pacrunner: refactor pacrunner to use GDBusConnection - use GDBusConnection instead of GDBusProxy. - rename "call-id" to "conf-id". It's really not a "call" but configuration that gets added and NMPacrunnerManager ensures that the configuration is send to pacrunner. - let "conf-id" keep a reference to NMPacrunnerManager. For one, when we remove configurations we need to call DestroyProxyConfiguration to remove it again. We cannot just abort the requests but must linger around until our configuration is properly cleaned up. Hence, we anyway cannot destroy the NMPacrunnerManager earlier. With respect to fixing shutdown not to leak anything, this merely means that we must wait (and iterate the main loop) as long as NMPacrunnerManager singleton still exits (that is anyway the plan how to fix shutdown). With these considerations it's also clear that our D-Bus calls must have a stricter timeout: NM_SHUTDOWN_TIMEOUT_MS. This is also nice because nm_pacrunner_manager_remove() no longer needs a manager parameter, it can just rely on having a reference to the manager. - for logging the configuration IDs, don't log pointer values. Logging pointer values should be avoided as it defeats ASLR. Instead, give them a "log_id" number. - pacrunner is a D-Bus activatable service. D-Bus activatable services needs special care. We don't want to start it over and over again. Instead, we only try to "StartServiceByName" if - we have any configuration to add - if pacrunner is currently confirmed not to be running (by watching name owner changes) - we didn't try to start it already. That means, only start it at the beginning and afterwards set a flag to block it. When we see pacrunner appear on D-Bus we always clear that flag, that means if pacrunner drops of, we will try to restart it (once). --- src/NetworkManagerUtils.h | 2 +- src/devices/nm-device.c | 34 +- src/nm-pacrunner-manager.c | 836 +++++++++++++++++++----------------- src/nm-pacrunner-manager.h | 18 +- src/vpn/nm-vpn-connection.c | 28 +- 5 files changed, 477 insertions(+), 441 deletions(-) diff --git a/src/NetworkManagerUtils.h b/src/NetworkManagerUtils.h index 2b1f5ed891..4be5d462b3 100644 --- a/src/NetworkManagerUtils.h +++ b/src/NetworkManagerUtils.h @@ -78,7 +78,7 @@ NMPlatformRoutingRule *nm_ip_routing_rule_to_platform (const NMIPRoutingRule *ru * SIGKILL. * * After NM_SHUTDOWN_TIMEOUT_MS, NetworkManager will however not yet terminate right - * away. It iterates the mainloop for another NM_SHUTDOWN_TIMEOUT_MS_EXTRA. This + * away. It iterates the mainloop for another NM_SHUTDOWN_TIMEOUT_MS_WATCHDOG. This * should give time to reap the child process (after SIGKILL). * * So, the maximum time we should wait before sending SIGKILL should be at most diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 89207586e7..38582937e4 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -397,8 +397,7 @@ typedef struct _NMDevicePrivate { /* Proxy Configuration */ NMProxyConfig *proxy_config; - NMPacrunnerManager *pacrunner_manager; - NMPacrunnerCallId *pacrunner_call_id; + NMPacrunnerConfId *pacrunner_conf_id; /* IP configuration info. Combined config from VPN, settings, and device */ union { @@ -11128,21 +11127,17 @@ nm_device_reactivate_ip6_config (NMDevice *self, } static void -_pacrunner_manager_send (NMDevice *self) +_pacrunner_manager_add (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, - &priv->pacrunner_call_id); + nm_pacrunner_manager_remove_clear (&priv->pacrunner_conf_id); - if (!priv->pacrunner_manager) - priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ()); - - priv->pacrunner_call_id = nm_pacrunner_manager_send (priv->pacrunner_manager, - nm_device_get_ip_iface (self), - priv->proxy_config, - NULL, - NULL); + priv->pacrunner_conf_id = nm_pacrunner_manager_add (nm_pacrunner_manager_get (), + priv->proxy_config, + nm_device_get_ip_iface (self), + NULL, + NULL); } static void @@ -11150,10 +11145,10 @@ reactivate_proxy_config (NMDevice *self) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - if (!priv->pacrunner_call_id) + if (!priv->pacrunner_conf_id) return; nm_device_set_proxy_config (self, priv->dhcp4.pac_url); - _pacrunner_manager_send (self); + _pacrunner_manager_add (self); } static gboolean @@ -15081,8 +15076,7 @@ _set_state_full (NMDevice *self, } } - nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, - &priv->pacrunner_call_id); + nm_pacrunner_manager_remove_clear (&priv->pacrunner_conf_id); break; case NM_DEVICE_STATE_DISCONNECTED: if ( priv->queued_act_request @@ -15102,7 +15096,7 @@ _set_state_full (NMDevice *self, NULL, NULL, NULL); if (priv->proxy_config) - _pacrunner_manager_send (self); + _pacrunner_manager_add (self); break; case NM_DEVICE_STATE_FAILED: /* Usually upon failure the activation chain is interrupted in @@ -16345,9 +16339,7 @@ dispose (GObject *object) dispatcher_cleanup (self); - nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, - &priv->pacrunner_call_id); - g_clear_object (&priv->pacrunner_manager); + nm_pacrunner_manager_remove_clear (&priv->pacrunner_conf_id); _cleanup_generic_pre (self, CLEANUP_TYPE_KEEP); diff --git a/src/nm-pacrunner-manager.c b/src/nm-pacrunner-manager.c index b9881f76f2..1ad9cb0f75 100644 --- a/src/nm-pacrunner-manager.c +++ b/src/nm-pacrunner-manager.c @@ -23,11 +23,14 @@ #include "nm-pacrunner-manager.h" #include "nm-utils.h" +#include "NetworkManagerUtils.h" #include "platform/nm-platform.h" +#include "nm-dbus-manager.h" #include "nm-proxy-config.h" #include "nm-ip4-config.h" #include "nm-ip6-config.h" #include "c-list/src/c-list.h" +#include "nm-glib-aux/nm-dbus-aux.h" #define PACRUNNER_DBUS_SERVICE "org.pacrunner" #define PACRUNNER_DBUS_INTERFACE "org.pacrunner.Manager" @@ -35,25 +38,27 @@ /*****************************************************************************/ -struct _NMPacrunnerCallId { - CList lst; +struct _NMPacrunnerConfId { + CList conf_id_lst; - /* this might be a dangling pointer after the async operation - * is cancelled. */ - NMPacrunnerManager *manager_maybe_dangling; + NMPacrunnerManager *self; + + GVariant *parameters; - GVariant *args; char *path; + guint64 log_id; guint refcount; }; -typedef struct _NMPacrunnerCallId Config; - typedef struct { - char *iface; - GDBusProxy *pacrunner; + GDBusConnection *dbus_connection; GCancellable *cancellable; - CList configs; + CList conf_id_lst_head; + guint64 log_id_counter; + guint name_owner_changed_id; + bool dbus_initied:1; + bool has_name_owner:1; + bool try_start_blocked:1; } NMPacrunnerManagerPrivate; struct _NMPacrunnerManager { @@ -79,332 +84,139 @@ NM_DEFINE_SINGLETON_GETTER (NMPacrunnerManager, nm_pacrunner_manager_get, NM_TYP #define _NMLOG(level, ...) __NMLOG_DEFAULT (level, _NMLOG_DOMAIN, "pacrunner", __VA_ARGS__) #define _NMLOG2_PREFIX_NAME "pacrunner" -#define _NMLOG2(level, config, ...) \ +#define _NMLOG2(level, conf_id, ...) \ G_STMT_START { \ nm_log ((level), _NMLOG_DOMAIN, NULL, NULL, \ - "%s%p]: " _NM_UTILS_MACRO_FIRST(__VA_ARGS__), \ + "%s%"G_GUINT64_FORMAT"]: " _NM_UTILS_MACRO_FIRST (__VA_ARGS__), \ _NMLOG2_PREFIX_NAME": call[", \ - (config) \ - _NM_UTILS_MACRO_REST(__VA_ARGS__)); \ + (conf_id)->log_id \ + _NM_UTILS_MACRO_REST (__VA_ARGS__)); \ } G_STMT_END /*****************************************************************************/ -static void pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data); +static void _call_destroy_proxy_configuration (NMPacrunnerManager *self, + NMPacrunnerConfId *conf_id, + const char *path, + gboolean verbose_log); /*****************************************************************************/ -static Config * -config_new (NMPacrunnerManager *manager, GVariant *args) +static NMPacrunnerConfId * +conf_id_ref (NMPacrunnerConfId *conf_id) { - Config *config; + nm_assert (conf_id); + nm_assert (conf_id->refcount > 0); - config = g_slice_new0 (Config); - config->manager_maybe_dangling = manager; - config->args = g_variant_ref_sink (args); - config->refcount = 1; - c_list_link_tail (&NM_PACRUNNER_MANAGER_GET_PRIVATE (manager)->configs, - &config->lst); - - return config; -} - -static Config * -config_ref (Config *config) -{ - nm_assert (config); - nm_assert (config->refcount > 0); - - config->refcount++; - return config; + conf_id->refcount++; + return conf_id; } static void -config_unref (Config *config) +conf_id_unref (NMPacrunnerConfId *conf_id) { - nm_assert (config); - nm_assert (config->refcount > 0); + nm_assert (conf_id); + nm_assert (conf_id->refcount > 0); - if (config->refcount == 1) { - g_variant_unref (config->args); - g_free (config->path); - c_list_unlink_stale (&config->lst); - g_slice_free (Config, config); + if (conf_id->refcount == 1) { + g_variant_unref (conf_id->parameters); + g_free (conf_id->path); + c_list_unlink_stale (&conf_id->conf_id_lst); + g_object_unref (conf_id->self); + g_slice_free (NMPacrunnerConfId, conf_id); } else - config->refcount--; + conf_id->refcount--; } +NM_AUTO_DEFINE_FCN0 (NMPacrunnerConfId *, _nm_auto_unref_conf_id, conf_id_unref); +#define nm_auto_unref_conf_id nm_auto (_nm_auto_unref_conf_id) + /*****************************************************************************/ static void -add_proxy_config (GVariantBuilder *proxy_data, const NMProxyConfig *proxy_config) -{ - const char *pac_url, *pac_script; - NMProxyConfigMethod method; - - method = nm_proxy_config_get_method (proxy_config); - - if (method == NM_PROXY_CONFIG_METHOD_AUTO) { - pac_url = nm_proxy_config_get_pac_url (proxy_config); - if (pac_url) { - g_variant_builder_add (proxy_data, "{sv}", - "URL", - g_variant_new_string (pac_url)); - } - - pac_script = nm_proxy_config_get_pac_script (proxy_config); - if (pac_script) { - g_variant_builder_add (proxy_data, "{sv}", - "Script", - g_variant_new_string (pac_script)); - } - } - - g_variant_builder_add (proxy_data, "{sv}", - "BrowserOnly", - g_variant_new_boolean (nm_proxy_config_get_browser_only (proxy_config))); -} - -static void -get_ip4_domains (GPtrArray *domains, NMIP4Config *ip4) +get_ip_domains (GPtrArray *domains, NMIPConfig *ip_config) { NMDedupMultiIter ipconf_iter; char *cidr; - const NMPlatformIP4Address *address; - const NMPlatformIP4Route *routes; - guint i; + guint i, num; char sbuf[NM_UTILS_INET_ADDRSTRLEN]; + int addr_family; - /* Extract searches */ - for (i = 0; i < nm_ip4_config_get_num_searches (ip4); i++) - g_ptr_array_add (domains, g_strdup (nm_ip4_config_get_search (ip4, i))); - - /* Extract domains */ - for (i = 0; i < nm_ip4_config_get_num_domains (ip4); i++) - g_ptr_array_add (domains, g_strdup (nm_ip4_config_get_domain (ip4, i))); - - /* Add addresses and routes in CIDR form */ - - nm_ip_config_iter_ip4_address_for_each (&ipconf_iter, ip4, &address) { - cidr = g_strdup_printf ("%s/%u", - nm_utils_inet4_ntop (address->address, sbuf), - address->plen); - g_ptr_array_add (domains, cidr); - } - - nm_ip_config_iter_ip4_route_for_each (&ipconf_iter, ip4, &routes) { - if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (routes)) - continue; - cidr = g_strdup_printf ("%s/%u", - nm_utils_inet4_ntop (routes->network, sbuf), - routes->plen); - g_ptr_array_add (domains, cidr); - } -} - -static void -get_ip6_domains (GPtrArray *domains, NMIP6Config *ip6) -{ - NMDedupMultiIter ipconf_iter; - char *cidr; - const NMPlatformIP6Address *address; - const NMPlatformIP6Route *routes; - guint i; - char sbuf[NM_UTILS_INET_ADDRSTRLEN]; - - /* Extract searches */ - for (i = 0; i < nm_ip6_config_get_num_searches (ip6); i++) - g_ptr_array_add (domains, g_strdup (nm_ip6_config_get_search (ip6, i))); - - /* Extract domains */ - for (i = 0; i < nm_ip6_config_get_num_domains (ip6); i++) - g_ptr_array_add (domains, g_strdup (nm_ip6_config_get_domain (ip6, i))); - - /* Add addresses and routes in CIDR form */ - nm_ip_config_iter_ip6_address_for_each (&ipconf_iter, ip6, &address) { - cidr = g_strdup_printf ("%s/%u", - nm_utils_inet6_ntop (&address->address, sbuf), - address->plen); - g_ptr_array_add (domains, cidr); - } - - nm_ip_config_iter_ip6_route_for_each (&ipconf_iter, ip6, &routes) { - if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (routes)) - continue; - cidr = g_strdup_printf ("%s/%u", - nm_utils_inet6_ntop (&routes->network, sbuf), - routes->plen); - g_ptr_array_add (domains, cidr); - } -} - -/*****************************************************************************/ - -static GCancellable * -_ensure_cancellable (NMPacrunnerManagerPrivate *priv) -{ - if (G_UNLIKELY (!priv->cancellable)) - priv->cancellable = g_cancellable_new (); - return priv->cancellable; -} - -static void -pacrunner_send_done (GObject *source, GAsyncResult *res, gpointer user_data) -{ - Config *config = user_data; - NMPacrunnerManager *self; - NMPacrunnerManagerPrivate *priv; - gs_free_error GError *error = NULL; - gs_unref_variant GVariant *variant = NULL; - const char *path = NULL; - - nm_assert (!config->path); - - variant = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, &error); - if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - goto out; - - self = NM_PACRUNNER_MANAGER (config->manager_maybe_dangling); - priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - - if (!variant) - _LOG2D (config, "sending failed: %s", error->message); - else { - g_variant_get (variant, "(&o)", &path); - - if (c_list_is_empty (&config->lst)) { - _LOG2D (config, "sent (%s), but destroy it right away", path); - g_dbus_proxy_call (priv->pacrunner, - "DestroyProxyConfiguration", - g_variant_new ("(o)", path), - G_DBUS_CALL_FLAGS_NO_AUTO_START, - -1, - _ensure_cancellable (priv), - pacrunner_remove_done, - config_ref (config)); - } else { - _LOG2D (config, "sent (%s)", path); - config->path = g_strdup (path); - } - } - -out: - config_unref (config); -} - -static void -pacrunner_send_config (NMPacrunnerManager *self, Config *config) -{ - NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - - if (priv->pacrunner) { - _LOG2T (config, "sending..."); - - nm_assert (!config->path); - g_dbus_proxy_call (priv->pacrunner, - "CreateProxyConfiguration", - config->args, - G_DBUS_CALL_FLAGS_NO_AUTO_START, - -1, - _ensure_cancellable (priv), - pacrunner_send_done, - config_ref (config)); - } -} - -static void -name_owner_changed (NMPacrunnerManager *self) -{ - NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - gs_free char *owner = NULL; - CList *iter; - - owner = g_dbus_proxy_get_name_owner (priv->pacrunner); - if (owner) { - _LOGD ("name owner appeared (%s)", owner); - c_list_for_each (iter, &priv->configs) - pacrunner_send_config (self, c_list_entry (iter, Config, lst)); - } else { - _LOGD ("name owner disappeared"); - nm_clear_g_cancellable (&priv->cancellable); - c_list_for_each (iter, &priv->configs) - nm_clear_g_free (&c_list_entry (iter, Config, lst)->path); - } -} - -static void -name_owner_changed_cb (GObject *object, - GParamSpec *pspec, - gpointer user_data) -{ - name_owner_changed (user_data); -} - -static void -pacrunner_proxy_cb (GObject *source, GAsyncResult *res, gpointer user_data) -{ - NMPacrunnerManager *self = user_data; - NMPacrunnerManagerPrivate *priv; - gs_free_error GError *error = NULL; - GDBusProxy *proxy; - - proxy = g_dbus_proxy_new_for_bus_finish (res, &error); - if (!proxy) { - if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - _LOGE ("failed to create D-Bus proxy for pacrunner: %s", error->message); + if (!ip_config) return; + + addr_family = nm_ip_config_get_addr_family (ip_config); + + num = nm_ip_config_get_num_searches (ip_config); + for (i = 0; i < num; i++) + g_ptr_array_add (domains, g_strdup (nm_ip_config_get_search (ip_config, i))); + + num = nm_ip_config_get_num_domains (ip_config); + for (i = 0; i < num; i++) + g_ptr_array_add (domains, g_strdup (nm_ip_config_get_domain (ip_config, i))); + + if (addr_family == AF_INET) { + const NMPlatformIP4Address *address; + + nm_ip_config_iter_ip4_address_for_each (&ipconf_iter, (NMIP4Config *) ip_config, &address) { + cidr = g_strdup_printf ("%s/%u", + nm_utils_inet4_ntop (address->address, sbuf), + address->plen); + g_ptr_array_add (domains, cidr); + } + } else { + const NMPlatformIP6Address *address; + + nm_ip_config_iter_ip6_address_for_each (&ipconf_iter, (NMIP6Config *) ip_config, &address) { + cidr = g_strdup_printf ("%s/%u", + nm_utils_inet6_ntop (&address->address, sbuf), + address->plen); + g_ptr_array_add (domains, cidr); + } } - priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + if (addr_family == AF_INET) { + const NMPlatformIP4Route *routes; - priv->pacrunner = proxy; - g_signal_connect (priv->pacrunner, "notify::g-name-owner", - G_CALLBACK (name_owner_changed_cb), self); - name_owner_changed (self); + nm_ip_config_iter_ip4_route_for_each (&ipconf_iter, (NMIP4Config *) ip_config, &routes) { + if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (routes)) + continue; + cidr = g_strdup_printf ("%s/%u", + nm_utils_inet4_ntop (routes->network, sbuf), + routes->plen); + g_ptr_array_add (domains, cidr); + } + } else { + const NMPlatformIP6Route *routes; + + nm_ip_config_iter_ip6_route_for_each (&ipconf_iter, (NMIP6Config *) ip_config, &routes) { + if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (routes)) + continue; + cidr = g_strdup_printf ("%s/%u", + nm_utils_inet6_ntop (&routes->network, sbuf), + routes->plen); + g_ptr_array_add (domains, cidr); + } + } } -/** - * nm_pacrunner_manager_send: - * @self: the #NMPacrunnerManager - * @iface: the iface for the connection or %NULL - * @proxy_config: proxy config of the connection - * @ip4_config: IP4 config of the connection to extract domain info from - * @ip6_config: IP6 config of the connection to extract domain info from - * - * Returns: a #NMPacrunnerCallId call id. The function cannot - * fail and always returns a non NULL pointer. The call-id may - * be used to remove the configuration later via nm_pacrunner_manager_remove(). - * Note that the call-id does not keep the @self instance alive. - * If you plan to remove the configuration later, you must keep - * the instance alive long enough. You can remove the configuration - * at most once using this call call-id. - */ -NMPacrunnerCallId * -nm_pacrunner_manager_send (NMPacrunnerManager *self, - const char *iface, - NMProxyConfig *proxy_config, - NMIP4Config *ip4_config, - NMIP6Config *ip6_config) +static GVariant * +_make_request_create_proxy_configuration (NMProxyConfig *proxy_config, + const char *iface, + NMIP4Config *ip4_config, + NMIP6Config *ip6_config) { - char **strv = NULL; + GVariantBuilder builder; NMProxyConfigMethod method; - NMPacrunnerManagerPrivate *priv; - GVariantBuilder proxy_data; - GPtrArray *domains; - Config *config; + const char *pac_url; + const char *pac_script; - g_return_val_if_fail (NM_IS_PACRUNNER_MANAGER (self), NULL); - g_return_val_if_fail (proxy_config, NULL); + nm_assert (NM_IS_PROXY_CONFIG (proxy_config)); - priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - - g_free (priv->iface); - priv->iface = g_strdup (iface); - - g_variant_builder_init (&proxy_data, G_VARIANT_TYPE_VARDICT); + g_variant_builder_init (&builder, G_VARIANT_TYPE_VARDICT); if (iface) { - g_variant_builder_add (&proxy_data, "{sv}", + g_variant_builder_add (&builder, "{sv}", "Interface", g_variant_new_string (iface)); } @@ -412,167 +224,410 @@ nm_pacrunner_manager_send (NMPacrunnerManager *self, method = nm_proxy_config_get_method (proxy_config); switch (method) { case NM_PROXY_CONFIG_METHOD_AUTO: - g_variant_builder_add (&proxy_data, "{sv}", + g_variant_builder_add (&builder, "{sv}", "Method", g_variant_new_string ("auto")); + pac_url = nm_proxy_config_get_pac_url (proxy_config); + if (pac_url) { + g_variant_builder_add (&builder, "{sv}", + "URL", + g_variant_new_string (pac_url)); + } + + pac_script = nm_proxy_config_get_pac_script (proxy_config); + if (pac_script) { + g_variant_builder_add (&builder, "{sv}", + "Script", + g_variant_new_string (pac_script)); + } break; case NM_PROXY_CONFIG_METHOD_NONE: - g_variant_builder_add (&proxy_data, "{sv}", + g_variant_builder_add (&builder, "{sv}", "Method", g_variant_new_string ("direct")); + break; } - /* Extract stuff from configs */ - add_proxy_config (&proxy_data, proxy_config); + g_variant_builder_add (&builder, "{sv}", + "BrowserOnly", + g_variant_new_boolean (nm_proxy_config_get_browser_only (proxy_config))); if (ip4_config || ip6_config) { + gs_unref_ptrarray GPtrArray *domains = NULL; + domains = g_ptr_array_new_with_free_func (g_free); - if (ip4_config) - get_ip4_domains (domains, ip4_config); - if (ip6_config) - get_ip6_domains (domains, ip6_config); + get_ip_domains (domains, NM_IP_CONFIG_CAST (ip4_config)); + get_ip_domains (domains, NM_IP_CONFIG_CAST (ip6_config)); - g_ptr_array_add (domains, NULL); - strv = (char **) g_ptr_array_free (domains, (domains->len == 1)); - - if (strv) { - g_variant_builder_add (&proxy_data, "{sv}", + if (domains->len > 0) { + g_variant_builder_add (&builder, "{sv}", "Domains", - g_variant_new_strv ((const char *const *) strv, -1)); - g_strfreev (strv); + g_variant_new_strv ((const char *const*) domains->pdata, + domains->len)); } } - config = config_new (self, g_variant_new ("(a{sv})", &proxy_data)); - - { - gs_free char *args_str = NULL; - - _LOG2D (config, "send: new config %s", - (args_str = g_variant_print (config->args, FALSE))); - } - - /* Send if pacrunner is available on bus, otherwise - * config has already been appended above to be - * sent when pacrunner appears. - */ - pacrunner_send_config (self, config); - - return config; + return g_variant_new ("(a{sv})", &builder); } +/*****************************************************************************/ + static void -pacrunner_remove_done (GObject *source, GAsyncResult *res, gpointer user_data) +_call_destroy_proxy_configuration_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) { - Config *config = user_data; + nm_auto_unref_conf_id NMPacrunnerConfId *conf_id = user_data; gs_free_error GError *error = NULL; gs_unref_variant GVariant *ret = NULL; - ret = g_dbus_proxy_call_finish (G_DBUS_PROXY (source), res, &error); + ret = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), res, &error); if (!ret) { if (!g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) - goto out; - _LOG2D (config, "remove failed: %s", error->message); - goto out; + _LOG2T (conf_id, "destroy proxy configuration: failed with %s", error->message); + else + _LOG2T (conf_id, "destroy proxy configuration: cancelled"); + return; + } + _LOG2T (conf_id, "destroy proxy configuration: success"); +} + +static void +_call_create_proxy_configuration_cb (GObject *source, + GAsyncResult *res, + gpointer user_data) +{ + nm_auto_unref_conf_id NMPacrunnerConfId *conf_id = user_data; + NMPacrunnerManager *self = NM_PACRUNNER_MANAGER (conf_id->self); + gs_free_error GError *error = NULL; + gs_unref_variant GVariant *variant = NULL; + const char *path = NULL; + + nm_assert (!conf_id->path); + + variant = g_dbus_connection_call_finish (G_DBUS_CONNECTION (source), res, &error); + + if (!variant) { + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + _LOG2T (conf_id, "create proxy configuration failed: %s", error->message); + else + _LOG2T (conf_id, "create proxy configuration cancelled"); + return; } - _LOG2D (config, "removed"); + g_variant_get (variant, "(&o)", &path); -out: - config_unref (config); + if (c_list_is_empty (&conf_id->conf_id_lst)) { + _LOG2T (conf_id, "create proxy configuration succeeded (%s), but destroy it right away", path); + _call_destroy_proxy_configuration (self, + conf_id, + path, + FALSE); + } else { + _LOG2T (conf_id, "create proxy configuration succeeded (%s)", path); + conf_id->path = g_strdup (path); + } +} + +static void +_call_destroy_proxy_configuration (NMPacrunnerManager *self, + NMPacrunnerConfId *conf_id, + const char *path, + gboolean verbose_log) +{ + NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + + if (verbose_log) + _LOG2T (conf_id, "destroy proxy configuration %s...", path); + + g_dbus_connection_call (priv->dbus_connection, + PACRUNNER_DBUS_SERVICE, + PACRUNNER_DBUS_PATH, + PACRUNNER_DBUS_INTERFACE, + "DestroyProxyConfiguration", + g_variant_new ("(o)", path), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NO_AUTO_START, + NM_SHUTDOWN_TIMEOUT_MS, + priv->cancellable, + _call_destroy_proxy_configuration_cb, + conf_id_ref (conf_id)); +} + +static void +_call_create_proxy_configuration (NMPacrunnerManager *self, + NMPacrunnerConfId *conf_id, + gboolean verbose_log) +{ + NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + + if (verbose_log) + _LOG2T (conf_id, "create proxy configuration..."); + + g_dbus_connection_call (priv->dbus_connection, + PACRUNNER_DBUS_SERVICE, + PACRUNNER_DBUS_PATH, + PACRUNNER_DBUS_INTERFACE, + "CreateProxyConfiguration", + conf_id->parameters, + G_VARIANT_TYPE ("(o)"), + G_DBUS_CALL_FLAGS_NO_AUTO_START, + NM_SHUTDOWN_TIMEOUT_MS, + priv->cancellable, + _call_create_proxy_configuration_cb, + conf_id_ref (conf_id)); +} + +static gboolean +_try_start_service_by_name (NMPacrunnerManager *self) +{ + NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + + if ( priv->try_start_blocked + || !priv->dbus_initied) + return FALSE; + + _LOGD ("try D-Bus activating pacrunner..."); + priv->try_start_blocked = TRUE; + nm_dbus_connection_call_start_service_by_name (priv->dbus_connection, + PACRUNNER_DBUS_SERVICE, + -1, + NULL, + NULL, + NULL); + return TRUE; +} + +/*****************************************************************************/ + +/** + * nm_pacrunner_manager_add: + * @self: the #NMPacrunnerManager + * @proxy_config: proxy config of the connection + * @iface: the iface for the connection or %NULL + * @ip4_config: IP4 config of the connection to extract domain info from + * @ip6_config: IP6 config of the connection to extract domain info from + * + * Returns: a #NMPacrunnerConfId id. The function cannot + * fail and always returns a non NULL pointer. The conf-id may + * be used to remove the configuration later via nm_pacrunner_manager_remove(). + * Note that the conf-id keeps the @self instance alive. + */ +NMPacrunnerConfId * +nm_pacrunner_manager_add (NMPacrunnerManager *self, + NMProxyConfig *proxy_config, + const char *iface, + NMIP4Config *ip4_config, + NMIP6Config *ip6_config) +{ + NMPacrunnerManagerPrivate *priv; + NMPacrunnerConfId *conf_id; + gs_free char *log_msg = NULL; + + g_return_val_if_fail (NM_IS_PACRUNNER_MANAGER (self), NULL); + g_return_val_if_fail (proxy_config, NULL); + + priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + + conf_id = g_slice_new (NMPacrunnerConfId); + *conf_id = (NMPacrunnerConfId) { + .log_id = ++priv->log_id_counter, + .refcount = 1, + .self = g_object_ref (self), + .parameters = g_variant_ref_sink (_make_request_create_proxy_configuration (proxy_config, + iface, + ip4_config, + ip6_config)), + }; + c_list_link_tail (&priv->conf_id_lst_head, + &conf_id->conf_id_lst); + + if (!priv->has_name_owner) { + _LOG2T (conf_id, "add config: %s (%s)", + (log_msg = g_variant_print (conf_id->parameters, FALSE)), + "pacrunner D-Bus service not running"); + _try_start_service_by_name (self); + } else { + _LOG2T (conf_id, "add config: %s (%s)", + (log_msg = g_variant_print (conf_id->parameters, FALSE)), + "create proxy configuration"); + _call_create_proxy_configuration (self, conf_id, FALSE); + } + + return conf_id; } /** * nm_pacrunner_manager_remove: - * @self: the #NMPacrunnerManager - * @call_id: the call-id obtained from nm_pacrunner_manager_send() + * @conf_id: the conf id obtained from nm_pacrunner_manager_add() */ void -nm_pacrunner_manager_remove (NMPacrunnerManager *self, NMPacrunnerCallId *call_id) +nm_pacrunner_manager_remove (NMPacrunnerConfId *conf_id) { + _nm_unused nm_auto_unref_conf_id NMPacrunnerConfId *conf_id_free = conf_id; + NMPacrunnerManager *self; NMPacrunnerManagerPrivate *priv; - Config *config; + + g_return_if_fail (conf_id); + + self = conf_id->self; g_return_if_fail (NM_IS_PACRUNNER_MANAGER (self)); - g_return_if_fail (call_id); - config = call_id; priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - _LOG2T (config, "removing..."); + _LOG2T (conf_id, "removing..."); - nm_assert (c_list_contains (&priv->configs, &config->lst)); + nm_assert (c_list_contains (&priv->conf_id_lst_head, &conf_id->conf_id_lst)); - if (priv->pacrunner) { - if (!config->path) { - /* send() failed or is still pending. The item is unlinked from - * priv->configs, so pacrunner_send_done() knows to call - * DestroyProxyConfiguration right away. - */ - } else { - g_dbus_proxy_call (priv->pacrunner, - "DestroyProxyConfiguration", - g_variant_new ("(o)", config->path), - G_DBUS_CALL_FLAGS_NO_AUTO_START, - -1, - _ensure_cancellable (priv), - pacrunner_remove_done, - config_ref (config)); - nm_clear_g_free (&config->path); - } + c_list_unlink (&conf_id->conf_id_lst); + + if (!conf_id->path) { + /* There is no ID to destroy the configuration. + * + * That can happen because: + * + * - pacrunner D-Bus service is not running (no name owner) and we didn't call CreateProxyConfiguration. + * - CreateProxyConfiguration failed. + * - CreateProxyConfiguration is in progress. + * + * In all cases there is nothing to do. Note that if CreateProxyConfiguration is in progress + * it has a reference on the conf-id and it will automatically destroy the configuration + * when it completes. + */ + return; } - c_list_unlink (&config->lst); - config_unref (config); + _call_destroy_proxy_configuration (self, conf_id, conf_id->path, TRUE); } gboolean -nm_pacrunner_manager_remove_clear (NMPacrunnerManager *self, - NMPacrunnerCallId **p_call_id) +nm_pacrunner_manager_remove_clear (NMPacrunnerConfId **p_conf_id) { - g_return_val_if_fail (p_call_id, FALSE); + g_return_val_if_fail (p_conf_id, FALSE); - /* if we have no call-id, allow for %NULL */ - g_return_val_if_fail ((!self && !*p_call_id) || NM_IS_PACRUNNER_MANAGER (self), FALSE); - - if (!*p_call_id) + if (!*p_conf_id) return FALSE; - nm_pacrunner_manager_remove (self, - g_steal_pointer (p_call_id)); + nm_pacrunner_manager_remove (g_steal_pointer (p_conf_id)); return TRUE; } /*****************************************************************************/ +static void +name_owner_changed (NMPacrunnerManager *self, + const char *name_owner) +{ + NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); + NMPacrunnerConfId *conf_id; + gboolean has_name_owner; + + has_name_owner = (name_owner && name_owner[0]); + + if ( priv->dbus_initied + && priv->has_name_owner == has_name_owner) + return; + + priv->has_name_owner = has_name_owner; + + nm_clear_g_cancellable (&priv->cancellable); + + if (has_name_owner) { + priv->dbus_initied = TRUE; + priv->try_start_blocked = FALSE; + _LOGD ("pacrunner appeared on D-Bus (%s)", name_owner); + priv->cancellable = g_cancellable_new (); + c_list_for_each_entry (conf_id, &priv->conf_id_lst_head, conf_id_lst) + _call_create_proxy_configuration (self, conf_id, TRUE); + } else { + if (!priv->dbus_initied) { + priv->dbus_initied = TRUE; + nm_assert (!priv->try_start_blocked); + _LOGD ("pacrunner not on D-Bus"); + } else + _LOGD ("pacrunner disappeared from D-Bus"); + if (!c_list_is_empty (&priv->conf_id_lst_head)) { + c_list_for_each_entry (conf_id, &priv->conf_id_lst_head, conf_id_lst) + nm_clear_g_free (&conf_id->path); + _try_start_service_by_name (self); + } + } +} + +static void +name_owner_changed_cb (GDBusConnection *connection, + const char *sender_name, + const char *object_path, + const char *interface_name, + const char *signal_name, + GVariant *parameters, + gpointer user_data) +{ + const char *new_owner; + + if (!g_variant_is_of_type (parameters, G_VARIANT_TYPE ("(sss)"))) + return; + + g_variant_get (parameters, + "(&s&s&s)", + NULL, + NULL, + &new_owner); + + name_owner_changed (user_data, new_owner); +} + +static void +get_name_owner_cb (const char *name_owner, + GError *error, + gpointer user_data) +{ + if ( !name_owner + && g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CANCELLED)) + return; + + name_owner_changed (user_data, name_owner); +} + +/*****************************************************************************/ + static void nm_pacrunner_manager_init (NMPacrunnerManager *self) { NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE (self); - c_list_init (&priv->configs); - g_dbus_proxy_new_for_bus (G_BUS_TYPE_SYSTEM, - G_DBUS_PROXY_FLAGS_NONE, - NULL, - PACRUNNER_DBUS_SERVICE, - PACRUNNER_DBUS_PATH, - PACRUNNER_DBUS_INTERFACE, - _ensure_cancellable (priv), - pacrunner_proxy_cb, - self); + c_list_init (&priv->conf_id_lst_head); + + priv->dbus_connection = nm_g_object_ref (NM_MAIN_DBUS_CONNECTION_GET); + + if (!priv->dbus_connection) { + _LOGD ("no D-Bus connection to talk to pacrunner"); + return; + } + + priv->name_owner_changed_id = nm_dbus_connection_signal_subscribe_name_owner_changed (priv->dbus_connection, + PACRUNNER_DBUS_SERVICE, + name_owner_changed_cb, + self, + NULL); + priv->cancellable = g_cancellable_new (); + + nm_dbus_connection_call_get_name_owner (priv->dbus_connection, + PACRUNNER_DBUS_SERVICE, + -1, + priv->cancellable, + get_name_owner_cb, + self); } static void dispose (GObject *object) { NMPacrunnerManagerPrivate *priv = NM_PACRUNNER_MANAGER_GET_PRIVATE ((NMPacrunnerManager *) object); - CList *iter, *safe; - c_list_for_each_safe (iter, safe, &priv->configs) { - c_list_unlink (iter); - config_unref (c_list_entry (iter, Config, lst)); - } + nm_assert (c_list_is_empty (&priv->conf_id_lst_head)); /* we cancel all pending operations. Note that pacrunner automatically * removes all configuration once NetworkManager disconnects from @@ -580,8 +635,9 @@ dispose (GObject *object) */ nm_clear_g_cancellable (&priv->cancellable); - g_clear_pointer (&priv->iface, g_free); - g_clear_object (&priv->pacrunner); + nm_clear_g_dbus_connection_signal (priv->dbus_connection, + &priv->name_owner_changed_id); + g_clear_object (&priv->dbus_connection); G_OBJECT_CLASS (nm_pacrunner_manager_parent_class)->dispose (object); } diff --git a/src/nm-pacrunner-manager.h b/src/nm-pacrunner-manager.h index 3080c4f5fd..35ccb3c351 100644 --- a/src/nm-pacrunner-manager.h +++ b/src/nm-pacrunner-manager.h @@ -31,22 +31,20 @@ typedef struct _NMPacrunnerManagerClass NMPacrunnerManagerClass; -typedef struct _NMPacrunnerCallId NMPacrunnerCallId; +typedef struct _NMPacrunnerConfId NMPacrunnerConfId; GType nm_pacrunner_manager_get_type (void); NMPacrunnerManager *nm_pacrunner_manager_get (void); -NMPacrunnerCallId *nm_pacrunner_manager_send (NMPacrunnerManager *self, - const char *iface, - NMProxyConfig *proxy_config, - NMIP4Config *ip4_config, - NMIP6Config *ip6_config); +NMPacrunnerConfId *nm_pacrunner_manager_add (NMPacrunnerManager *self, + NMProxyConfig *proxy_config, + const char *iface, + NMIP4Config *ip4_config, + NMIP6Config *ip6_config); -void nm_pacrunner_manager_remove (NMPacrunnerManager *self, - NMPacrunnerCallId *call_id); +void nm_pacrunner_manager_remove (NMPacrunnerConfId *conf_id); -gboolean nm_pacrunner_manager_remove_clear (NMPacrunnerManager *self, - NMPacrunnerCallId **p_call_id); +gboolean nm_pacrunner_manager_remove_clear (NMPacrunnerConfId **p_conf_id); #endif /* __NETWORKMANAGER_PACRUNNER_MANAGER_H__ */ diff --git a/src/vpn/nm-vpn-connection.c b/src/vpn/nm-vpn-connection.c index 5acf491a2c..2c4f33359f 100644 --- a/src/vpn/nm-vpn-connection.c +++ b/src/vpn/nm-vpn-connection.c @@ -124,8 +124,7 @@ typedef struct { GVariant *connect_hash; guint connect_timeout; NMProxyConfig *proxy_config; - NMPacrunnerManager *pacrunner_manager; - NMPacrunnerCallId *pacrunner_call_id; + NMPacrunnerConfId *pacrunner_conf_id; gboolean has_ip4; NMIP4Config *ip4_config; guint32 ip4_internal_gw; @@ -552,18 +551,12 @@ _set_vpn_state (NMVpnConnection *self, NULL); if (priv->proxy_config) { - nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, - &priv->pacrunner_call_id); - if (!priv->pacrunner_manager) { - /* the pending call doesn't keep NMPacrunnerManager alive. - * Take a reference to it. */ - priv->pacrunner_manager = g_object_ref (nm_pacrunner_manager_get ()); - } - priv->pacrunner_call_id = nm_pacrunner_manager_send (priv->pacrunner_manager, - priv->ip_iface, - priv->proxy_config, - priv->ip4_config, - priv->ip6_config); + nm_pacrunner_manager_remove_clear (&priv->pacrunner_conf_id); + priv->pacrunner_conf_id = nm_pacrunner_manager_add (nm_pacrunner_manager_get (), + priv->proxy_config, + priv->ip_iface, + priv->ip4_config, + priv->ip6_config); } break; case STATE_DEACTIVATING: @@ -594,8 +587,7 @@ _set_vpn_state (NMVpnConnection *self, } } - nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, - &priv->pacrunner_call_id); + nm_pacrunner_manager_remove_clear (&priv->pacrunner_conf_id); break; case STATE_FAILED: case STATE_DISCONNECTED: @@ -2801,9 +2793,7 @@ dispose (GObject *object) fw_call_cleanup (self); - nm_pacrunner_manager_remove_clear (priv->pacrunner_manager, - &priv->pacrunner_call_id); - g_clear_object (&priv->pacrunner_manager); + nm_pacrunner_manager_remove_clear (&priv->pacrunner_conf_id); G_OBJECT_CLASS (nm_vpn_connection_parent_class)->dispose (object); } From 156f4ee53f0e8007344f68d513bb13958facd72f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 15:00:37 +0200 Subject: [PATCH 29/32] core/pppd-plugin: use GDBusConnection in "nm-pppd-plugin.c" - use GDBusConnection instead of GDBusProxy. - namespace global variables with a "gl" struct. - don't log __func__. If a log line should have a certain topic/tag, the tag should be set manually, not based on the function name. It just looks odd. Also, it's unnecessary. --- src/ppp/nm-pppd-plugin.c | 198 +++++++++++++++++++++------------------ 1 file changed, 105 insertions(+), 93 deletions(-) diff --git a/src/ppp/nm-pppd-plugin.c b/src/ppp/nm-pppd-plugin.c index a8d6749a43..d687ab444f 100644 --- a/src/ppp/nm-pppd-plugin.c +++ b/src/ppp/nm-pppd-plugin.c @@ -46,17 +46,18 @@ int plugin_init (void); char pppd_version[] = VERSION; -static GDBusProxy *proxy = NULL; +static struct { + GDBusConnection *dbus_connection; + char *ipparam; +} gl; static void nm_phasechange (void *data, int arg) { NMPPPStatus ppp_status = NM_PPP_STATUS_UNKNOWN; - char new_name[IF_NAMESIZE]; char *ppp_phase; - int index; - g_return_if_fail (G_IS_DBUS_PROXY (proxy)); + g_return_if_fail (G_IS_DBUS_CONNECTION (gl.dbus_connection)); switch (arg) { case PHASE_DEAD: @@ -117,33 +118,49 @@ nm_phasechange (void *data, int arg) break; } - g_message ("nm-ppp-plugin: (%s): status %d / phase '%s'", - __func__, + g_message ("nm-ppp-plugin: status %d / phase '%s'", ppp_status, ppp_phase); if (ppp_status != NM_PPP_STATUS_UNKNOWN) { - g_dbus_proxy_call (proxy, - "SetState", - g_variant_new ("(u)", ppp_status), - G_DBUS_CALL_FLAGS_NONE, -1, - NULL, - NULL, NULL); + g_dbus_connection_call (gl.dbus_connection, + NM_DBUS_SERVICE, + gl.ipparam, + NM_DBUS_INTERFACE_PPP, + "SetState", + g_variant_new ("(u)", ppp_status), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + NULL, + NULL); } if (ppp_status == NM_PPP_STATUS_RUNNING) { - index = if_nametoindex (ifname); + gs_unref_variant GVariant *ret = NULL; + char new_name[IF_NAMESIZE]; + int ifindex; + + ifindex = if_nametoindex (ifname); + /* Make a sync call to ensure that when the call * terminates the interface already has its final * name. */ - g_dbus_proxy_call_sync (proxy, - "SetIfindex", - g_variant_new ("(i)", index), - G_DBUS_CALL_FLAGS_NONE, - 25000, - NULL, NULL); + ret = g_dbus_connection_call_sync (gl.dbus_connection, + NM_DBUS_SERVICE, + gl.ipparam, + NM_DBUS_INTERFACE_PPP, + "SetIfindex", + g_variant_new ("(i)", ifindex), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + 25000, + NULL, + NULL); + /* Update the name in pppd if NM changed it */ - if ( if_indextoname (index, new_name) + if ( if_indextoname (ifindex, new_name) && !nm_streq0 (ifname, new_name)) { g_message ("nm-ppp-plugin: interface name changed from '%s' to '%s'", ifname, new_name); g_strlcpy (ifname, new_name, IF_NAMESIZE); @@ -159,12 +176,12 @@ nm_ip_up (void *data, int arg) GVariantBuilder builder; guint32 pppd_made_up_address = htonl (0x0a404040 + ifunit); - g_return_if_fail (G_IS_DBUS_PROXY (proxy)); + g_return_if_fail (G_IS_DBUS_CONNECTION (gl.dbus_connection)); - g_message ("nm-ppp-plugin: (%s): ip-up event", __func__); + g_message ("nm-ppp-plugin: ip-up event"); if (!opts.ouraddr) { - g_warning ("nm-ppp-plugin: (%s): didn't receive an internal IP from pppd!", __func__); + g_warning ("nm-ppp-plugin: didn't receive an internal IP from pppd!"); nm_phasechange (NULL, PHASE_DEAD); return; } @@ -235,14 +252,20 @@ nm_ip_up (void *data, int arg) wins, len, sizeof (guint32))); } - g_message ("nm-ppp-plugin: (%s): sending IPv4 config to NetworkManager...", __func__); + g_message ("nm-ppp-plugin: sending IPv4 config to NetworkManager..."); - g_dbus_proxy_call (proxy, - "SetIp4Config", - g_variant_new ("(a{sv})", &builder), - G_DBUS_CALL_FLAGS_NONE, -1, - NULL, - NULL, NULL); + g_dbus_connection_call (gl.dbus_connection, + NM_DBUS_SERVICE, + gl.ipparam, + NM_DBUS_INTERFACE_PPP, + "SetIp4Config", + g_variant_new ("(a{sv})", &builder), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + NULL, + NULL); } static GVariant * @@ -263,9 +286,9 @@ nm_ip6_up (void *data, int arg) ipv6cp_options *go = &ipv6cp_gotoptions[0]; GVariantBuilder builder; - g_return_if_fail (G_IS_DBUS_PROXY (proxy)); + g_return_if_fail (G_IS_DBUS_CONNECTION (gl.dbus_connection)); - g_message ("nm-ppp-plugin: (%s): ip6-up event", __func__); + g_message ("nm-ppp-plugin: ip6-up event"); g_variant_builder_init (&builder, G_VARIANT_TYPE_VARDICT); /* Keep sending the interface name to be backwards compatible @@ -283,14 +306,20 @@ nm_ip6_up (void *data, int arg) /* DNS is done via DHCPv6 or router advertisements */ - g_message ("nm-ppp-plugin: (%s): sending IPv6 config to NetworkManager...", __func__); + g_message ("nm-ppp-plugin: sending IPv6 config to NetworkManager..."); - g_dbus_proxy_call (proxy, - "SetIp6Config", - g_variant_new ("(a{sv})", &builder), - G_DBUS_CALL_FLAGS_NONE, -1, - NULL, - NULL, NULL); + g_dbus_connection_call (gl.dbus_connection, + NM_DBUS_SERVICE, + gl.ipparam, + NM_DBUS_INTERFACE_PPP, + "SetIp6Config", + g_variant_new ("(a{sv})", &builder), + G_VARIANT_TYPE ("()"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + NULL, + NULL); } static int @@ -308,10 +337,10 @@ get_pap_check (void) static int get_credentials (char *username, char *password) { - const char *my_username = NULL; - const char *my_password = NULL; - GVariant *ret; - GError *err = NULL; + gs_unref_variant GVariant *ret = NULL; + gs_free_error GError *error = NULL; + const char *my_username; + const char *my_password; if (!password) { /* pppd is checking pap support; return 1 for supported */ @@ -320,34 +349,33 @@ get_credentials (char *username, char *password) } g_return_val_if_fail (username, -1); - g_return_val_if_fail (G_IS_DBUS_PROXY (proxy), -1); + g_return_val_if_fail (G_IS_DBUS_CONNECTION (gl.dbus_connection), -1); - g_message ("nm-ppp-plugin: (%s): passwd-hook, requesting credentials...", __func__); + g_message ("nm-ppp-plugin: passwd-hook, requesting credentials..."); - ret = g_dbus_proxy_call_sync (proxy, - "NeedSecrets", - NULL, - G_DBUS_CALL_FLAGS_NONE, -1, - NULL, &err); + ret = g_dbus_connection_call_sync (gl.dbus_connection, + NM_DBUS_SERVICE, + gl.ipparam, + NM_DBUS_INTERFACE_PPP, + "NeedSecrets", + NULL, + G_VARIANT_TYPE ("(ss)"), + G_DBUS_CALL_FLAGS_NONE, + -1, + NULL, + &error); if (!ret) { - g_warning ("nm-ppp-plugin: (%s): could not get secrets: %s", - __func__, - err->message); - g_error_free (err); + g_warning ("nm-ppp-plugin: could not get secrets: %s", + error->message); return -1; } - g_message ("nm-ppp-plugin: (%s): got credentials from NetworkManager", __func__); + g_message ("nm-ppp-plugin: got credentials from NetworkManager"); g_variant_get (ret, "(&s&s)", &my_username, &my_password); - if (my_username) - g_strlcpy (username, my_username, MAXNAMELEN); - - if (my_password) - g_strlcpy (password, my_password, MAXSECRETLEN); - - g_variant_unref (ret); + g_strlcpy (username, my_username, MAXNAMELEN); + g_strlcpy (password, my_password, MAXSECRETLEN); return 1; } @@ -355,12 +383,12 @@ get_credentials (char *username, char *password) static void nm_exit_notify (void *data, int arg) { - g_return_if_fail (G_IS_DBUS_PROXY (proxy)); + g_return_if_fail (G_IS_DBUS_CONNECTION (gl.dbus_connection)); - g_message ("nm-ppp-plugin: (%s): cleaning up", __func__); + g_message ("nm-ppp-plugin: cleaning up"); - g_object_unref (proxy); - proxy = NULL; + g_clear_object (&gl.dbus_connection); + nm_clear_g_free (&gl.ipparam); } static void @@ -387,37 +415,21 @@ add_ip6_notifier (void) int plugin_init (void) { - GDBusConnection *bus; - GError *err = NULL; + gs_free_error GError *err = NULL; - g_message ("nm-ppp-plugin: (%s): initializing", __func__); + g_message ("nm-ppp-plugin: initializing"); - bus = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &err); - if (!bus) { - g_warning ("nm-pppd-plugin: (%s): couldn't connect to system bus: %s", - __func__, err->message); - g_error_free (err); + nm_assert (!gl.dbus_connection); + nm_assert (!gl.ipparam); + + gl.dbus_connection = g_bus_get_sync (G_BUS_TYPE_SYSTEM, NULL, &err); + if (!gl.dbus_connection) { + g_warning ("nm-pppd-plugin: couldn't connect to system bus: %s", + err->message); return -1; } - /* NM passes in the object path of the corresponding PPPManager - * object as the 'ipparam' argument to pppd. - */ - proxy = g_dbus_proxy_new_sync (bus, - G_DBUS_PROXY_FLAGS_NONE, - NULL, - NM_DBUS_SERVICE, - ipparam, - NM_DBUS_INTERFACE_PPP, - NULL, &err); - g_object_unref (bus); - - if (!proxy) { - g_warning ("nm-pppd-plugin: (%s): couldn't create D-Bus proxy: %s", - __func__, err->message); - g_error_free (err); - return -1; - } + gl.ipparam = g_strdup (ipparam); chap_passwd_hook = get_credentials; chap_check_hook = get_chap_check; @@ -426,7 +438,7 @@ plugin_init (void) add_notifier (&phasechange, nm_phasechange, NULL); add_notifier (&ip_up_notifier, nm_ip_up, NULL); - add_notifier (&exitnotify, nm_exit_notify, proxy); + add_notifier (&exitnotify, nm_exit_notify, NULL); add_ip6_notifier (); return 0; From 78999f9b611e416dfedd23e23568b2e6df76b7d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 May 2019 07:42:00 +0200 Subject: [PATCH 30/32] shared: add NM_HASH_OBFUSCATE_PTR() macro We want to log pointer values to indicate the related parties of a log message. But we should not, because plain pointer values can be used to defeat ASLR. Instead, we have nm_hash_obfuscate_ptr() to managle a pointer and give a distinct (albeit not 100% unique) 64 bit integer for logging. But for the logging messages to be meaning-full, all related parties must use the same static-seed. Add a macro NM_HASH_OBFUSCATE_PTR() that uses a particular seed. --- shared/nm-glib-aux/nm-hash-utils.h | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/shared/nm-glib-aux/nm-hash-utils.h b/shared/nm-glib-aux/nm-hash-utils.h index 3f622f99fb..af115a7c67 100644 --- a/shared/nm-glib-aux/nm-hash-utils.h +++ b/shared/nm-glib-aux/nm-hash-utils.h @@ -291,7 +291,7 @@ gboolean nm_pstr_equal (gconstpointer a, gconstpointer b); /*****************************************************************************/ -#define NM_HASH_OBFUSCATE_PTR_FMT "%016llx" +#define NM_HASH_OBFUSCATE_PTR_FMT "%016" G_GINT64_MODIFIER "x" /* sometimes we want to log a pointer directly, for providing context/information about * the message that get logged. Logging pointer values directly defeats ASLR, so we should @@ -307,9 +307,19 @@ gboolean nm_pstr_equal (gconstpointer a, gconstpointer b); \ nm_hash_init (&_h, (static_seed)); \ nm_hash_update_val (&_h, _val_obf_ptr); \ - (unsigned long long) nm_hash_complete_u64 (&_h); \ + nm_hash_complete_u64 (&_h); \ }) +/* if you want to log obfuscated pointer for a certain context (like, NMPRuleManager + * logging user-tags), then you are advised to use nm_hash_obfuscate_ptr() with your + * own, unique static-seed. + * + * However, for example the singleton constructors log the obfuscated pointer values + * for all singletons, so they must all be obfuscated with the same seed. So, this + * macro uses a particular static seed that should be used by when comparing pointer + * values in a global context. */ +#define NM_HASH_OBFUSCATE_PTR(ptr) (nm_hash_obfuscate_ptr (1678382159u, ptr)) + /*****************************************************************************/ #endif /* __NM_HASH_UTILS_H__ */ From 58df3f37ea031b77592e6879c7ccea61cbdfd5a0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 May 2019 15:40:04 +0200 Subject: [PATCH 31/32] core: don't log plain pointer values for singletons Logging pointer values allows to defeat ASLR. Don't do that. --- src/nm-auth-manager.c | 3 ++- src/nm-config.c | 3 ++- src/nm-core-utils.c | 6 ++++-- src/nm-core-utils.h | 8 ++++++-- src/nm-manager.c | 3 ++- src/platform/nm-platform.c | 3 ++- 6 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/nm-auth-manager.c b/src/nm-auth-manager.c index 6d8aa96104..f62c1afae8 100644 --- a/src/nm-auth-manager.c +++ b/src/nm-auth-manager.c @@ -575,7 +575,8 @@ nm_auth_manager_setup (gboolean polkit_enabled) singleton_instance = self; nm_singleton_instance_register (); - nm_log_dbg (LOGD_CORE, "setup %s singleton (%p)", "NMAuthManager", singleton_instance); + nm_log_dbg (LOGD_CORE, "setup %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", + "NMAuthManager", NM_HASH_OBFUSCATE_PTR (singleton_instance)); return self; } diff --git a/src/nm-config.c b/src/nm-config.c index 3e82bdec05..9fcd6591d5 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -2709,7 +2709,8 @@ nm_config_setup (const NMConfigCmdLineOptions *cli, char **atomic_section_prefix /* usually, you would not see this logging line because when creating the * NMConfig instance, the logging is not yet set up to print debug message. */ - nm_log_dbg (LOGD_CORE, "setup %s singleton (%p)", "NMConfig", singleton_instance); + nm_log_dbg (LOGD_CORE, "setup %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", + "NMConfig", NM_HASH_OBFUSCATE_PTR (singleton_instance)); } return singleton_instance; } diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index b0cc914e77..02ce8c81c9 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -142,8 +142,10 @@ _nm_singleton_instance_destroy (void) g_object_weak_unref (instance, _nm_singleton_instance_weak_cb, NULL); - if (instance->ref_count > 1) - nm_log_dbg (LOGD_CORE, "disown %s singleton (%p)", G_OBJECT_TYPE_NAME (instance), instance); + if (instance->ref_count > 1) { + nm_log_dbg (LOGD_CORE, "disown %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", + G_OBJECT_TYPE_NAME (instance), NM_HASH_OBFUSCATE_PTR (instance)); + } g_object_unref (instance); } diff --git a/src/nm-core-utils.h b/src/nm-core-utils.h index 1f9996ca80..17af1f51eb 100644 --- a/src/nm-core-utils.h +++ b/src/nm-core-utils.h @@ -42,7 +42,9 @@ static void \ _singleton_instance_weak_ref_cb (gpointer data, \ GObject *where_the_object_was) \ { \ - nm_log_dbg (LOGD_CORE, "disposing %s singleton (%p)", G_STRINGIFY (TYPE), singleton_instance); \ + nm_log_dbg (LOGD_CORE, "disposing %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", \ + G_STRINGIFY (TYPE), \ + NM_HASH_OBFUSCATE_PTR (singleton_instance)); \ singleton_instance = NULL; \ } \ static inline void \ @@ -73,7 +75,9 @@ GETTER (void) \ singleton_instance = (g_object_new (GTYPE, ##__VA_ARGS__, NULL)); \ g_assert (singleton_instance); \ nm_singleton_instance_register (); \ - nm_log_dbg (LOGD_CORE, "create %s singleton (%p)", G_STRINGIFY (TYPE), singleton_instance); \ + nm_log_dbg (LOGD_CORE, "create %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", \ + G_STRINGIFY (TYPE), \ + NM_HASH_OBFUSCATE_PTR (singleton_instance)); \ } \ return singleton_instance; \ } \ diff --git a/src/nm-manager.c b/src/nm-manager.c index 90bc69bfbb..33d847e66c 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -7272,7 +7272,8 @@ nm_manager_setup (void) singleton_instance = self; nm_singleton_instance_register (); - _LOGD (LOGD_CORE, "setup %s singleton (%p)", "NMManager", singleton_instance); + nm_log_dbg (LOGD_CORE, "setup %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", + "NMManager", NM_HASH_OBFUSCATE_PTR (singleton_instance)); nm_dbus_object_export (NM_DBUS_OBJECT (self)); return self; diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 7add7dcdbe..a2d4116de1 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -221,7 +221,8 @@ nm_platform_setup (NMPlatform *instance) nm_singleton_instance_register (); - nm_log_dbg (LOGD_CORE, "setup %s singleton (%p, %s)", "NMPlatform", singleton_instance, G_OBJECT_TYPE_NAME (instance)); + nm_log_dbg (LOGD_CORE, "setup %s singleton ("NM_HASH_OBFUSCATE_PTR_FMT")", + "NMPlatform", NM_HASH_OBFUSCATE_PTR (instance)); } /** From 3712e7a89fd2efc9b0d2c09cda766d2357a63ba9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 10 May 2019 08:03:54 +0200 Subject: [PATCH 32/32] shared: use union instead of _nm_alignas() for static hash-seed We want the the hash-seed array is alined so it can be used both as guint, guint32, and guint64 directly. Don't use _nm_alignas() but instead just add the fields to the union so we get proper alignment. While at at, also let the seed argument to c_siphash_init() be aligned to 64 integers. c_siphash_init() does not require that, but it tries to read the seed as (unaligned) LE 64 bit integers. So, it doesn't hurt. --- shared/nm-glib-aux/nm-hash-utils.c | 16 +++++++++++----- shared/nm-glib-aux/nm-hash-utils.h | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/shared/nm-glib-aux/nm-hash-utils.c b/shared/nm-glib-aux/nm-hash-utils.c index 6e728e6b20..3ab39db712 100644 --- a/shared/nm-glib-aux/nm-hash-utils.c +++ b/shared/nm-glib-aux/nm-hash-utils.c @@ -45,7 +45,10 @@ _get_hash_key_init (void) * to use it as guint* or guint64* pointer. */ static union { guint8 v8[HASH_KEY_SIZE]; - } g_arr _nm_alignas (guint64); + guint _align_as_uint; + guint32 _align_as_uint32; + guint64 _align_as_uint64; + } g_arr; const guint8 *g; union { guint8 v8[HASH_KEY_SIZE]; @@ -125,14 +128,17 @@ void nm_hash_siphash42_init (CSipHash *h, guint static_seed) { const guint8 *g; - guint seed[HASH_KEY_SIZE_GUINT]; + union { + guint64 _align_as_uint64; + guint arr[HASH_KEY_SIZE_GUINT]; + } seed; nm_assert (h); g = _get_hash_key (); - memcpy (seed, g, HASH_KEY_SIZE); - seed[0] ^= static_seed; - c_siphash_init (h, (const guint8 *) seed); + memcpy (&seed, g, HASH_KEY_SIZE); + seed.arr[0] ^= static_seed; + c_siphash_init (h, (const guint8 *) &seed); } guint diff --git a/shared/nm-glib-aux/nm-hash-utils.h b/shared/nm-glib-aux/nm-hash-utils.h index af115a7c67..07518868cf 100644 --- a/shared/nm-glib-aux/nm-hash-utils.h +++ b/shared/nm-glib-aux/nm-hash-utils.h @@ -34,7 +34,7 @@ void nm_hash_siphash42_init (CSipHash *h, guint static_seed); * * Note, that this is guaranteed to use siphash42 under the hood (contrary to * all other NMHash API, which leave this undefined). That matters at the point, - * where the caller needs to be sure that a reasonably strong hasing algorithm + * where the caller needs to be sure that a reasonably strong hashing algorithm * is used. (Yes, NMHash is all about siphash24, but otherwise that is not promised * anywhere). *