From 89d0fdfa36f31360418cd91b59a3dac5e7371b7d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 2 May 2019 10:08:09 +0200 Subject: [PATCH] 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