colin/NetworkManager
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

core: don't call nm_auth_chain_destroy() from the callback

Browse Source 
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).
This commit is contained in:
Thomas Haller
2019-05-02 10:08:09 +02:00
parent aab06c7c4b
commit 89d0fdfa36
4 changed files with 57 additions and 85 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/nm-auth-utils.c
View File

@@ -355,11 +355,14 @@ nm_auth_chain_new_subject (NMAuthSubject *subject,
* Destroys the auth-chain. By destroying the auth-chain, you also cancel * 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 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 only exception is, you may call nm_auth_chain_destroy() from inside
* the callback. In this case, @self stays alive until the callback returns. * 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 * 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 void
nm_auth_chain_destroy (NMAuthChain *self) nm_auth_chain_destroy (NMAuthChain *self)

97
src/nm-manager.c
View File

@@ -1142,7 +1142,7 @@ _reload_auth_cb (NMAuthChain *chain,
char s_buf[60]; char s_buf[60];
NMConfigChangeFlags reload_type = NM_CONFIG_CHANGE_NONE; 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); priv->auth_chains = g_slist_remove (priv->auth_chains, chain);
flags = GPOINTER_TO_UINT (nm_auth_chain_get_data (chain, "flags")); flags = GPOINTER_TO_UINT (nm_auth_chain_get_data (chain, "flags"));
@@ -1188,14 +1188,11 @@ _reload_auth_cb (NMAuthChain *chain,
if (ret_error) { if (ret_error) {
g_dbus_method_invocation_take_error (context, ret_error); g_dbus_method_invocation_take_error (context, ret_error);
goto out; return;
} }
nm_config_reload (priv->config, reload_type, TRUE); nm_config_reload (priv->config, reload_type, TRUE);
g_dbus_method_invocation_return_value (context, NULL); g_dbus_method_invocation_return_value (context, NULL);
out:
nm_auth_chain_destroy (chain);
} }
static void static void
@@ -2344,23 +2341,23 @@ device_auth_done_cb (NMAuthChain *chain,
{ {
NMManager *self = NM_MANAGER (user_data); NMManager *self = NM_MANAGER (user_data);
NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self);
GError *error = NULL; gs_free_error GError *error = NULL;
NMAuthCallResult result; NMAuthCallResult result;
NMDevice *device; NMDevice *device;
const char *permission; const char *permission;
NMDeviceAuthRequestFunc callback; NMDeviceAuthRequestFunc callback;
NMAuthSubject *subject; NMAuthSubject *subject;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->auth_chains = g_slist_remove (priv->auth_chains, 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, "requested-permission");
g_assert (permission); nm_assert (permission);
callback = nm_auth_chain_get_data (chain, "callback"); callback = nm_auth_chain_get_data (chain, "callback");
g_assert (callback); nm_assert (callback);
device = nm_auth_chain_get_data (chain, "device"); 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); result = nm_auth_chain_get_result (chain, permission);
subject = nm_auth_chain_get_subject (chain); subject = nm_auth_chain_get_subject (chain);
@@ -2380,16 +2377,13 @@ device_auth_done_cb (NMAuthChain *chain,
permission); permission);
} }
g_assert (error || (result == NM_AUTH_CALL_RESULT_YES)); nm_assert (error || (result == NM_AUTH_CALL_RESULT_YES));
callback (device, callback (device,
context, context,
subject, subject,
error, error,
nm_auth_chain_get_data (chain, "user-data")); nm_auth_chain_get_data (chain, "user-data"));
g_clear_error (&error);
nm_auth_chain_destroy (chain);
} }
static void static void
@@ -5636,7 +5630,7 @@ deactivate_net_auth_done_cb (NMAuthChain *chain,
NMActiveConnection *active; NMActiveConnection *active;
char *path; char *path;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->auth_chains = g_slist_remove (priv->auth_chains, chain); 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); g_dbus_method_invocation_take_error (context, error);
else else
g_dbus_method_invocation_return_value (context, NULL); g_dbus_method_invocation_return_value (context, NULL);
nm_auth_chain_destroy (chain);
} }
static void static void
@@ -6069,7 +6061,7 @@ enable_net_done_cb (NMAuthChain *chain,
gboolean enable; gboolean enable;
NMAuthSubject *subject; NMAuthSubject *subject;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->auth_chains = g_slist_remove (priv->auth_chains, chain); priv->auth_chains = g_slist_remove (priv->auth_chains, chain);
enable = GPOINTER_TO_UINT (nm_auth_chain_get_data (chain, "enable")); 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, ret_error = g_error_new_literal (NM_MANAGER_ERROR,
NM_MANAGER_ERROR_PERMISSION_DENIED, NM_MANAGER_ERROR_PERMISSION_DENIED,
"Not authorized to enable/disable networking"); "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) { if (ret_error) {
nm_audit_log_control_op (NM_AUDIT_OP_NET_CONTROL, enable ? "on" : "off", FALSE, nm_audit_log_control_op (NM_AUDIT_OP_NET_CONTROL, enable ? "on" : "off", FALSE,
subject, ret_error->message); subject, ret_error->message);
g_dbus_method_invocation_take_error (context, ret_error); 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 static void
@@ -6174,7 +6163,7 @@ get_permissions_done_cb (NMAuthChain *chain,
GError *ret_error; GError *ret_error;
GVariantBuilder results; GVariantBuilder results;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->auth_chains = g_slist_remove (priv->auth_chains, chain); priv->auth_chains = g_slist_remove (priv->auth_chains, chain);
if (error) { if (error) {
@@ -6184,31 +6173,30 @@ get_permissions_done_cb (NMAuthChain *chain,
"Permissions request failed: %s", "Permissions request failed: %s",
error->message); error->message);
g_dbus_method_invocation_take_error (context, ret_error); g_dbus_method_invocation_take_error (context, ret_error);
} else { return;
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));
} }
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 static void
@@ -6400,10 +6388,9 @@ check_connectivity_auth_done_cb (NMAuthChain *chain,
NM_MANAGER_ERROR_PERMISSION_DENIED, NM_MANAGER_ERROR_PERMISSION_DENIED,
"Not authorized to recheck connectivity"); "Not authorized to recheck connectivity");
} }
if (error) { if (error) {
g_dbus_method_invocation_take_error (context, error); g_dbus_method_invocation_take_error (context, error);
goto out; return;
} }
data = g_slice_new (ConnectivityCheckData); data = g_slice_new (ConnectivityCheckData);
@@ -6434,9 +6421,6 @@ check_connectivity_auth_done_cb (NMAuthChain *chain,
data); data);
/* @data got destroyed. */ /* @data got destroyed. */
} }
out:
nm_auth_chain_destroy (chain);
} }
static void static void
@@ -6875,7 +6859,6 @@ out:
g_dbus_method_invocation_return_dbus_error (invocation, error_name, error_message); g_dbus_method_invocation_return_dbus_error (invocation, error_name, error_message);
else else
g_dbus_method_invocation_return_value (invocation, NULL); g_dbus_method_invocation_return_value (invocation, NULL);
nm_auth_chain_destroy (chain);
} }
void void
@@ -7009,8 +6992,6 @@ checkpoint_auth_done_cb (NMAuthChain *chain,
g_dbus_method_invocation_take_error (context, error); g_dbus_method_invocation_take_error (context, error);
else else
g_dbus_method_invocation_return_value (context, variant); g_dbus_method_invocation_return_value (context, variant);
nm_auth_chain_destroy (chain);
} }
static void static void

