From da3e0af01a90bc26fbd1a87dd1083ba93862eae5 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 4 Oct 2018 09:35:35 +0200 Subject: [PATCH 1/7] libnm-core: fix coverity warning 3. NetworkManager-1.14.0/libnm-core/nm-utils.c:4944: var_compare_op: Comparing "str" to null implies that "str" might be null. 4. NetworkManager-1.14.0/libnm-core/nm-utils.c:4958: var_deref_op: Dereferencing null pointer "str". # 4956| # 4957| /* do some very basic validation to see if this might be a JSON object. */ # 4958|-> if (str[0] == '{') { # 4959| gsize l; # 4960| (cherry picked from commit 9b04b871a0ff2b510f504d12558e7f1de8bb2f74) --- libnm-core/nm-utils.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index f6e1555ef8..0f524e11be 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -4941,18 +4941,18 @@ const char **nm_utils_enum_get_values (GType type, int from, int to) static gboolean _nm_utils_is_json_object_no_validation (const char *str, GError **error) { - if (str) { - /* libjansson also requires only utf-8 encoding. */ - if (!g_utf8_validate (str, -1, NULL)) { - g_set_error_literal (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("not valid utf-8")); - return FALSE; - } - while (g_ascii_isspace (str[0])) - str++; + nm_assert (str); + + /* libjansson also requires only utf-8 encoding. */ + if (!g_utf8_validate (str, -1, NULL)) { + g_set_error_literal (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("not valid utf-8")); + return FALSE; } + while (g_ascii_isspace (str[0])) + str++; /* do some very basic validation to see if this might be a JSON object. */ if (str[0] == '{') { From 1d25a494857b8109309e6650b938dcb59d7b8314 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 4 Oct 2018 09:36:11 +0200 Subject: [PATCH 2/7] libnm-core: use g_variant_type_equal() to compare variant types Even if a direct pointer comparison should be fine, use the proper function. GVariantType documentation says: "Two types may not be compared by value; use g_variant_type_equal() or g_variant_type_is_subtype_of()." This also fixes coverity warnings. (cherry picked from commit 27ab932a499e300b966ef387d9ed4cac4d3b2b4c) --- libnm-core/nm-setting-ip-config.c | 2 +- libnm-core/nm-setting-sriov.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/libnm-core/nm-setting-ip-config.c b/libnm-core/nm-setting-ip-config.c index fa2535b8f0..2b3134b666 100644 --- a/libnm-core/nm-setting-ip-config.c +++ b/libnm-core/nm-setting-ip-config.c @@ -1310,7 +1310,7 @@ nm_ip_route_attribute_validate (const char *name, return FALSE; } - if (spec->type == G_VARIANT_TYPE_STRING) { + if (g_variant_type_equal (spec->type, G_VARIANT_TYPE_STRING)) { const char *string = g_variant_get_string (value, NULL); gs_free char *string_free = NULL; char *sep; diff --git a/libnm-core/nm-setting-sriov.c b/libnm-core/nm-setting-sriov.c index 45b3a1d2f5..7228fb0cc2 100644 --- a/libnm-core/nm-setting-sriov.c +++ b/libnm-core/nm-setting-sriov.c @@ -433,7 +433,7 @@ nm_sriov_vf_attribute_validate (const char *name, return FALSE; } - if (spec->type == G_VARIANT_TYPE_STRING) { + if (g_variant_type_equal (spec->type, G_VARIANT_TYPE_STRING)) { const char *string; switch (spec->str_type) { From 6df6ed4483ecff3fe3a81c1fc34bfe528621698f Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 4 Oct 2018 09:36:38 +0200 Subject: [PATCH 3/7] dispatcher: fix shellcheck warnings Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. And likewise, prefer [ p ] || [ q ] over [ p -o q ]. https://github.com/koalaman/shellcheck/wiki/SC2166 (cherry picked from commit 9e43821e1741e2800df00f71ced0569629b032ab) --- examples/dispatcher/10-ifcfg-rh-routes.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/dispatcher/10-ifcfg-rh-routes.sh b/examples/dispatcher/10-ifcfg-rh-routes.sh index 147506e471..d72400aed7 100755 --- a/examples/dispatcher/10-ifcfg-rh-routes.sh +++ b/examples/dispatcher/10-ifcfg-rh-routes.sh @@ -45,7 +45,7 @@ handle_ip_file() { } -if [ "$2" != "pre-up" -a "$2" != "down" ]; then +if [ "$2" != "pre-up" ] && [ "$2" != "down" ]; then exit 0 fi @@ -59,7 +59,7 @@ if [ -z "$profile" ]; then exit 0 fi -if ! [ -f "$dir/rule-$profile" -o -f "$dir/rule6-$profile" ]; then +if [ ! -f "$dir/rule-$profile" ] && [ ! -f "$dir/rule6-$profile" ]; then exit 0 fi From 8cbc08ce298e57de895d5431e0374b0df32f6562 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 4 Oct 2018 09:37:26 +0200 Subject: [PATCH 4/7] device: fix a wrong comparison 'i <= G_MAXINT' is always true. (cherry picked from commit e3b9606b24513c5cd386aac013625f75c1a392e9) --- src/devices/nm-device-ethernet-utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/devices/nm-device-ethernet-utils.c b/src/devices/nm-device-ethernet-utils.c index 8bdb19917a..b18eadfa9b 100644 --- a/src/devices/nm-device-ethernet-utils.c +++ b/src/devices/nm-device-ethernet-utils.c @@ -29,7 +29,7 @@ nm_device_ethernet_utils_get_default_wired_name (GHashTable *existing_ids) int i; /* Find the next available unique connection name */ - for (i = 1; i <= G_MAXINT; i++) { + for (i = 1; i < G_MAXINT; i++) { temp = g_strdup_printf (_("Wired connection %d"), i); if ( !existing_ids || !g_hash_table_contains (existing_ids, temp)) From 8fb85b1f5b43d449e63d9d4d09f96c2ced34023c Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 4 Oct 2018 09:37:55 +0200 Subject: [PATCH 5/7] shared/nm-utils: avoid a coverity warning 1. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1242: value_overwrite: Overwriting previous write to "ch" with value from "v". 2. NetworkManager-1.14.0/shared/nm-utils/nm-shared-utils.c:1239: assigned_value: Assigning value from "++str[0]" to "ch" here, but that stored value is overwritten before it can be used. # 1237| if (ch >= '0' && ch <= '7') { # 1238| v = v * 8 + (ch - '0'); # 1239|-> ch = (++str)[0]; # 1240| } # 1241| } Don't assign ch when it is going to be overwritten. (cherry picked from commit e4154895ff7da736ed0021280324aeb718a50408) --- shared/nm-utils/nm-shared-utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shared/nm-utils/nm-shared-utils.c b/shared/nm-utils/nm-shared-utils.c index 022c065283..4c2e329797 100644 --- a/shared/nm-utils/nm-shared-utils.c +++ b/shared/nm-utils/nm-shared-utils.c @@ -1236,7 +1236,7 @@ nm_utils_buf_utf8safe_unescape (const char *str, gsize *out_len, gpointer *to_fr ch = (++str)[0]; if (ch >= '0' && ch <= '7') { v = v * 8 + (ch - '0'); - ch = (++str)[0]; + ++str; } } ch = v; From c81d35d3529e222bd3278144d4376ad187205364 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Thu, 4 Oct 2018 09:38:58 +0200 Subject: [PATCH 6/7] libnm-core: remove unneeded comparisons a_gendata and b_gendata cannot be NULL. This makes coverity happy. (cherry picked from commit d0f85092b9af18d4fc26b6ec733f825739e956a0) --- libnm-core/nm-setting.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libnm-core/nm-setting.c b/libnm-core/nm-setting.c index 0cec516517..fa0125b965 100644 --- a/libnm-core/nm-setting.c +++ b/libnm-core/nm-setting.c @@ -1422,7 +1422,7 @@ nm_setting_diff (NMSetting *a, } else { g_hash_table_iter_init (&iter, a_gendata->hash); while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &val)) { - val2 = b_gendata ? g_hash_table_lookup (b_gendata->hash, key) : NULL; + val2 = g_hash_table_lookup (b_gendata->hash, key); compared_any = TRUE; if ( !val2 || !g_variant_equal (val, val2)) { @@ -1432,7 +1432,7 @@ nm_setting_diff (NMSetting *a, } g_hash_table_iter_init (&iter, b_gendata->hash); while (g_hash_table_iter_next (&iter, (gpointer *) &key, (gpointer *) &val)) { - val2 = a_gendata ? g_hash_table_lookup (a_gendata->hash, key) : NULL; + val2 = g_hash_table_lookup (a_gendata->hash, key); compared_any = TRUE; if ( !val2 || !g_variant_equal (val, val2)) { From 1d718cd16f884ad1f133ef19e09ad073862174af Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 7 Oct 2018 13:56:25 +0200 Subject: [PATCH 7/7] libnm-core: fix int comparisons in team setting (cherry picked from commit 72b45417712186b0247ba5a69e42d54a27763fb0) --- libnm-core/nm-setting-team.c | 4 ++-- shared/nm-utils/nm-shared-utils.h | 29 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/libnm-core/nm-setting-team.c b/libnm-core/nm-setting-team.c index e6737e48ce..e311ba7776 100644 --- a/libnm-core/nm-setting-team.c +++ b/libnm-core/nm-setting-team.c @@ -125,9 +125,9 @@ nm_team_link_watcher_new_ethtool (int delay_up, NMTeamLinkWatcher *watcher; const char *val_fail = NULL; - if (delay_up < 0 || delay_up > G_MAXINT32) + if (delay_up < 0 || !_NM_INT_LE_MAXINT32 (delay_up)) val_fail = "delay-up"; - if (delay_down < 0 || delay_down > G_MAXINT32) + if (delay_down < 0 || !_NM_INT_LE_MAXINT32 (delay_up)) val_fail = "delay-down"; if (val_fail) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_FAILED, diff --git a/shared/nm-utils/nm-shared-utils.h b/shared/nm-utils/nm-shared-utils.h index 5125bd3d85..bc492571a8 100644 --- a/shared/nm-utils/nm-shared-utils.h +++ b/shared/nm-utils/nm-shared-utils.h @@ -34,7 +34,7 @@ _NM_INT_NOT_NEGATIVE (gssize val) * * When using such an enum for accessing an array, one naturally wants to check * that the enum is not negative. However, the compiler doesn't like a plain - * comparisong "enum_val >= 0", because (if the enum is unsigned), it will warn + * comparison "enum_val >= 0", because (if the enum is unsigned), it will warn * that the expression is always true *duh*. Not even a cast to a signed * type helps to avoid the compiler warning in any case. * @@ -43,6 +43,33 @@ _NM_INT_NOT_NEGATIVE (gssize val) return val >= 0; } +/* check whether the integer value is smaller than G_MAXINT32. This macro exists + * for the sole purpose, that a plain "((int) value <= G_MAXINT32)" comparison + * may cause the compiler or coverity that this check is always TRUE. But the + * check depends on compile time and the size of C type "int". Of course, most + * of the time in is gint32 and an int value is always <= G_MAXINT32. The check + * exists to catch cases where that is not true. + * + * Together with the G_STATIC_ASSERT(), we make sure that this is always satisfied. */ +G_STATIC_ASSERT (sizeof (int) == sizeof (gint32)); +#if _NM_CC_SUPPORT_GENERIC +#define _NM_INT_LE_MAXINT32(value) \ + ({ \ + _nm_unused typeof (value) _value = (value); \ + \ + _Generic((value), \ + int: TRUE \ + ); \ + }) +#else +#define _NM_INT_LE_MAXINT32(value) ({ \ + _nm_unused typeof (value) _value = (value); \ + _nm_unused const int *_p_value = &_value; \ + \ + TRUE; \ + }) +#endif + /*****************************************************************************/ static inline char