[PATCH 1/1] libnm-util: don't assert in nm_setting_get_secret_flags() and avoid assertion in agent_secrets_done_cb()
- From: Thomas Haller <thaller redhat com>
- To: networkmanager-list gnome org
- Subject: [PATCH 1/1] libnm-util: don't assert in nm_setting_get_secret_flags() and avoid assertion in agent_secrets_done_cb()
- Date: Sun, 23 Feb 2014 18:45:03 +0100
When secret providers return the connection hash in GetSecrets(),
this hash also contains non-secret properties.
for_each_secret() iterated over all entries of the @secrets hash
and triggered the assertion in nm_setting_get_secret_flags() (see
below).
NM should not assert against user provided input. Change
nm_setting_get_secret_flags() to silently return FALSE, if the property
is not a secret. This makes nm_setting_get_secret_flags() useful to
check whether a property is actually a secret property.
Adjust all calls to nm_setting_get_secret_flags(), so that we assert
where appropriate.
Also, agent_secrets_done_cb() clears now all non-secrets properties
from the hash, using the new argument @remove_non_secrets when calling
for_each_secret().
#0 0x0000003370c504e9 in g_logv () from /lib64/libglib-2.0.so.0
#1 0x0000003370c5063f in g_log () from /lib64/libglib-2.0.so.0
#2 0x00007fa4b0c1c156 in get_secret_flags (setting=0x1e3ac60, secret_name=0x1ea9180 "security",
verify_secret=1, out_flags=0x7fff7507857c, error=0x0) at nm-setting.c:1091
#3 0x00007fa4b0c1c2b2 in nm_setting_get_secret_flags (setting=0x1e3ac60, secret_name=0x1ea9180 "security",
out_flags=0x7fff7507857c, error=0x0) at nm-setting.c:1124
#4 0x0000000000463d03 in for_each_secret (connection=0x1deb2f0, secrets=0x1e9f860, callback=0x464f1b
<has_system_owned_secrets>, callback_data=0x7fff7507865c) at settings/nm-settings-connection.c:203
#5 0x000000000046525f in agent_secrets_done_cb (manager=0x1dddf50, call_id=1, agent_dbus_owner=0x1ddb9e0
":1.39", agent_username=0x1e51710 "thom", agent_has_modify=1, setting_name=0x1e91f90
"802-11-wireless-security",
flags=NM_SETTINGS_GET_SECRETS_FLAG_ALLOW_INTERACTION, secrets=0x1e9f860, error=0x0,
user_data=0x1deb2f0, other_data2=0x477d61 <get_secrets_cb>, other_data3=0x1ea92a0) at
settings/nm-settings-connection.c:757
#6 0x00000000004dc4fd in get_complete_cb (parent=0x1ea6300, secrets=0x1e9f860, agent_dbus_owner=0x1ddb9e0
":1.39", agent_username=0x1e51710 "thom", error=0x0, user_data=0x1dddf50) at settings/nm-agent-manager.c:1139
#7 0x00000000004dab54 in req_complete_success (req=0x1ea6300, secrets=0x1e9f860,
agent_dbus_owner=0x1ddb9e0 ":1.39", agent_uname=0x1e51710 "thom") at settings/nm-agent-manager.c:502
#8 0x00000000004db86e in get_done_cb (agent=0x1e89530, call_id=0x1, secrets=0x1e9f860, error=0x0,
user_data=0x1ea6300) at settings/nm-agent-manager.c:856
#9 0x00000000004de9d0 in get_callback (proxy=0x1e47530, call=0x1, user_data=0x1ea10f0) at
settings/nm-secret-agent.c:267
#10 0x000000337380cad2 in complete_pending_call_and_unlock () from /lib64/libdbus-1.so.3
#11 0x000000337380fdc1 in dbus_connection_dispatch () from /lib64/libdbus-1.so.3
#12 0x000000342800ad65 in message_queue_dispatch () from /lib64/libdbus-glib-1.so.2
#13 0x0000003370c492a6 in g_main_context_dispatch () from /lib64/libglib-2.0.so.0
#14 0x0000003370c49628 in g_main_context_iterate.isra.24 () from /lib64/libglib-2.0.so.0
#15 0x0000003370c49a3a in g_main_loop_run () from /lib64/libglib-2.0.so.0
#16 0x000000000042e5c6 in main (argc=1, argv=0x7fff75078e88) at main.c:644
Signed-off-by: Thomas Haller <thaller redhat com>
---
libnm-util/nm-setting-vpn.c | 13 +++++++++---
libnm-util/nm-setting.c | 20 ++++++++++++------
src/devices/nm-device-wifi.c | 18 +++++++++--------
src/settings/nm-agent-manager.c | 6 ++++--
src/settings/nm-settings-connection.c | 28 ++++++++++++++++++--------
src/settings/plugins/ifnet/connection_parser.c | 3 ++-
src/settings/plugins/keyfile/writer.c | 11 ++++++++--
7 files changed, 69 insertions(+), 30 deletions(-)
diff --git a/libnm-util/nm-setting-vpn.c b/libnm-util/nm-setting-vpn.c
index b5cffc1..e88321d 100644
--- a/libnm-util/nm-setting-vpn.c
+++ b/libnm-util/nm-setting-vpn.c
@@ -612,8 +612,10 @@ compare_one_secret (NMSettingVPN *a,
NMSettingSecretFlags a_secret_flags = NM_SETTING_SECRET_FLAG_NONE;
NMSettingSecretFlags b_secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (NM_SETTING (a), key, &a_secret_flags, NULL);
- nm_setting_get_secret_flags (NM_SETTING (b), key, &b_secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (NM_SETTING (a), key, &a_secret_flags, NULL))
+ g_return_val_if_reached (FALSE);
+ if (!nm_setting_get_secret_flags (NM_SETTING (b), key, &b_secret_flags, NULL))
+ g_return_val_if_reached (FALSE);
/* If the secret flags aren't the same, the settings aren't the same */
if (a_secret_flags != b_secret_flags)
@@ -667,6 +669,7 @@ clear_secrets_with_flags (NMSetting *setting,
GHashTableIter iter;
const char *secret;
gboolean changed = TRUE;
+ gboolean invalid_property = FALSE;
if (priv->secrets == NULL)
return FALSE;
@@ -676,7 +679,10 @@ clear_secrets_with_flags (NMSetting *setting,
while (g_hash_table_iter_next (&iter, (gpointer) &secret, NULL)) {
NMSettingSecretFlags flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, secret, &flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, secret, &flags, NULL)) {
+ invalid_property = TRUE;
+ continue;
+ }
if (func (setting, pspec->name, flags, user_data) == TRUE) {
g_hash_table_iter_remove (&iter);
changed = TRUE;
@@ -686,6 +692,7 @@ clear_secrets_with_flags (NMSetting *setting,
if (changed)
g_object_notify (G_OBJECT (setting), NM_SETTING_VPN_SECRETS);
+ g_return_val_if_fail (!invalid_property, changed); (void) invalid_property;
return changed;
}
diff --git a/libnm-util/nm-setting.c b/libnm-util/nm-setting.c
index 8b2ee5d..c87408f 100644
--- a/libnm-util/nm-setting.c
+++ b/libnm-util/nm-setting.c
@@ -533,8 +533,10 @@ compare_property (NMSetting *setting,
NMSettingSecretFlags a_secret_flags = NM_SETTING_SECRET_FLAG_NONE;
NMSettingSecretFlags b_secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, prop_spec->name, &a_secret_flags, NULL);
- nm_setting_get_secret_flags (other, prop_spec->name, &b_secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, prop_spec->name, &a_secret_flags, NULL))
+ g_return_val_if_reached (FALSE);
+ if (!nm_setting_get_secret_flags (other, prop_spec->name, &b_secret_flags, NULL))
+ g_return_val_if_reached (FALSE);
/* If the secret flags aren't the same the settings aren't the same */
if (a_secret_flags != b_secret_flags)
@@ -639,7 +641,8 @@ should_compare_prop (NMSetting *setting,
if (comp_flags & NM_SETTING_COMPARE_FLAG_IGNORE_SECRETS)
return FALSE;
- nm_setting_get_secret_flags (setting, prop_name, &secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, prop_name, &secret_flags, NULL))
+ g_return_val_if_reached (FALSE);
if ( (comp_flags & NM_SETTING_COMPARE_FLAG_IGNORE_AGENT_OWNED_SECRETS)
&& (secret_flags & NM_SETTING_SECRET_FLAG_AGENT_OWNED))
@@ -862,7 +865,9 @@ clear_secrets_with_flags (NMSetting *setting,
gboolean changed = FALSE;
/* Clear the secret if the user function says to do so */
- nm_setting_get_secret_flags (setting, pspec->name, &flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, pspec->name, &flags, NULL))
+ g_return_val_if_reached (FALSE);
+
if (func (setting, pspec->name, flags, user_data) == TRUE) {
GValue value = G_VALUE_INIT;
@@ -1087,8 +1092,11 @@ get_secret_flags (NMSetting *setting,
char *flags_prop;
NMSettingSecretFlags flags = NM_SETTING_SECRET_FLAG_NONE;
- if (verify_secret)
- g_return_val_if_fail (is_secret_prop (setting, secret_name, error), FALSE);
+ if (verify_secret && !is_secret_prop (setting, secret_name, error)) {
+ if (out_flags)
+ *out_flags = NM_SETTING_SECRET_FLAG_NONE;
+ return FALSE;
+ }
flags_prop = g_strdup_printf ("%s-flags", secret_name);
g_object_get (G_OBJECT (setting), flags_prop, &flags, NULL);
diff --git a/src/devices/nm-device-wifi.c b/src/devices/nm-device-wifi.c
index fe4c7e0..81f4104 100644
--- a/src/devices/nm-device-wifi.c
+++ b/src/devices/nm-device-wifi.c
@@ -2178,10 +2178,11 @@ need_new_8021x_secrets (NMDeviceWifi *self,
s_8021x = nm_connection_get_setting_802_1x (connection);
if (s_8021x) {
- nm_setting_get_secret_flags (NM_SETTING (s_8021x),
- NM_SETTING_802_1X_PASSWORD,
- &secret_flags,
- NULL);
+ if (!nm_setting_get_secret_flags (NM_SETTING (s_8021x),
+ NM_SETTING_802_1X_PASSWORD,
+ &secret_flags,
+ NULL))
+ g_assert_not_reached ();
if (secret_flags & NM_SETTING_SECRET_FLAG_NOT_SAVED)
*setting_name = NM_SETTING_802_1X_SETTING_NAME;
return *setting_name ? TRUE : FALSE;
@@ -2189,10 +2190,11 @@ need_new_8021x_secrets (NMDeviceWifi *self,
s_wsec = nm_connection_get_setting_wireless_security (connection);
if (s_wsec) {
- nm_setting_get_secret_flags (NM_SETTING (s_wsec),
- NM_SETTING_WIRELESS_SECURITY_LEAP_PASSWORD,
- &secret_flags,
- NULL);
+ if (!nm_setting_get_secret_flags (NM_SETTING (s_wsec),
+ NM_SETTING_WIRELESS_SECURITY_LEAP_PASSWORD,
+ &secret_flags,
+ NULL))
+ g_assert_not_reached ();
if (secret_flags & NM_SETTING_SECRET_FLAG_NOT_SAVED)
*setting_name = NM_SETTING_WIRELESS_SECURITY_SETTING_NAME;
return *setting_name ? TRUE : FALSE;
diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c
index ae93069..b158243 100644
--- a/src/settings/nm-agent-manager.c
+++ b/src/settings/nm-agent-manager.c
@@ -996,12 +996,14 @@ check_system_secrets_cb (NMSetting *setting,
g_hash_table_iter_init (&iter, (GHashTable *) g_value_get_boxed (value));
while (g_hash_table_iter_next (&iter, (gpointer *) &secret_name, NULL)) {
secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, secret_name, &secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, secret_name, &secret_flags, NULL))
+ g_return_if_reached ();
if (secret_flags == NM_SETTING_SECRET_FLAG_NONE)
*has_system = TRUE;
}
} else {
- nm_setting_get_secret_flags (setting, key, &secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, key, &secret_flags, NULL))
+ g_return_if_reached ();
if (secret_flags == NM_SETTING_SECRET_FLAG_NONE)
*has_system = TRUE;
}
diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c
index 095d033..52138e2 100644
--- a/src/settings/nm-settings-connection.c
+++ b/src/settings/nm-settings-connection.c
@@ -141,6 +141,7 @@ typedef gboolean (*ForEachSecretFunc) (GHashTableIter *iter,
static void
for_each_secret (NMConnection *connection,
GHashTable *secrets,
+ gboolean remove_non_secrets,
ForEachSecretFunc callback,
gpointer callback_data)
{
@@ -172,6 +173,9 @@ for_each_secret (NMConnection *connection,
const char *secret_name;
GValue *val;
+ if (g_hash_table_size (setting_hash) <= 0)
+ continue;
+
/* Get the actual NMSetting from the connection so we can get secret flags
* from the connection data, since flags aren't secrets. What we're
* iterating here is just the secrets, not a whole connection.
@@ -195,13 +199,21 @@ for_each_secret (NMConnection *connection,
g_hash_table_iter_init (&vpn_secrets_iter, g_value_get_boxed (val));
while (g_hash_table_iter_next (&vpn_secrets_iter, (gpointer) &secret_name,
NULL)) {
secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, secret_name, &secret_flags,
NULL);
- if (callback (&vpn_secrets_iter, secret_flags, callback_data) ==
FALSE)
+ if (!nm_setting_get_secret_flags (setting, secret_name,
&secret_flags, NULL)) {
+ if (remove_non_secrets)
+ g_hash_table_iter_remove (&secret_iter);
+ continue;
+ }
+ if (callback && callback (&vpn_secrets_iter, secret_flags,
callback_data) == FALSE)
return;
}
} else {
- nm_setting_get_secret_flags (setting, secret_name, &secret_flags, NULL);
- if (callback (&secret_iter, secret_flags, callback_data) == FALSE)
+ if (!nm_setting_get_secret_flags (setting, secret_name, &secret_flags, NULL))
{
+ if (remove_non_secrets)
+ g_hash_table_iter_remove (&secret_iter);
+ continue;
+ }
+ if (callback && callback (&secret_iter, secret_flags, callback_data) == FALSE)
return;
}
}
@@ -754,7 +766,7 @@ agent_secrets_done_cb (NMAgentManager *manager,
* save those system-owned secrets. If not, discard them and use the
* existing secrets, or fail the connection.
*/
- for_each_secret (NM_CONNECTION (self), secrets, has_system_owned_secrets, &agent_had_system);
+ for_each_secret (NM_CONNECTION (self), secrets, TRUE, has_system_owned_secrets,
&agent_had_system);
if (agent_had_system) {
if (flags == NM_SETTINGS_GET_SECRETS_FLAG_NONE) {
/* No user interaction was allowed when requesting secrets; the
@@ -766,7 +778,7 @@ agent_secrets_done_cb (NMAgentManager *manager,
call_id,
agent_dbus_owner);
- for_each_secret (NM_CONNECTION (self), secrets, clear_nonagent_secrets, NULL);
+ for_each_secret (NM_CONNECTION (self), secrets, FALSE,
clear_nonagent_secrets, NULL);
} else if (agent_has_modify == FALSE) {
/* Agent didn't successfully authenticate; clear system-owned secrets
* from the secrets the agent returned.
@@ -776,7 +788,7 @@ agent_secrets_done_cb (NMAgentManager *manager,
setting_name,
call_id);
- for_each_secret (NM_CONNECTION (self), secrets, clear_nonagent_secrets, NULL);
+ for_each_secret (NM_CONNECTION (self), secrets, FALSE,
clear_nonagent_secrets, NULL);
}
}
} else {
@@ -795,7 +807,7 @@ agent_secrets_done_cb (NMAgentManager *manager,
* came back. Unsaved secrets by definition require user interaction.
*/
if (flags == NM_SETTINGS_GET_SECRETS_FLAG_NONE)
- for_each_secret (NM_CONNECTION (self), secrets, clear_unsaved_secrets, NULL);
+ for_each_secret (NM_CONNECTION (self), secrets, TRUE, clear_unsaved_secrets, NULL);
/* Update the connection with our existing secrets from backing storage */
nm_connection_clear_secrets (NM_CONNECTION (self));
diff --git a/src/settings/plugins/ifnet/connection_parser.c b/src/settings/plugins/ifnet/connection_parser.c
index 72f4fdb..7089e6d 100644
--- a/src/settings/plugins/ifnet/connection_parser.c
+++ b/src/settings/plugins/ifnet/connection_parser.c
@@ -2913,7 +2913,8 @@ check_unsupported_secrets (NMSetting *setting,
if (flags & NM_SETTING_PARAM_SECRET) {
NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, key, &secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, key, &secret_flags, NULL))
+ g_return_if_reached ();
if (secret_flags != NM_SETTING_SECRET_FLAG_NONE)
*unsupported_secret = TRUE;
}
diff --git a/src/settings/plugins/keyfile/writer.c b/src/settings/plugins/keyfile/writer.c
index 2f8c7cc..2aee0e4 100644
--- a/src/settings/plugins/keyfile/writer.c
+++ b/src/settings/plugins/keyfile/writer.c
@@ -433,6 +433,7 @@ write_hash_of_string (GKeyFile *file,
const char *property = NULL, *data = NULL;
const char *group_name = nm_setting_get_name (setting);
gboolean vpn_secrets = FALSE;
+ gboolean invalid_property = FALSE;
/* Write VPN secrets out to a different group to keep them separate */
if (NM_IS_SETTING_VPN (setting) && !strcmp (key, NM_SETTING_VPN_SECRETS)) {
@@ -451,7 +452,10 @@ write_hash_of_string (GKeyFile *file,
if (vpn_secrets) {
NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, property, &secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, property, &secret_flags, NULL)) {
+ invalid_property = TRUE;
+ continue;
+ }
if (secret_flags != NM_SETTING_SECRET_FLAG_NONE)
write_item = FALSE;
}
@@ -459,6 +463,8 @@ write_hash_of_string (GKeyFile *file,
if (write_item)
nm_keyfile_plugin_kf_set_string (file, group_name, property, data);
}
+
+ g_return_if_fail (!invalid_property); (void) invalid_property;
}
static void
@@ -886,7 +892,8 @@ write_setting_value (NMSetting *setting,
if (pspec && (pspec->flags & NM_SETTING_PARAM_SECRET) && !NM_IS_SETTING_VPN (setting)) {
NMSettingSecretFlags secret_flags = NM_SETTING_SECRET_FLAG_NONE;
- nm_setting_get_secret_flags (setting, key, &secret_flags, NULL);
+ if (!nm_setting_get_secret_flags (setting, key, &secret_flags, NULL))
+ g_assert_not_reached ();
if (secret_flags != NM_SETTING_SECRET_FLAG_NONE)
return;
}
--
1.8.5.3
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