18
src/settings/nm-agent-manager.c
View File

@@ -323,7 +323,7 @@ agent_register_permissions_done (NMAuthChain *chain,
NMAuthCallResult result; NMAuthCallResult result;
CList *iter; CList *iter;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->chains = g_slist_remove (priv->chains, chain); 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) c_list_for_each (iter, &priv->requests)
request_add_agent (c_list_entry (iter, Request, lst_request), agent); request_add_agent (c_list_entry (iter, Request, lst_request), agent);
} }
nm_auth_chain_destroy (chain);
} }
static NMSecretAgent * static NMSecretAgent *
@@ -537,8 +535,7 @@ request_free (Request *req)
case REQUEST_TYPE_CON_DEL: case REQUEST_TYPE_CON_DEL:
g_object_unref (req->con.connection); g_object_unref (req->con.connection);
g_free (req->con.path); g_free (req->con.path);
if (req->con.chain) nm_clear_pointer (&req->con.chain, nm_auth_chain_destroy);
nm_auth_chain_destroy (req->con.chain);
if (req->request_type == REQUEST_TYPE_CON_GET) { if (req->request_type == REQUEST_TYPE_CON_GET) {
g_free (req->con.get.setting_name); g_free (req->con.get.setting_name);
g_strfreev (req->con.get.hints); 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_GET:
case REQUEST_TYPE_CON_SAVE: case REQUEST_TYPE_CON_SAVE:
case REQUEST_TYPE_CON_DEL: case REQUEST_TYPE_CON_DEL:
if (req->con.chain) { /* This cancels the pending authorization requests. */
/* This cancels the pending authorization requests. */ nm_clear_pointer (&req->con.chain, nm_auth_chain_destroy);
nm_auth_chain_destroy (req->con.chain);
req->con.chain = NULL;
}
break; break;
default: default:
g_assert_not_reached (); 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); _con_get_request_start_proceed (req, req->con.current_has_modify);
} }
nm_auth_chain_destroy (chain);
} }
static void 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_PROTECTED, share_protected);
nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, share_open); nm_secret_agent_add_permission (agent, NM_AUTH_PERMISSION_WIFI_SHARE_OPEN, share_open);
nm_auth_chain_destroy (chain);
} }
static void static void

