From 5b36f215f4592a528e345ce7e2b8299ee9ca4282 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 16:54:35 +0200 Subject: [PATCH 01/22] ifcfg-rh: fix code that looks like a leak in write_bridge_vlans() "string" is leaked in the error case. But in practice, this cannot happen because nm_bridge_vlan_to_str() cannot fail. While at it, replace GString by NMStrBuf. Thanks Coverity: Error: RESOURCE_LEAK (CWE-772): NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1565: alloc_fn: Storage is returned from allocation function "g_string_new". NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1565: var_assign: Assigning: "string" = storage returned from "g_string_new("")". NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c:1572: leaked_storage: Variable "string" going out of scope leaks the storage it points to. # 1570| vlan_str = nm_bridge_vlan_to_str(vlan, error); # 1571| if (!vlan_str) # 1572|-> return FALSE; # 1573| if (string->len > 0) # 1574| g_string_append(string, ","); --- .../settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index d5ceab3217..4338c30593 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -16,6 +16,7 @@ #include #include "libnm-glib-aux/nm-enum-utils.h" +#include "libnm-glib-aux/nm-str-buf.h" #include "libnm-glib-aux/nm-io-utils.h" #include "nm-manager.h" #include "nm-setting-connection.h" @@ -1558,7 +1559,7 @@ write_bridge_vlans(NMSetting * setting, { gs_unref_ptrarray GPtrArray *vlans = NULL; NMBridgeVlan * vlan; - GString * string; + nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(0, FALSE); guint i; g_object_get(setting, property_name, &vlans, NULL); @@ -1566,7 +1567,6 @@ write_bridge_vlans(NMSetting * setting, if (!vlans || !vlans->len) return TRUE; - string = g_string_new(""); for (i = 0; i < vlans->len; i++) { gs_free char *vlan_str = NULL; @@ -1574,13 +1574,12 @@ write_bridge_vlans(NMSetting * setting, vlan_str = nm_bridge_vlan_to_str(vlan, error); if (!vlan_str) return FALSE; - if (string->len > 0) - g_string_append(string, ","); - nm_utils_escaped_tokens_escape_gstr_assert(vlan_str, ",", string); + if (strbuf.len > 0) + nm_str_buf_append_c(&strbuf, ','); + nm_str_buf_append(&strbuf, nm_utils_escaped_tokens_escape_unnecessary(vlan_str, ",")); } - svSetValueStr(ifcfg, key, string->str); - g_string_free(string, TRUE); + svSetValueStr(ifcfg, key, nm_str_buf_get_str(&strbuf)); return TRUE; } From f5685e5bc96d59c36fc9499b410bd8699afe13a8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 17:05:45 +0200 Subject: [PATCH 02/22] ifcfg-rh: add comment about unreachable code in write_bridge_vlans() --- src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 4338c30593..3d02ddbd4f 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1572,8 +1572,11 @@ write_bridge_vlans(NMSetting * setting, vlan = vlans->pdata[i]; vlan_str = nm_bridge_vlan_to_str(vlan, error); - if (!vlan_str) - return FALSE; + if (!vlan_str) { + /* nm_bridge_vlan_to_str() cannot fail (for now). */ + nm_assert_not_reached(); + continue; + } if (strbuf.len > 0) nm_str_buf_append_c(&strbuf, ','); nm_str_buf_append(&strbuf, nm_utils_escaped_tokens_escape_unnecessary(vlan_str, ",")); From 72e8336fdcd0b08092d7bff9fb9d3a478672127d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 17:04:34 +0200 Subject: [PATCH 03/22] libnm: add internal accessors for _nm_setting_bridge{,_port}_get_vlans() For internal code, when we control what we are doing, make it possible to directly access the internal GPtrArray. --- src/libnm-core-impl/nm-setting-bridge-port.c | 8 ++++++++ src/libnm-core-impl/nm-setting-bridge.c | 10 ++++++++++ src/libnm-core-intern/nm-core-internal.h | 6 ++++++ 3 files changed, 24 insertions(+) diff --git a/src/libnm-core-impl/nm-setting-bridge-port.c b/src/libnm-core-impl/nm-setting-bridge-port.c index 9d1ff25986..415b65e355 100644 --- a/src/libnm-core-impl/nm-setting-bridge-port.c +++ b/src/libnm-core-impl/nm-setting-bridge-port.c @@ -281,6 +281,14 @@ nm_setting_bridge_port_clear_vlans(NMSettingBridgePort *setting) } } +GPtrArray * +_nm_setting_bridge_port_get_vlans(NMSettingBridgePort *setting) +{ + nm_assert(NM_IS_SETTING_BRIDGE_PORT(setting)); + + return NM_SETTING_BRIDGE_PORT_GET_PRIVATE(setting)->vlans; +} + /*****************************************************************************/ static gboolean diff --git a/src/libnm-core-impl/nm-setting-bridge.c b/src/libnm-core-impl/nm-setting-bridge.c index 202a8791ed..e606432afe 100644 --- a/src/libnm-core-impl/nm-setting-bridge.c +++ b/src/libnm-core-impl/nm-setting-bridge.c @@ -910,6 +910,16 @@ nm_setting_bridge_clear_vlans(NMSettingBridge *setting) } } +GPtrArray * +_nm_setting_bridge_get_vlans(NMSettingBridge *setting) +{ + nm_assert(NM_IS_SETTING_BRIDGE(setting)); + + return NM_SETTING_BRIDGE_GET_PRIVATE(setting)->vlans; +} + +/*****************************************************************************/ + /** * nm_setting_bridge_get_group_address: * @setting: the #NMSettingBridge diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index c186fe1229..d4e7bf4e38 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -509,6 +509,12 @@ gboolean _nm_setting_bond_option_supported(const char *option, NMBondMode mode); /*****************************************************************************/ +GPtrArray *_nm_setting_bridge_get_vlans(NMSettingBridge *setting); + +GPtrArray *_nm_setting_bridge_port_get_vlans(NMSettingBridgePort *setting); + +/*****************************************************************************/ + NMSettingBluetooth *_nm_connection_get_setting_bluetooth_for_nap(NMConnection *connection); /*****************************************************************************/ From 7065d75b9116bc55a03103c3b2d8546b8a05f996 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 17:09:06 +0200 Subject: [PATCH 04/22] ifcfg-rh: avoid cloning vlans array in write_bridge_vlans() --- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index 3d02ddbd4f..00c7c33c5c 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1552,17 +1552,20 @@ write_team_setting(NMConnection *connection, shvarFile *ifcfg, gboolean *wired, static gboolean write_bridge_vlans(NMSetting * setting, - const char *property_name, + gboolean is_port, shvarFile * ifcfg, const char *key, GError ** error) { - gs_unref_ptrarray GPtrArray *vlans = NULL; - NMBridgeVlan * vlan; - nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(0, FALSE); - guint i; + GPtrArray * vlans; + NMBridgeVlan * vlan; + nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(0, FALSE); + guint i; - g_object_get(setting, property_name, &vlans, NULL); + if (is_port) + vlans = _nm_setting_bridge_port_get_vlans(NM_SETTING_BRIDGE_PORT(setting)); + else + vlans = _nm_setting_bridge_get_vlans(NM_SETTING_BRIDGE(setting)); if (!vlans || !vlans->len) return TRUE; @@ -1818,11 +1821,7 @@ write_bridge_setting(NMConnection *connection, shvarFile *ifcfg, gboolean *wired svSetValueStr(ifcfg, "BRIDGING_OPTS", opts->str); g_string_free(opts, TRUE); - if (!write_bridge_vlans((NMSetting *) s_bridge, - NM_SETTING_BRIDGE_VLANS, - ifcfg, - "BRIDGE_VLANS", - error)) + if (!write_bridge_vlans((NMSetting *) s_bridge, FALSE, ifcfg, "BRIDGE_VLANS", error)) return FALSE; svSetValueStr(ifcfg, "TYPE", TYPE_BRIDGE); @@ -1873,11 +1872,7 @@ write_bridge_port_setting(NMConnection *connection, shvarFile *ifcfg, GError **e svSetValueStr(ifcfg, "BRIDGING_OPTS", string->str); g_string_free(string, TRUE); - if (!write_bridge_vlans((NMSetting *) s_port, - NM_SETTING_BRIDGE_PORT_VLANS, - ifcfg, - "BRIDGE_PORT_VLANS", - error)) + if (!write_bridge_vlans((NMSetting *) s_port, TRUE, ifcfg, "BRIDGE_PORT_VLANS", error)) return FALSE; return TRUE; From 1556732ef0c11e01ef73e6b8b485b8f26c99bd98 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 17:31:59 +0200 Subject: [PATCH 05/22] glib-aux: add nm_str_buf_append_unichar() helper --- src/libnm-glib-aux/nm-str-buf.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libnm-glib-aux/nm-str-buf.h b/src/libnm-glib-aux/nm-str-buf.h index f6661e3b7f..0236394001 100644 --- a/src/libnm-glib-aux/nm-str-buf.h +++ b/src/libnm-glib-aux/nm-str-buf.h @@ -207,6 +207,17 @@ nm_str_buf_append_c_hex(NMStrBuf *strbuf, char ch, gboolean upper_case) strbuf->_priv_str[strbuf->_priv_len++] = nm_hexchar((guchar) ch, upper_case); } +static inline void +nm_str_buf_append_unichar(NMStrBuf *strbuf, gunichar wc) +{ + int l; + + nm_str_buf_maybe_expand(strbuf, 6 + 1, FALSE); + l = g_unichar_to_utf8(wc, &strbuf->_priv_str[strbuf->_priv_len]); + nm_assert(l > 0 && l <= 6); + strbuf->_priv_len += (gsize) l; +} + static inline void nm_str_buf_append_len(NMStrBuf *strbuf, const char *str, gsize len) { From dd3aa1224a3d182c258ea97fa5355dc58856e00d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 17:18:03 +0200 Subject: [PATCH 06/22] ifcfg-rh: use NMStrBuf in svUnescape() This is a popular, low-level function. Let's use NMStrBuf. Also, Coverity wrongly things that there is a leak here. This change should also avoid that: Error: RESOURCE_LEAK (CWE-772): NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/shvar.c:411: alloc_arg: "_gstr_init" allocates memory that is stored into "str". NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/shvar.c:423: noescape: Resource "str" is not freed or pointed-to in "g_string_append_len". NetworkManager-1.31.3/src/core/settings/plugins/ifcfg-rh/shvar.c:619: leaked_storage: Variable "str" going out of scope leaks the storage it points to. # 617| nm_assert(!str); # 618| *to_free = NULL; # 619|-> return ""; # 620| } # 621| Profile: We run test-ifcfg-rh which calls svUnescape() under realistic circumstances. However, the test does too many other things that svUnescape() would be measurable. So use the following patch, to run the tested code more frequently: diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.c b/src/core/settings/plugins/ifcfg-rh/shvar.c index c6099dd1731c..18a907113ea9 100644 --- a/src/core/settings/plugins/ifcfg-rh/shvar.c +++ b/src/core/settings/plugins/ifcfg-rh/shvar.c @@ -645,6 +645,24 @@ out_error: return NULL; } +#define svUnescape(value, to_free) \ + ({ \ + const char *_value = (value); \ + const char *_result; \ + int _i; \ + \ + for (_i = 0; TRUE; _i++) { \ + gs_free char *_to_free; \ + \ + _result = svUnescape(_value, &_to_free); \ + if (_i < 1000) \ + continue; \ + *(to_free) = g_steal_pointer(&_to_free); \ + break; \ + } \ + _result; \ + }) + /*****************************************************************************/ shvarFile * Build: CFLAGS='-O2' ./autogen.sh --with-more-asserts=0 make -j 10 src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh && \ src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh && perf stat -r 50 -B src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh Before: Performance counter stats for 'src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (20 runs): 590.56 msec task-clock:u # 0.972 CPUs utilized ( +- 0.48% ) 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 1,091 page-faults:u # 0.002 M/sec ( +- 0.12% ) 2,022,618,453 cycles:u # 3.425 GHz ( +- 0.33% ) 4,165,011,633 instructions:u # 2.06 insn per cycle ( +- 0.01% ) 1,168,673,648 branches:u # 1978.910 M/sec ( +- 0.01% ) 8,279,364 branch-misses:u # 0.71% of all branches ( +- 0.14% ) 0.60739 +- 0.00292 seconds time elapsed ( +- 0.48% ) After: Performance counter stats for 'src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (50 runs): 580.19 msec task-clock:u # 0.972 CPUs utilized ( +- 0.33% ) 0 context-switches:u # 0.000 K/sec 0 cpu-migrations:u # 0.000 K/sec 1,092 page-faults:u # 0.002 M/sec ( +- 0.08% ) 1,956,368,933 cycles:u # 3.372 GHz ( +- 0.22% ) 4,106,984,148 instructions:u # 2.10 insn per cycle ( +- 0.01% ) 1,087,931,864 branches:u # 1875.143 M/sec ( +- 0.01% ) 7,731,041 branch-misses:u # 0.71% of all branches ( +- 0.15% ) 0.59680 +- 0.00193 seconds time elapsed ( +- 0.32% ) The run time varies greatly. But it can be seen that the new code is consistently faster. --- src/core/settings/plugins/ifcfg-rh/shvar.c | 71 +++++++++++----------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/shvar.c b/src/core/settings/plugins/ifcfg-rh/shvar.c index c6099dd173..371756e8c3 100644 --- a/src/core/settings/plugins/ifcfg-rh/shvar.c +++ b/src/core/settings/plugins/ifcfg-rh/shvar.c @@ -14,6 +14,7 @@ #include #include +#include "libnm-glib-aux/nm-str-buf.h" #include "libnm-core-intern/nm-core-internal.h" #include "nm-core-utils.h" #include "libnm-glib-aux/nm-enum-utils.h" @@ -318,14 +319,14 @@ _ch_hex_get(char ch) } static void -_gstr_init(GString **str, const char *value, gsize i) +_strbuf_init(NMStrBuf *str, const char *value, gsize i) { nm_assert(str); nm_assert(value); - if (!(*str)) { - /* if @str is not yet initialized, it allocates - * a new GString and copies @i characters from + if (str->allocated == 0) { + /* if @str is not yet initialized, it initializes + * a new NMStrBuf and copies @i characters from * @value over. * * Unescaping usually does not extend the length of a string, @@ -335,20 +336,20 @@ _gstr_init(GString **str, const char *value, gsize i) * (FACTOR*strlen(value) + CONST), which is non trivial to get * right in all cases. Also, we would have to provision for the * very unlikely extreme case. - * Instead, use a GString buffer which can grow as needed. But for an + * Instead, use a NMStrBuf buffer which can grow as needed. But for an * initial guess, strlen(value) is a good start */ - *str = g_string_new_len(NULL, strlen(value) + 3); - if (i) - g_string_append_len(*str, value, i); + nm_str_buf_maybe_expand(str, strlen(value) + 3u, FALSE); + nm_str_buf_append_len(str, value, i); } } const char * svUnescape(const char *value, char **to_free) { - gsize i, j; - GString *str = NULL; + NMStrBuf str = NM_STR_BUF_INIT(0, FALSE); int looks_like_old_svescaped = -1; + gsize i; + gsize j; /* we handle bash syntax here (note that ifup has #!/bin/bash. * Thus, see https://www.gnu.org/software/bash/manual/html_node/Quoting.html#Quoting */ @@ -395,20 +396,20 @@ svUnescape(const char *value, char **to_free) if (value[i] == '\\') { /* backslash escape */ - _gstr_init(&str, value, i); + _strbuf_init(&str, value, i); i++; if (G_UNLIKELY(value[i] == '\0')) { /* we don't support line continuation */ goto out_error; } - g_string_append_c(str, value[i]); + nm_str_buf_append_c(&str, value[i]); i++; goto loop1_next; } if (value[i] == '\'') { /* single quotes */ - _gstr_init(&str, value, i); + _strbuf_init(&str, value, i); i++; j = i; while (TRUE) { @@ -420,14 +421,14 @@ svUnescape(const char *value, char **to_free) break; j++; } - g_string_append_len(str, &value[i], j - i); + nm_str_buf_append_len(&str, &value[i], j - i); i = j + 1; goto loop1_next; } if (value[i] == '"') { /* double quotes */ - _gstr_init(&str, value, i); + _strbuf_init(&str, value, i); i++; while (TRUE) { if (value[i] == '"') { @@ -466,11 +467,11 @@ svUnescape(const char *value, char **to_free) if (looks_like_old_svescaped < 0) looks_like_old_svescaped = _looks_like_old_svescaped(value); if (!looks_like_old_svescaped) - g_string_append_c(str, '\\'); + nm_str_buf_append_c(&str, '\\'); } else - g_string_append_c(str, '\\'); + nm_str_buf_append_c(&str, '\\'); } - g_string_append_c(str, value[i]); + nm_str_buf_append_c(&str, value[i]); i++; } goto loop1_next; @@ -478,7 +479,7 @@ svUnescape(const char *value, char **to_free) if (value[i] == '$' && value[i + 1] == '\'') { /* ANSI-C Quoting */ - _gstr_init(&str, value, i); + _strbuf_init(&str, value, i); i += 2; while (TRUE) { char ch; @@ -552,7 +553,7 @@ svUnescape(const char *value, char **to_free) } } /* like bash, we cut too large numbers off. E.g. A=$'\772' becomes 0xfa */ - g_string_append_c(str, (guint8) v); + nm_str_buf_append_c(&str, (guint8) v); } else if (NM_IN_SET(value[i], 'x', 'u', 'U')) { const char escape_type = value[i]; int max_digits = escape_type == 'x' ? 2 : escape_type == 'u' ? 4 : 8; @@ -561,8 +562,7 @@ svUnescape(const char *value, char **to_free) i++; if (!_ch_hex_is(value[i])) { /* missing hex value after "\x" escape. This is treated like no escaping. */ - g_string_append_c(str, '\\'); - g_string_append_c(str, escape_type); + nm_str_buf_append_c(&str, '\\', escape_type); } else { v = _ch_hex_get(value[i]); i++; @@ -574,22 +574,21 @@ svUnescape(const char *value, char **to_free) i++; } if (escape_type == 'x') - g_string_append_c(str, v); + nm_str_buf_append_c(&str, v); else { /* we treat the unicode escapes as utf-8 encoded values. */ - g_string_append_unichar(str, v); + nm_str_buf_append_unichar(&str, v); } } } else { - g_string_append_c(str, '\\'); - g_string_append_c(str, value[i]); + nm_str_buf_append_c(&str, '\\', value[i]); i++; } goto loop_ansic_next; } } else ch = value[i]; - g_string_append_c(str, ch); + nm_str_buf_append_c(&str, ch); i++; loop_ansic_next:; } @@ -603,8 +602,8 @@ loop_ansic_next:; } /* an unquoted, regular character. Just consume it directly. */ - if (str) - g_string_append_c(str, value[i]); + if (str.allocated > 0) + nm_str_buf_append_c(&str, value[i]); i++; loop1_next:; @@ -614,18 +613,19 @@ loop1_next:; out_value: if (i == 0) { - nm_assert(!str); + nm_assert(str.allocated == 0); + nm_assert(!str._priv_str); *to_free = NULL; return ""; } - if (str) { - if (str->len == 0 || str->str[0] == '\0') { - g_string_free(str, TRUE); + if (str.allocated > 0) { + if (str.len == 0 || nm_str_buf_get_str_unsafe(&str)[0] == '\0') { + nm_str_buf_destroy(&str); *to_free = NULL; return ""; } else { - *to_free = g_string_free(str, FALSE); + *to_free = nm_str_buf_finalize(&str, NULL); return *to_free; } } @@ -639,8 +639,7 @@ out_value: return value; out_error: - if (str) - g_string_free(str, TRUE); + nm_str_buf_destroy(&str); *to_free = NULL; return NULL; } From ceaa1c369f18a104f2fcda39b5b7dd6e0ce1f422 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 18:09:22 +0200 Subject: [PATCH 07/22] core: fix leak in _config_data_get_main_auth_polkit() Found by Coverity: Error: RESOURCE_LEAK (CWE-772): NetworkManager-1.31.3/src/core/nm-config-data.c:450: alloc_fn: Storage is returned from allocation function "nm_config_data_get_value". NetworkManager-1.31.3/src/core/nm-config-data.c:450: var_assign: Assigning: "str" = storage returned from "nm_config_data_get_value(self, "main", "auth-polkit", (enum [unnamed type of NMConfigGetValueFlags])6)". NetworkManager-1.31.3/src/core/nm-config-data.c:454: noescape: Resource "str" is not freed or pointed-to in "nm_auth_polkit_mode_from_string". NetworkManager-1.31.3/src/core/nm-config-data.c:465: leaked_storage: Variable "str" going out of scope leaks the storage it points to. # 463| NM_SET_OUT(out_invalid_config, FALSE); # 464| # 465|-> return auth_polkit_mode; # 466| } # 467| Fixes: 6d7446e52f5b ('core: add main.auth-polkit option "root-only"') --- src/core/nm-config-data.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/nm-config-data.c b/src/core/nm-config-data.c index 8594ed503a..94b099157c 100644 --- a/src/core/nm-config-data.c +++ b/src/core/nm-config-data.c @@ -445,7 +445,7 @@ static NMAuthPolkitMode _config_data_get_main_auth_polkit(const NMConfigData *self, gboolean *out_invalid_config) { NMAuthPolkitMode auth_polkit_mode; - const char * str; + gs_free char * str = NULL; str = nm_config_data_get_value(self, NM_CONFIG_KEYFILE_GROUP_MAIN, From 02dbba49d6d316077885c832fba57369cadd4195 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 18:17:14 +0200 Subject: [PATCH 08/22] libnm: fix leak in nm_vpn_service_plugin_read_vpn_details() Found by Coverity: Error: RESOURCE_LEAK (CWE-772): NetworkManager-1.31.3/src/libnm-client-impl/nm-vpn-service-plugin.c:814: alloc_fn: Storage is returned from allocation function "g_string_new". NetworkManager-1.31.3/src/libnm-client-impl/nm-vpn-service-plugin.c:814: var_assign: Assigning: "key" = storage returned from "g_string_new(line->str + strlen("DATA_KEY="))". NetworkManager-1.31.3/src/libnm-client-impl/nm-vpn-service-plugin.c:815: var_assign: Assigning: "str" = "key". NetworkManager-1.31.3/src/libnm-client-impl/nm-vpn-service-plugin.c:855: leaked_storage: Variable "str" going out of scope leaks the storage it points to. NetworkManager-1.31.3/src/libnm-client-impl/nm-vpn-service-plugin.c:855: leaked_storage: Variable "key" going out of scope leaks the storage it points to. # 853| NM_SET_OUT(out_secrets, g_steal_pointer(&secrets)); # 854| } # 855|-> return success; # 856| } # 857| Fixes: 3dfb72b92687 ('service-plugin: allow continuations in the auth-dialog protocol') --- src/libnm-client-impl/nm-vpn-service-plugin.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libnm-client-impl/nm-vpn-service-plugin.c b/src/libnm-client-impl/nm-vpn-service-plugin.c index 74e9e24f4f..016c761c40 100644 --- a/src/libnm-client-impl/nm-vpn-service-plugin.c +++ b/src/libnm-client-impl/nm-vpn-service-plugin.c @@ -755,8 +755,9 @@ nm_vpn_service_plugin_read_vpn_details(int fd, GHashTable **out_data, GHashTable gs_unref_hashtable GHashTable *secrets = NULL; gboolean success = FALSE; GHashTable * hash = NULL; - GString * key = NULL, *val = NULL; - nm_auto_free_gstring GString *line = NULL; + nm_auto_free_gstring GString *key = NULL; + nm_auto_free_gstring GString *val = NULL; + nm_auto_free_gstring GString *line = NULL; char c; GString *str = NULL; From 272119d925a8892f56d93dcfeeecac421f0ccec2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 18:23:38 +0200 Subject: [PATCH 09/22] libnm: add nm_auto_unref_tc_action cleanup macro for NMTCAction --- src/libnm-core-aux-intern/nm-libnm-core-utils.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libnm-core-aux-intern/nm-libnm-core-utils.h b/src/libnm-core-aux-intern/nm-libnm-core-utils.h index a7160dc1b7..e0a6aaf1b1 100644 --- a/src/libnm-core-aux-intern/nm-libnm-core-utils.h +++ b/src/libnm-core-aux-intern/nm-libnm-core-utils.h @@ -36,6 +36,9 @@ NM_AUTO_DEFINE_FCN0(NMTCQdisc *, _nm_auto_unref_tc_qdisc, nm_tc_qdisc_unref); #define nm_auto_unref_tc_tfilter nm_auto(_nm_auto_unref_tc_tfilter) NM_AUTO_DEFINE_FCN0(NMTCTfilter *, _nm_auto_unref_tc_tfilter, nm_tc_tfilter_unref); +#define nm_auto_unref_tc_action nm_auto(_nm_auto_unref_tc_action) +NM_AUTO_DEFINE_FCN0(NMTCAction *, _nm_auto_unref_tc_action, nm_tc_action_unref); + #define nm_auto_unref_bridge_vlan nm_auto(_nm_auto_unref_bridge_vlan) NM_AUTO_DEFINE_FCN0(NMBridgeVlan *, _nm_auto_unref_bridge_vlan, nm_bridge_vlan_unref); From 3cd56e92d4fa0fd2957094f3f46bf0e6d59cdbd5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 18:25:48 +0200 Subject: [PATCH 10/22] libnm: fix leak in nm_utils_tc_tfilter_from_str() Found by Coverity: Error: RESOURCE_LEAK (CWE-772): NetworkManager-1.31.3/src/libnm-core-impl/nm-utils.c:2772: alloc_fn: Storage is returned from allocation function "nm_utils_tc_action_from_str". NetworkManager-1.31.3/src/libnm-core-impl/nm-utils.c:2772: var_assign: Assigning: "action" = storage returned from "nm_utils_tc_action_from_str(extra_opts, error)". NetworkManager-1.31.3/src/libnm-core-impl/nm-utils.c:2785: leaked_storage: Variable "action" going out of scope leaks the storage it points to. # 2783| tfilter = nm_tc_tfilter_new(kind, parent, error); # 2784| if (!tfilter) # 2785|-> return NULL; # 2786| # 2787| nm_tc_tfilter_set_handle(tfilter, handle); Fixes: de41c45e616c ('libnm-core: add functionality for dealing with tc-style traffic filter specifiers') --- src/libnm-core-impl/nm-utils.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/libnm-core-impl/nm-utils.c b/src/libnm-core-impl/nm-utils.c index 1d2c216a5b..b606aaa39f 100644 --- a/src/libnm-core-impl/nm-utils.c +++ b/src/libnm-core-impl/nm-utils.c @@ -2738,14 +2738,14 @@ static const NMVariantAttributeSpec *const tc_tfilter_attribute_spec[] = { NMTCTfilter * nm_utils_tc_tfilter_from_str(const char *str, GError **error) { - guint32 handle = TC_H_UNSPEC; - guint32 parent = TC_H_UNSPEC; - gs_free char * kind = NULL; - gs_free char * rest = NULL; - NMTCAction * action = NULL; - const char * extra_opts = NULL; - NMTCTfilter * tfilter = NULL; - gs_unref_hashtable GHashTable *ht = NULL; + guint32 handle = TC_H_UNSPEC; + guint32 parent = TC_H_UNSPEC; + gs_free char * kind = NULL; + gs_free char * rest = NULL; + nm_auto_unref_tc_action NMTCAction *action = NULL; + const char * extra_opts = NULL; + NMTCTfilter * tfilter = NULL; + gs_unref_hashtable GHashTable *ht = NULL; GVariant * variant; nm_assert(str); @@ -2785,10 +2785,8 @@ nm_utils_tc_tfilter_from_str(const char *str, GError **error) return NULL; nm_tc_tfilter_set_handle(tfilter, handle); - if (action) { + if (action) nm_tc_tfilter_set_action(tfilter, action); - nm_tc_action_unref(action); - } return tfilter; } From 90f5d9114f9bb69a0420fa3f460bac03bdd83954 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 18:26:30 +0200 Subject: [PATCH 11/22] libnm: use nm_auto_unref_tc_action cleanup macro in nm_utils_tc_action_from_str() --- src/libnm-core-impl/nm-utils.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/libnm-core-impl/nm-utils.c b/src/libnm-core-impl/nm-utils.c index b606aaa39f..1bc44bb5de 100644 --- a/src/libnm-core-impl/nm-utils.c +++ b/src/libnm-core-impl/nm-utils.c @@ -2587,9 +2587,9 @@ nm_utils_tc_action_to_str(NMTCAction *action, GError **error) NMTCAction * nm_utils_tc_action_from_str(const char *str, GError **error) { - const char * kind = NULL; - const char * rest = NULL; - NMTCAction * action = NULL; + const char * kind = NULL; + const char * rest = NULL; + nm_auto_unref_tc_action NMTCAction *action = NULL; gs_unref_hashtable GHashTable *ht = NULL; gs_unref_hashtable GHashTable * options = NULL; GVariant * variant; @@ -2631,23 +2631,20 @@ nm_utils_tc_action_from_str(const char *str, GError **error) gpointer key, value; if (!attrs) { - nm_tc_action_unref(action); g_set_error(error, 1, 0, _("unsupported action option: '%s'."), rest); return NULL; } options = nm_utils_parse_variant_attributes(rest, ' ', ' ', FALSE, attrs, error); - if (!options) { - nm_tc_action_unref(action); + if (!options) return NULL; - } g_hash_table_iter_init(&iter, options); while (g_hash_table_iter_next(&iter, &key, &value)) nm_tc_action_set_attribute(action, key, g_variant_ref_sink(value)); } - return action; + return g_steal_pointer(&action); } /*****************************************************************************/ From 820ab364fd2b2962004330e6ba32931e4efcc2db Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 18:49:17 +0200 Subject: [PATCH 12/22] tests: avoid Coverity warning UNINIT in nmtst_keyfile_get_num_keys() A false positive: Error: UNINIT (CWE-457): NetworkManager-1.31.3/src/libnm-glib-aux/nm-test-utils.h:2816: var_decl: Declaring variable "l" without initializer. NetworkManager-1.31.3/src/libnm-glib-aux/nm-test-utils.h:2828: uninit_use: Using uninitialized value "l". # 2826| nmtst_assert_success(keys, error); # 2827| # 2828|-> g_assert_cmpint(NM_PTRARRAY_LEN(keys), ==, l); # 2829| # 2830| return l; --- src/libnm-glib-aux/nm-test-utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index a5157c1026..ff8eb60662 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -2813,7 +2813,7 @@ nmtst_keyfile_get_num_keys(GKeyFile *keyfile, const char *group_name) { gs_strfreev char **keys = NULL; gs_free_error GError *error = NULL; - gsize l; + gsize l = 0; g_assert(keyfile); g_assert(group_name); From 936b60e00f099a40acdbb3d1b1562709e5ef433f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 19:01:16 +0200 Subject: [PATCH 13/22] wifi: avoid large shift for calculating netmask in ip4_config_to_iwd_config() Found by Coverity: Error: BAD_SHIFT (CWE-682): [#def53] NetworkManager-1.31.3/src/core/devices/wifi/nm-wifi-utils.c:1590: zero_return: Function call "nm_ip_address_get_prefix(addr)" returns 0. NetworkManager-1.31.3/src/core/devices/wifi/nm-wifi-utils.c:1590: assignment: Assigning: "prefix" = "nm_ip_address_get_prefix(addr)". The value of "prefix" is now 0. NetworkManager-1.31.3/src/core/devices/wifi/nm-wifi-utils.c:1591: large_shift: In expression "0xffffffffU << 32U - prefix", left shifting by more than 31 bits has undefined behavior. The shift amount, "32U - prefix", is 32. # 1589| NMIPAddress *addr = nm_setting_ip_config_get_address(s_ip, 0); # 1590| guint prefix = nm_ip_address_get_prefix(addr); # 1591|-> in_addr_t netmask = htonl(0xffffffffu << (32 - prefix)); # 1592| char buf[INET_ADDRSTRLEN]; # 1593| Fixes: 9d22ae7981d7 ('wifi: Add utilities for writing IWD connection profiles') --- src/core/devices/wifi/nm-wifi-utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/devices/wifi/nm-wifi-utils.c b/src/core/devices/wifi/nm-wifi-utils.c index a8209c36bb..9250e6dd42 100644 --- a/src/core/devices/wifi/nm-wifi-utils.c +++ b/src/core/devices/wifi/nm-wifi-utils.c @@ -1591,7 +1591,7 @@ ip4_config_to_iwd_config(GKeyFile *file, NMSettingIPConfig *s_ip, GError **error if (num) { NMIPAddress *addr = nm_setting_ip_config_get_address(s_ip, 0); guint prefix = nm_ip_address_get_prefix(addr); - in_addr_t netmask = htonl(0xffffffffu << (32 - prefix)); + in_addr_t netmask = _nm_utils_ip4_prefix_to_netmask(prefix); char buf[INET_ADDRSTRLEN]; nm_ip_address_get_address_binary(addr, &ip); From 44abe6d6612898796e7069abd3bc0e1ce7f9b8b6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 19:13:43 +0200 Subject: [PATCH 14/22] systemd/tests: avoid using g_free() with buffers allocated with malloc() in _test_unbase64mem_mem() Coverity says: Error: ALLOC_FREE_MISMATCH (CWE-762): NetworkManager-1.31.3/src/core/tests/test-systemd.c:261: alloc: Allocation of memory which must be freed using "free". NetworkManager-1.31.3/src/core/tests/test-systemd.c:274: free: Calling "_nm_auto_g_free" frees "exp2_arr" using "g_free" but it should have been freed using "free". # 272| g_assert_cmpmem(expected_arr, expected_len, exp3_arr, exp3_len); # 273| } # 274|-> } # 275| # 276| #define _test_unbase64mem(base64, expected_str) \ Error: ALLOC_FREE_MISMATCH (CWE-762): NetworkManager-1.31.3/src/core/tests/test-systemd.c:270: alloc: Allocation of memory which must be freed using "free". NetworkManager-1.31.3/src/core/tests/test-systemd.c:274: free: Calling "_nm_auto_g_free" frees "exp3_arr" using "g_free" but it should have been freed using "free". # 272| g_assert_cmpmem(expected_arr, expected_len, exp3_arr, exp3_len); # 273| } # 274|-> } # 275| # 276| #define _test_unbase64mem(base64, expected_str) \ Fixes: 0298d540782b ('systemd: expose unbase64mem() as nm_sd_utils_unbase64mem()') --- src/core/tests/test-systemd.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/tests/test-systemd.c b/src/core/tests/test-systemd.c index c6711f0b2b..810632f8db 100644 --- a/src/core/tests/test-systemd.c +++ b/src/core/tests/test-systemd.c @@ -247,11 +247,11 @@ _test_unbase64mem_mem(const char *base64, const guint8 *expected_arr, gsize expe { gs_free char *expected_base64 = NULL; int r; - gs_free guint8 *exp2_arr = NULL; - gs_free guint8 *exp3_arr = NULL; - gsize exp2_len; - gsize exp3_len; - gsize i; + nm_auto_free guint8 *exp2_arr = NULL; + nm_auto_free guint8 *exp3_arr = NULL; + gsize exp2_len; + gsize exp3_len; + gsize i; expected_base64 = g_base64_encode(expected_arr, expected_len); From 64985beef81db801d21bf7c55f724621a90f53fe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 19:13:43 +0200 Subject: [PATCH 15/22] dhcp/systemd: avoid using g_free() with buffers allocated with malloc() in lease_to_ip4_config()() Coverity says: Error: ALLOC_FREE_MISMATCH (CWE-762): NetworkManager-1.31.3/src/core/dhcp/nm-dhcp-systemd.c:234: alloc: Allocation of memory which must be freed using "free". NetworkManager-1.31.3/src/core/dhcp/nm-dhcp-systemd.c:447: free: Calling "_nm_auto_g_free" frees "routes" using "g_free" but it should have been freed using "free". # 445| } # 446| NM_SET_OUT(out_options, g_steal_pointer(&options)); # 447|-> return g_steal_pointer(&ip4_config); # 448| } # 449| Fixes: acc0d792244d ('systemd: merge branch 'systemd' into master') --- src/core/dhcp/nm-dhcp-systemd.c | 46 ++++++++++++++++----------------- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/core/dhcp/nm-dhcp-systemd.c b/src/core/dhcp/nm-dhcp-systemd.c index f278062c95..5b34e7b47a 100644 --- a/src/core/dhcp/nm-dhcp-systemd.c +++ b/src/core/dhcp/nm-dhcp-systemd.c @@ -85,29 +85,29 @@ lease_to_ip4_config(NMDedupMultiIndex *multi_idx, const struct in_addr * addr_list; char addr_str[NM_UTILS_INET_ADDRSTRLEN]; const char * s; - nm_auto_free_gstring GString *str = NULL; - gs_free sd_dhcp_route **routes = NULL; - const char *const * search_domains = NULL; - guint16 mtu; - int i, num; - const void * data; - gsize data_len; - gboolean metered = FALSE; - gboolean has_router_from_classless = FALSE; - gboolean has_classless_route = FALSE; - gboolean has_static_route = FALSE; - const gint32 ts = nm_utils_get_monotonic_timestamp_sec(); - gint64 ts_time = time(NULL); - struct in_addr a_address; - struct in_addr a_netmask; - struct in_addr a_next_server; - struct in_addr server_id; - struct in_addr broadcast; - const struct in_addr * a_router; - guint32 a_plen; - guint32 a_lifetime; - guint32 renewal; - guint32 rebinding; + nm_auto_free_gstring GString *str = NULL; + nm_auto_free sd_dhcp_route **routes = NULL; + const char *const * search_domains = NULL; + guint16 mtu; + int i, num; + const void * data; + gsize data_len; + gboolean metered = FALSE; + gboolean has_router_from_classless = FALSE; + gboolean has_classless_route = FALSE; + gboolean has_static_route = FALSE; + const gint32 ts = nm_utils_get_monotonic_timestamp_sec(); + gint64 ts_time = time(NULL); + struct in_addr a_address; + struct in_addr a_netmask; + struct in_addr a_next_server; + struct in_addr server_id; + struct in_addr broadcast; + const struct in_addr * a_router; + guint32 a_plen; + guint32 a_lifetime; + guint32 renewal; + guint32 rebinding; gs_free nm_sd_dhcp_option *private_options = NULL; nm_assert(lease != NULL); From b487cf30a9dfaf80244b2d34e6678511ad45e165 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 19:41:37 +0200 Subject: [PATCH 16/22] glib-aux/tests: add test for _nm_utils_ip4_prefix_to_netmask() --- .../tests/test-shared-general.c | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/libnm-glib-aux/tests/test-shared-general.c b/src/libnm-glib-aux/tests/test-shared-general.c index b290dfea9a..afdb8871de 100644 --- a/src/libnm-glib-aux/tests/test-shared-general.c +++ b/src/libnm-glib-aux/tests/test-shared-general.c @@ -237,6 +237,34 @@ test_nm_ip4_addr_is_localhost(void) /*****************************************************************************/ +static void +test_nm_utils_ip4_prefix_to_netmask(void) +{ + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(0), ==, nmtst_inet4_from_string("0.0.0.0")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(1), ==, nmtst_inet4_from_string("128.0.0.0")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(2), ==, nmtst_inet4_from_string("192.0.0.0")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(16), + ==, + nmtst_inet4_from_string("255.255.0.0")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(24), + ==, + nmtst_inet4_from_string("255.255.255.0")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(30), + ==, + nmtst_inet4_from_string("255.255.255.252")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(31), + ==, + nmtst_inet4_from_string("255.255.255.254")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(32), + ==, + nmtst_inet4_from_string("255.255.255.255")); + g_assert_cmpint(_nm_utils_ip4_prefix_to_netmask(33), + ==, + nmtst_inet4_from_string("255.255.255.255")); +} + +/*****************************************************************************/ + static void test_unaligned(void) { @@ -1279,6 +1307,8 @@ main(int argc, char **argv) g_test_add_func("/general/test_nm_strdup_int", test_nm_strdup_int); g_test_add_func("/general/test_nm_strndup_a", test_nm_strndup_a); g_test_add_func("/general/test_nm_ip4_addr_is_localhost", test_nm_ip4_addr_is_localhost); + g_test_add_func("/general/test_nm_utils_ip4_prefix_to_netmask", + test_nm_utils_ip4_prefix_to_netmask); g_test_add_func("/general/test_unaligned", test_unaligned); g_test_add_func("/general/test_strv_cmp", test_strv_cmp); g_test_add_func("/general/test_strstrip_avoid_copy", test_strstrip_avoid_copy); From 463db1c7a605972ec7f0b05e0a59dbc9b6208756 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 19:43:10 +0200 Subject: [PATCH 17/22] tui: use internal _nm_utils_ip4_prefix_to_netmask() helper nm_utils_ip4_prefix_to_netmask() is public API of libnm. As we also want to have this function at a few places where we don't have libnm, we have an internal variant _nm_utils_ip4_prefix_to_netmask(). Use the internal variant consistently and everywhere. --- src/nmtui/nm-editor-bindings.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nmtui/nm-editor-bindings.c b/src/nmtui/nm-editor-bindings.c index 162e8b1b38..2b1c474847 100644 --- a/src/nmtui/nm-editor-bindings.c +++ b/src/nmtui/nm-editor-bindings.c @@ -444,7 +444,7 @@ ip_route_transform_from_dest_string(GBinding * binding, inet_pton(addr_family, addrstr, &v4); if (nm_utils_ip_is_site_local(AF_INET, &v4)) { prefix = nm_utils_ip4_get_default_prefix(v4); - if (v4 & (~nm_utils_ip4_prefix_to_netmask(prefix))) + if (v4 & (~_nm_utils_ip4_prefix_to_netmask(prefix))) prefix = 32; } else prefix = 32; From d527d3874c31a6e11c1c59e2454da01fc4e575c3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 21:24:56 +0200 Subject: [PATCH 18/22] glib-aux: workaround coverty warning about comparing nm_json_int_t with int64 Error: CONSTANT_EXPRESSION_RESULT (CWE-569): [#def240] NetworkManager-1.31.3/src/libnm-glib-aux/nm-json-aux.h:260: result_independent_of_operands: "v < -9223372036854775808LL /* (gint64)(-9223372036854775807L - 1L) */" is always false regardless of the values of its operands. This occurs as the logical first operand of "||". # 258| # 259| v = vt->nm_json_integer_value(elem); # 260|-> if (v < G_MININT64 || v > G_MAXINT64) # 261| return -ERANGE; # 262| Error: CONSTANT_EXPRESSION_RESULT (CWE-569): [#def241] NetworkManager-1.31.3/src/libnm-glib-aux/nm-json-aux.h:279: result_independent_of_operands: "v > 18446744073709551615UL" is always false regardless of the values of its operands. This occurs as the logical second operand of "||". # 277| # 278| v = vt->nm_json_integer_value(elem); # 279|-> if (v < 0 || v > G_MAXUINT64) # 280| return -ERANGE; # 281| --- src/libnm-glib-aux/nm-json-aux.h | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/libnm-glib-aux/nm-json-aux.h b/src/libnm-glib-aux/nm-json-aux.h index 1a4b539524..936e914641 100644 --- a/src/libnm-glib-aux/nm-json-aux.h +++ b/src/libnm-glib-aux/nm-json-aux.h @@ -256,9 +256,15 @@ nm_jansson_json_as_int64(const NMJsonVt *vt, const nm_json_t *elem, gint64 *out_ if (!nm_json_is_integer(elem)) return -EINVAL; + /* assert that this integer is signed. */ + G_STATIC_ASSERT_EXPR(((nm_json_int_t) -1) < 0); + v = vt->nm_json_integer_value(elem); - if (v < G_MININT64 || v > G_MAXINT64) - return -ERANGE; + + if (sizeof(v) > sizeof(gint64)) { + if (v < G_MININT64 || v > G_MAXINT64) + return -ERANGE; + } NM_SET_OUT(out_val, v); return 1; @@ -275,10 +281,18 @@ nm_jansson_json_as_uint64(const NMJsonVt *vt, const nm_json_t *elem, guint64 *ou if (!nm_json_is_integer(elem)) return -EINVAL; + /* assert that this integer is signed. */ + G_STATIC_ASSERT_EXPR(((nm_json_int_t) -1) < 0); + v = vt->nm_json_integer_value(elem); - if (v < 0 || v > G_MAXUINT64) + if (v < 0) return -ERANGE; + if (sizeof(v) > sizeof(gint64)) { + if (v > G_MAXUINT64) + return -ERANGE; + } + NM_SET_OUT(out_val, v); return 1; } From 2c628e4762aa6bd97304d3fcca66cc99dee62e34 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 21:35:55 +0200 Subject: [PATCH 19/22] libnmc-base:fix leak in NMSecretAgentSimple's request_secrets_from_ui() Found by Coverity: Error: RESOURCE_LEAK (CWE-772): [#def271] [important] NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: alloc_fn: Storage is returned from allocation function "nm_utils_ssid_to_utf8". NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: var_assign: Assigning: "ssid_utf8" = storage returned from "nm_utils_ssid_to_utf8(g_bytes_get_data(ssid, NULL), g_bytes_get_size(ssid))". NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:877: noescape: Resource "ssid_utf8" is not freed or pointed-to in "g_strdup_printf". NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:882: leaked_storage: Variable "ssid_utf8" going out of scope leaks the storage it points to. # 880| # 881| if (!add_wireless_secrets(request, secrets)) # 882|-> goto out_fail; # 883| } else if (nm_connection_is_type(request->connection, NM_SETTING_WIRED_SETTING_NAME)) { # 884| title = _("Wired 802.1X authentication"); Error: RESOURCE_LEAK (CWE-772): [#def272] [important] NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: alloc_fn: Storage is returned from allocation function "nm_utils_ssid_to_utf8". NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: var_assign: Assigning: "ssid_utf8" = storage returned from "nm_utils_ssid_to_utf8(g_bytes_get_data(ssid, NULL), g_bytes_get_size(ssid))". NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:877: noescape: Resource "ssid_utf8" is not freed or pointed-to in "g_strdup_printf". NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:883: leaked_storage: Variable "ssid_utf8" going out of scope leaks the storage it points to. # 881| if (!add_wireless_secrets(request, secrets)) # 882| goto out_fail; # 883|-> } else if (nm_connection_is_type(request->connection, NM_SETTING_WIRED_SETTING_NAME)) { # 884| title = _("Wired 802.1X authentication"); # 885| msg = g_strdup_printf(_("Secrets are required to access the wired network %s"), Fixes: 3fbabde4c3b9 ('libnm-core: replace GByteArray with pointer + length in some APIs') --- src/libnmc-base/nm-secret-agent-simple.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnmc-base/nm-secret-agent-simple.c b/src/libnmc-base/nm-secret-agent-simple.c index 69617d0fee..419a9a6ebf 100644 --- a/src/libnmc-base/nm-secret-agent-simple.c +++ b/src/libnmc-base/nm-secret-agent-simple.c @@ -867,7 +867,7 @@ request_secrets_from_ui(RequestData *request) if (nm_connection_is_type(request->connection, NM_SETTING_WIRELESS_SETTING_NAME)) { NMSettingWireless *s_wireless; GBytes * ssid; - char * ssid_utf8; + gs_free char * ssid_utf8 = NULL; s_wireless = nm_connection_get_setting_wireless(request->connection); ssid = nm_setting_wireless_get_ssid(s_wireless); From 853f411567fef300b184f1745bee19f38be5cdfa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 21:39:30 +0200 Subject: [PATCH 20/22] libnmt-newt: fix leak in nmt_newt_button_build_component() Found by Coverity: Error: RESOURCE_LEAK (CWE-772): [#def274] [important] NetworkManager-1.31.3/src/libnmt-newt/nmt-newt-button.c:118: alloc_fn: Storage is returned from allocation function "g_strdup_printf". NetworkManager-1.31.3/src/libnmt-newt/nmt-newt-button.c:118: var_assign: Assigning: "label" = storage returned from "g_strdup_printf(" <%s>", priv->label)". NetworkManager-1.31.3/src/libnmt-newt/nmt-newt-button.c:119: noescape: Resource "label" is not freed or pointed-to in "nmt_newt_locale_from_utf8". NetworkManager-1.31.3/src/libnmt-newt/nmt-newt-button.c:125: leaked_storage: Variable "label" going out of scope leaks the storage it points to. # 123| } # 124| # 125|-> return co; # 126| } # 127| Fixes: 3bda3fb60c10 ('nmtui: initial import of nmtui') --- src/libnmt-newt/nmt-newt-button.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libnmt-newt/nmt-newt-button.c b/src/libnmt-newt/nmt-newt-button.c index 09ca1648a8..4518499b86 100644 --- a/src/libnmt-newt/nmt-newt-button.c +++ b/src/libnmt-newt/nmt-newt-button.c @@ -108,7 +108,8 @@ nmt_newt_button_build_component(NmtNewtComponent *component, gboolean sensitive) { NmtNewtButtonPrivate *priv = NMT_NEWT_BUTTON_GET_PRIVATE(component); newtComponent co; - char * label = NULL, *label_lc; + gs_free char * label = NULL; + char * label_lc; if (sensitive) { label_lc = nmt_newt_locale_from_utf8(priv->label); From 61029d406452305ee6a7414dbd264fbc6f535a4b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 21:39:30 +0200 Subject: [PATCH 21/22] libnmt-newt: use cleanup macro in nmt_newt_button_build_component() --- src/libnmt-newt/nmt-newt-button.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libnmt-newt/nmt-newt-button.c b/src/libnmt-newt/nmt-newt-button.c index 4518499b86..974d05bdf4 100644 --- a/src/libnmt-newt/nmt-newt-button.c +++ b/src/libnmt-newt/nmt-newt-button.c @@ -108,18 +108,17 @@ nmt_newt_button_build_component(NmtNewtComponent *component, gboolean sensitive) { NmtNewtButtonPrivate *priv = NMT_NEWT_BUTTON_GET_PRIVATE(component); newtComponent co; - gs_free char * label = NULL; - char * label_lc; + gs_free char * label_lc = NULL; if (sensitive) { label_lc = nmt_newt_locale_from_utf8(priv->label); co = newtCompactButton(-1, -1, label_lc); - g_free(label_lc); } else { + gs_free char *label = NULL; + label = g_strdup_printf(" <%s>", priv->label); label_lc = nmt_newt_locale_from_utf8(label); co = newtLabel(-1, -1, label_lc); - g_free(label_lc); newtLabelSetColors(co, NMT_NEWT_COLORSET_DISABLED_BUTTON); } From e5f37477c066233fd8ae3aeaaeeff992ea584d9a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 6 May 2021 21:44:34 +0200 Subject: [PATCH 22/22] cli: fix leak in print_wifi_connection() Found by Coverity: Error: RESOURCE_LEAK (CWE-772): [#def297] [important] NetworkManager-1.31.3/src/nmcli/devices.c:4610: alloc_fn: Storage is returned from allocation function "nm_utils_ssid_to_utf8". NetworkManager-1.31.3/src/nmcli/devices.c:4610: var_assign: Assigning: "ssid" = storage returned from "nm_utils_ssid_to_utf8(g_bytes_get_data(ssid_bytes, NULL), g_bytes_get_size(ssid_bytes))". NetworkManager-1.31.3/src/nmcli/devices.c:4612: noescape: Resource "ssid" is not freed or pointed-to in "g_print". NetworkManager-1.31.3/src/nmcli/devices.c:4642: noescape: Resource "ssid" is not freed or pointed-to in "string_append_mecard". NetworkManager-1.31.3/src/nmcli/devices.c:4654: leaked_storage: Variable "ssid" going out of scope leaks the storage it points to. # 4652| # 4653| g_print("\n"); # 4654|-> } # 4655| # 4656| static gboolean Fixes: 7061341a41d5 ('cli: add "nmcli d wifi show"') --- src/nmcli/devices.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nmcli/devices.c b/src/nmcli/devices.c index c165701b3d..9a22e5f775 100644 --- a/src/nmcli/devices.c +++ b/src/nmcli/devices.c @@ -4599,7 +4599,7 @@ print_wifi_connection(const NmcConfig *nmc_config, NMConnection *connection) const char * psk = NULL; const char * type = NULL; GBytes * ssid_bytes; - char * ssid; + gs_free char * ssid = NULL; GString * string; s_wireless = nm_connection_get_setting_wireless(connection);