From c87433ebd2f349ce78e3a32e244b462a358f6347 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Jun 2021 13:26:19 +0200 Subject: [PATCH 1/4] platform: avoid wrong coverity warning in nmp_utils_sysctl_open_netdir() The warning is wrong, because we already assert for the string length a few lines earlier. Error: STRING_OVERFLOW (CWE-120): [#def595] NetworkManager-1.31.90/src/libnm-platform/nm-platform-utils.c:1896: fixed_size_dest: You might overrun the 16-character fixed-size string "ifname_buf_last_try" by copying "ifname" without checking the length. # 1894| if (nm_streq(ifname, ifname_buf_last_try)) # 1895| return -1; # 1896|-> strcpy(ifname_buf_last_try, ifname); # 1897| # 1898| fd_dir = open(sysdir, O_DIRECTORY | O_CLOEXEC); --- src/libnm-platform/nm-platform-utils.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libnm-platform/nm-platform-utils.c b/src/libnm-platform/nm-platform-utils.c index 6435dcc482..2470704a6f 100644 --- a/src/libnm-platform/nm-platform-utils.c +++ b/src/libnm-platform/nm-platform-utils.c @@ -1893,7 +1893,9 @@ nmp_utils_sysctl_open_netdir(int ifindex, const char *ifname_guess, char *out_if * end of the @try_count. */ if (nm_streq(ifname, ifname_buf_last_try)) return -1; - strcpy(ifname_buf_last_try, ifname); + + if (g_strlcpy(ifname_buf_last_try, ifname, IFNAMSIZ) >= IFNAMSIZ) + nm_assert_not_reached(); fd_dir = open(sysdir, O_DIRECTORY | O_CLOEXEC); if (fd_dir < 0) From 7825609f1f7bb0a0ea710a2fddba426f30a519de Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Jun 2021 13:32:29 +0200 Subject: [PATCH 2/4] glib-aux/tests: avoid coverity warning in test_nm_g_source_sentinel() Coverity wrongly think there is a use after free in the test: Error: USE_AFTER_FREE (CWE-416): [#def559] [important] NetworkManager-1.31.90/src/libnm-glib-aux/tests/test-shared-general.c:1305: alias: Assigning: "s1" = "_s". Now both point to the same storage. NetworkManager-1.31.90/src/libnm-glib-aux/tests/test-shared-general.c:1324: freed_arg: "g_source_unref" frees "s1". NetworkManager-1.31.90/src/libnm-glib-aux/tests/test-shared-general.c:1330: deref_after_free: Dereferencing freed pointer "s1". # 1328| s2 = nm_g_source_sentinel_get(0); # 1329| g_assert(s2 == s1); # 1330|-> g_assert_cmpint(g_atomic_int_get(&s1->ref_count), >=, 1); # 1331| } # 1332| } Rework the code in the hope to avoid the false warning. --- src/libnm-glib-aux/tests/test-shared-general.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-glib-aux/tests/test-shared-general.c b/src/libnm-glib-aux/tests/test-shared-general.c index 4987b5a901..6c7ab4882f 100644 --- a/src/libnm-glib-aux/tests/test-shared-general.c +++ b/src/libnm-glib-aux/tests/test-shared-general.c @@ -1326,8 +1326,8 @@ test_nm_g_source_sentinel(void) if (nmtst_get_rand_bool()) { s2 = nm_g_source_sentinel_get(0); + g_assert_cmpint(g_atomic_int_get(&s2->ref_count), >=, 1); g_assert(s2 == s1); - g_assert_cmpint(g_atomic_int_get(&s1->ref_count), >=, 1); } } From 627503ad8680413af264c86d942f3269dd5bdd55 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Jun 2021 13:35:58 +0200 Subject: [PATCH 3/4] cli: avoid coverity warning in do_connection_down() Error: USE_AFTER_FREE (CWE-416): [#def729] [important] NetworkManager-1.31.90/src/nmcli/connections.c:3288: freed_arg: "connection_cb_info_finish" frees "info". NetworkManager-1.31.90/src/nmcli/connections.c:3287: pass_freed_arg: Passing freed pointer "info" as an argument to "g_signal_handlers_disconnect_matched". # 3285| # 3286| if (info) { # 3287|-> g_signal_handlers_disconnect_by_func(active, down_active_connection_state_cb, info); # 3288| connection_cb_info_finish(info, active); # 3289| } --- src/nmcli/connections.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/nmcli/connections.c b/src/nmcli/connections.c index de915e3d84..9f700caed0 100644 --- a/src/nmcli/connections.c +++ b/src/nmcli/connections.c @@ -3284,7 +3284,13 @@ do_connection_down(const NMCCommand *cmd, NmCli *nmc, int argc, const char *cons g_clear_error(&error); if (info) { + /* coverity thinks that info might be freed already while we still iterate + * the loop. But it cannot, because connection_cb_info_finish() only does some + * kind of ref-counting that ensures info stays alive long enough. */ + + /* coverity[pass_freed_arg] */ g_signal_handlers_disconnect_by_func(active, down_active_connection_state_cb, info); + connection_cb_info_finish(info, active); } } From 14ec96f262f8a6f22d7a84f0a8b48ff9d6ff7fe0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 8 Jun 2021 13:43:10 +0200 Subject: [PATCH 4/4] libnm/tests: avoid coverity warning in test_setting_connection_secondaries_verify() nm_strvarray_get_strv() returns the input pointer itself, if _secondaries is NULL. It does so intentionally and correctly to create an artificial empty strv array. Coverity doesn't like this. Try to workaround the warning: Error: ARRAY_VS_SINGLETON (CWE-119): [#def484] NetworkManager-1.31.90/src/libnm-core-impl/tests/test-setting.c:4544: address_of: Taking address with "&_secondaries" yields a singleton pointer. NetworkManager-1.31.90/src/libnm-core-impl/tests/test-setting.c:4544: identity_transfer: Passing "&_secondaries" as argument 1 to function "nm_strvarray_get_strv", which returns that argument. NetworkManager-1.31.90/src/libnm-core-impl/tests/test-setting.c:4544: callee_ptr_arith: Passing "_Generic (nm_strvarray_get_strv(&_secondaries, NULL), char const * const * : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char const ** : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char * const * : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char ** : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), void const * : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), void * : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char const * const * const : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char const ** const : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char * const * const : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), char ** const : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), void const * const : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL), void * const : (char const * const *)nm_strvarray_get_strv(&_secondaries, NULL))" to function "_nm_utils_strv_cmp_n" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. # 4542| G_STMT_END # 4543| # 4544|-> _assert_secondaries(s_con, (const char *const *) arr->pdata); # 4545| # 4546| /* reimplement the normalization that we expect to happen and --- src/libnm-core-impl/tests/test-setting.c | 74 ++++++++++++------------ 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index f4b9b7cb48..8709283d78 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -4503,42 +4503,44 @@ test_setting_connection_secondaries_verify(void) g_object_set(s_con, NM_SETTING_CONNECTION_SECONDARIES, arr->pdata, NULL); -#define _assert_secondaries(s_con, expected) \ - G_STMT_START \ - { \ - NMSettingConnection *const _s_con = (s_con); \ - const char *const * _expected = (expected); \ - GArray * _secondaries; \ - const guint _expected_len = NM_PTRARRAY_LEN(_expected); \ - gs_strfreev char ** _sec_strv = NULL; \ - guint _i; \ - \ - g_assert(_expected); \ - \ - if (nmtst_get_rand_bool()) { \ - _secondaries = _nm_setting_connection_get_secondaries(_s_con); \ - g_assert_cmpint(_expected_len, ==, nm_g_array_len(_secondaries)); \ - g_assert((_expected_len == 0) == (!_secondaries)); \ - g_assert(nm_utils_strv_equal(_expected, nm_strvarray_get_strv(&_secondaries, NULL))); \ - } \ - \ - if (nmtst_get_rand_bool()) { \ - g_object_get(_s_con, NM_SETTING_CONNECTION_SECONDARIES, &_sec_strv, NULL); \ - g_assert_cmpint(_expected_len, ==, NM_PTRARRAY_LEN(_sec_strv)); \ - g_assert((_expected_len == 0) == (!_sec_strv)); \ - g_assert(nm_utils_strv_equal(_expected, _sec_strv ?: NM_STRV_EMPTY())); \ - } \ - \ - g_assert_cmpint(nm_setting_connection_get_num_secondaries(_s_con), ==, _expected_len); \ - if (nmtst_get_rand_bool()) { \ - for (_i = 0; _i < _expected_len; _i++) { \ - g_assert_cmpstr(nm_setting_connection_get_secondary(_s_con, _i), \ - ==, \ - _expected[_i]); \ - } \ - g_assert_null(nm_setting_connection_get_secondary(_s_con, _expected_len)); \ - } \ - } \ +#define _assert_secondaries(s_con, expected) \ + G_STMT_START \ + { \ + NMSettingConnection *const _s_con = (s_con); \ + const char *const * _expected = (expected); \ + GArray * _secondaries; \ + const guint _expected_len = NM_PTRARRAY_LEN(_expected); \ + gs_strfreev char ** _sec_strv = NULL; \ + guint _i; \ + \ + g_assert(_expected); \ + \ + if (nmtst_get_rand_bool()) { \ + _secondaries = _nm_setting_connection_get_secondaries(_s_con); \ + g_assert_cmpint(_expected_len, ==, nm_g_array_len(_secondaries)); \ + g_assert((_expected_len == 0) == (!_secondaries)); \ + g_assert(nm_utils_strv_equal(_expected, \ + _secondaries ? nm_strvarray_get_strv(&_secondaries, NULL) \ + : NM_PTRARRAY_EMPTY(const char *))); \ + } \ + \ + if (nmtst_get_rand_bool()) { \ + g_object_get(_s_con, NM_SETTING_CONNECTION_SECONDARIES, &_sec_strv, NULL); \ + g_assert_cmpint(_expected_len, ==, NM_PTRARRAY_LEN(_sec_strv)); \ + g_assert((_expected_len == 0) == (!_sec_strv)); \ + g_assert(nm_utils_strv_equal(_expected, _sec_strv ?: NM_STRV_EMPTY())); \ + } \ + \ + g_assert_cmpint(nm_setting_connection_get_num_secondaries(_s_con), ==, _expected_len); \ + if (nmtst_get_rand_bool()) { \ + for (_i = 0; _i < _expected_len; _i++) { \ + g_assert_cmpstr(nm_setting_connection_get_secondary(_s_con, _i), \ + ==, \ + _expected[_i]); \ + } \ + g_assert_null(nm_setting_connection_get_secondary(_s_con, _expected_len)); \ + } \ + } \ G_STMT_END _assert_secondaries(s_con, (const char *const *) arr->pdata);