18
src/settings/nm-settings.c
View File

@@ -1141,7 +1141,7 @@ pk_add_cb (NMAuthChain *chain,
NMSettings *self = NM_SETTINGS (user_data); NMSettings *self = NM_SETTINGS (user_data);
NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self); NMSettingsPrivate *priv = NM_SETTINGS_GET_PRIVATE (self);
NMAuthCallResult result; NMAuthCallResult result;
GError *error = NULL; gs_free_error GError *error = NULL;
NMConnection *connection = NULL; NMConnection *connection = NULL;
gs_unref_object NMSettingsConnection *added = NULL; gs_unref_object NMSettingsConnection *added = NULL;
NMSettingsAddCallback callback; NMSettingsAddCallback callback;
@@ -1150,12 +1150,13 @@ pk_add_cb (NMAuthChain *chain,
const char *perm; const char *perm;
gboolean save_to_disk; gboolean save_to_disk;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->auths = g_slist_remove (priv->auths, chain); priv->auths = g_slist_remove (priv->auths, chain);
perm = nm_auth_chain_get_data (chain, "perm"); perm = nm_auth_chain_get_data (chain, "perm");
g_assert (perm); nm_assert (perm);
result = nm_auth_chain_get_result (chain, perm); result = nm_auth_chain_get_result (chain, perm);
if (chain_error) { if (chain_error) {
@@ -1189,11 +1190,10 @@ pk_add_cb (NMAuthChain *chain,
callback (self, added, error, context, subject, callback_data); callback (self, added, error, context, subject, callback_data);
/* Send agent-owned secrets to the agents */ /* 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); send_agent_owned_secrets (self, added, subject);
g_clear_error (&error);
nm_auth_chain_destroy (chain);
} }
/* FIXME: remove if/when kernel supports adhoc wpa */ /* FIXME: remove if/when kernel supports adhoc wpa */
@@ -1513,7 +1513,7 @@ pk_hostname_cb (NMAuthChain *chain,
GError *error = NULL; GError *error = NULL;
const char *hostname; const char *hostname;
g_assert (context); nm_assert (G_IS_DBUS_METHOD_INVOCATION (context));
priv->auths = g_slist_remove (priv->auths, chain); 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); g_dbus_method_invocation_take_error (context, error);
else else
g_dbus_method_invocation_return_value (context, NULL); g_dbus_method_invocation_return_value (context, NULL);
nm_auth_chain_destroy (chain);
} }
static void static void