From fbc6008260f58b625e3a6bd26129c66b51904012 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 10:22:59 +0100 Subject: [PATCH 1/7] core: fix uninialized boolean variable in reset_autoconnect_all() It's not critical, because at worst we get a false-positive that something changed. Found by coverity. Fixes: 4e7b05de7981c28eba6db48eb16476372594fed2 --- src/nm-policy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-policy.c b/src/nm-policy.c index 81a19feee7..d6fcf95e77 100644 --- a/src/nm-policy.c +++ b/src/nm-policy.c @@ -1409,7 +1409,7 @@ reset_autoconnect_all (NMPolicy *self, NMPolicyPrivate *priv = NM_POLICY_GET_PRIVATE (self); NMSettingsConnection *const*connections = NULL; guint i; - gboolean changed; + gboolean changed = FALSE; _LOGD (LOGD_DEVICE, "re-enabling autoconnect for all connections%s%s%s", device ? " on " : "", From a7087b1f0539d43783c67c36ba9ed2165f81cb77 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 10:27:51 +0100 Subject: [PATCH 2/7] core: avoid dereferencing NULL in nm_utils_resolve_conf_parse() Found by coverity. Fixes: 8f1ef161f4dd5ac197b622ac681d55d64c176797 --- src/nm-core-utils.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 40a0c352af..f6b33a1490 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -1960,8 +1960,7 @@ nm_utils_resolve_conf_parse (int addr_family, gsize i_tokens; tokens = nm_utils_strsplit_set (s, " \t"); - nm_assert (tokens); - for (i_tokens = 0; tokens[i_tokens]; i_tokens++) { + for (i_tokens = 0; tokens && tokens[i_tokens]; i_tokens++) { gs_free char *t = g_strstrip (g_strdup (tokens[i_tokens])); if ( _nm_utils_dns_option_validate (t, NULL, NULL, From fb2da4b26c86a1916ad52cba00d6e6c725e74f4c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 10:31:01 +0100 Subject: [PATCH 3/7] libnm: fix checking argument in nm_team_link_watcher_new_arp_ping() Found by coverity. Fixes: 6c93e32212f9901e882d5a9ca221d037162bc740 --- libnm-core/nm-setting-team.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libnm-core/nm-setting-team.c b/libnm-core/nm-setting-team.c index 7f38dd2559..ad397c9aa7 100644 --- a/libnm-core/nm-setting-team.c +++ b/libnm-core/nm-setting-team.c @@ -249,7 +249,7 @@ nm_team_link_watcher_new_arp_ping (gint init_wait, return NULL; } - if (strpbrk (target_host, " \\/\t=\"\'")) { + if (strpbrk (source_host, " \\/\t=\"\'")) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_FAILED, _("source-host '%s' contains invalid characters"), source_host); return NULL; From 27e8fffdb833748dfeb6648b8768c4ef48822841 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 10:37:59 +0100 Subject: [PATCH 4/7] platform: fix crash hashing NMPlatformTfilter and NMPlatformQdisc @kind might be NULL. There are 3 forms of the hash-update functions for string: str(), str0(), and strarr(). - str0() is when the string might be NULL. - str() does not allow the string to be NULL - strarr() is like str(), except it adds a G_STATIC_ASSERT() that the argument is a C array. The reason why a difference between str() and str0() exists, is because str0() hashes NULL different from a "" or any other string. This has an overhead, because it effectively must hash another bit of information that tells whether a string was passed or not. The reason is, that hashing a tupple of two strings should always yield a different hash value, even for "aa",""; "a","a"; "","aa", where naive concatentation would yield identical hash values in all three cases. Fixes: e75fc8279becce29e688551930d85e59e1280c89 --- src/platform/nm-platform.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index c794970ca4..369effb901 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -5320,7 +5320,7 @@ nm_platform_qdisc_to_string (const NMPlatformQdisc *qdisc, char *buf, gsize len) void nm_platform_qdisc_hash_update (const NMPlatformQdisc *obj, NMHashState *h) { - nm_hash_update_str (h, obj->kind); + nm_hash_update_str0 (h, obj->kind); nm_hash_update_vals (h, obj->ifindex, obj->addr_family, @@ -5387,17 +5387,17 @@ nm_platform_tfilter_to_string (const NMPlatformTfilter *tfilter, char *buf, gsiz void nm_platform_tfilter_hash_update (const NMPlatformTfilter *obj, NMHashState *h) { - nm_hash_update_str (h, obj->kind); + nm_hash_update_str0 (h, obj->kind); nm_hash_update_vals (h, obj->ifindex, obj->addr_family, obj->handle, obj->parent, obj->info); - nm_hash_update_str (h, obj->action.kind); if (obj->action.kind) { + nm_hash_update_str (h, obj->action.kind); if (nm_streq (obj->action.kind, NM_PLATFORM_ACTION_KIND_SIMPLE)) - nm_hash_update_str (h, obj->action.simple.sdata); + nm_hash_update_strarr (h, obj->action.simple.sdata); } } From 62d4dba74bac791c06ba70547f81b78eab5b9dc5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 10:49:09 +0100 Subject: [PATCH 5/7] platform: assert() for valid item in nm_platform_link_get_all() Coverity thinks that item might be NULL, but actually it cannot. Unclear how to avoid the false positive. --- src/platform/nm-platform.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 369effb901..f5775f021c 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -667,6 +667,7 @@ skip: /* There is a loop, pop the first (remaining) element from the list. * This can happen for veth pairs where each peer is parent of the other end. */ item = NMP_OBJECT_CAST_LINK (links->pdata[first_idx]); + nm_assert (item); g_hash_table_remove (unseen, GINT_TO_POINTER (item->ifindex)); g_ptr_array_add (result, links->pdata[first_idx]); links->pdata[first_idx] = NULL; From c274b565a66e0c2932377554ce4d33a4772602e4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 10:59:26 +0100 Subject: [PATCH 6/7] cli: avoid out-of-bounds-read for show_device_info() Probably not critical, because it will still include the terminating NULL, and just continue to fill the temporary buffer with static addresses. Found by coverity. Fixes: bfb9fd0d2f3d602a69537fe7776426ee9202ce9e --- clients/cli/devices.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clients/cli/devices.c b/clients/cli/devices.c index 8aad8b75c0..44ed7b35dd 100644 --- a/clients/cli/devices.c +++ b/clients/cli/devices.c @@ -1113,8 +1113,8 @@ show_device_info (NMDevice *device, NmCli *nmc) (const NMMetaAbstractInfo *const*) nmc_fields_dev_show_general, FALSE, NULL, NULL); - row = g_new0 (NmcOutputField, _NM_META_SETTING_TYPE_NUM + 1); - for (i = 0; i < _NM_META_SETTING_TYPE_NUM; i++) + row = g_new0 (NmcOutputField, G_N_ELEMENTS (nmc_fields_dev_show_general)); + for (i = 0; i < G_N_ELEMENTS (nmc_fields_dev_show_general); i++) row[i].info = (const NMMetaAbstractInfo *) &nmc_fields_dev_show_general[i]; print_required_fields (&nmc->nmc_config, NMC_OF_FLAG_MAIN_HEADER_ONLY, From f44f21c87e157c84ab865334f115e9fecd302465 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Dec 2017 11:05:52 +0100 Subject: [PATCH 7/7] core: avoid leaks parsing team link-watcher Found by coverity. --- libnm-core/nm-utils.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index 73eb4b520b..73035e5ecb 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -5155,13 +5155,16 @@ _nm_utils_team_link_watcher_from_json (json_t *json_element) g_return_val_if_fail (json_element, NULL); json_object_foreach (json_element, j_key, j_val) { - if (nm_streq (j_key, "name")) + if (nm_streq (j_key, "name")) { + g_free (name); name = strdup (json_string_value (j_val)); - else if (nm_streq (j_key, "target_host")) + } else if (nm_streq (j_key, "target_host")) { + g_free (target_host); target_host = strdup (json_string_value (j_val)); - else if (nm_streq (j_key, "source_host")) + } else if (nm_streq (j_key, "source_host")) { + g_free (source_host); source_host = strdup (json_string_value (j_val)); - else if (NM_IN_STRSET (j_key, "delay_up", "init_wait")) + } else if (NM_IN_STRSET (j_key, "delay_up", "init_wait")) val1 = json_integer_value (j_val); else if (NM_IN_STRSET (j_key, "delay_down", "interval")) val2 = json_integer_value (j_val);