From 9a0f3f3e0979a7f65d9c4b015624de1d59b11c0f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 19:05:44 +0200 Subject: [PATCH 01/16] glib-aux: add nm_utils_strv_dup_shallow_maybe_a() helper --- src/libnm-glib-aux/nm-shared-utils.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 4493e73bd5..024b68d676 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1907,6 +1907,34 @@ const char **_nm_utils_strv_dup_packed(const char *const *strv, gssize len); #define nm_utils_strv_dup_packed(strv, len) _nm_utils_strv_dup_packed(NM_CAST_STRV_CC(strv), (len)) +#define nm_utils_strv_dup_shallow_maybe_a(alloca_maxlen, strv, len, to_free) \ + ({ \ + const char *const *const _strv = NM_CAST_STRV_CC(strv); \ + const gssize _len = (len); \ + const char ** _result = NULL; \ + const char ***const _to_free = (to_free); \ + \ + G_STATIC_ASSERT_EXPR((alloca_maxlen) <= 500u / sizeof(const char *)); \ + G_STATIC_ASSERT_EXPR((alloca_maxlen) > 0u); \ + nm_assert(_to_free && !*_to_free); \ + \ + if (_len >= 0 || _strv) { \ + const gsize _l = (_len < 0) ? NM_PTRARRAY_LEN(_strv) : ((gsize) _len); \ + \ + if (G_LIKELY(_l < (alloca_maxlen))) { \ + _result = g_newa(const char *, _l + 1); \ + } else { \ + _result = g_new(const char *, _l + 1); \ + *_to_free = _result; \ + } \ + if (_l > 0) \ + memcpy(_result, _strv, _l * sizeof(const char *)); \ + _result[_l] = NULL; \ + } \ + \ + _result; \ + }) + /*****************************************************************************/ GSList *nm_utils_g_slist_find_str(const GSList *list, const char *needle); From a266bc15b284f0edaa354e3d10e6d610e709a5f0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 19:26:31 +0200 Subject: [PATCH 02/16] glib-aux: add nm_strv_has_duplicate() helper --- src/libnm-glib-aux/nm-shared-utils.c | 30 ++++++++++++++++++++++++++++ src/libnm-glib-aux/nm-shared-utils.h | 2 ++ 2 files changed, 32 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index 626956aefa..0cac64b5d1 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -2290,6 +2290,36 @@ nm_utils_strv_find_first(char **list, gssize len, const char *needle) return -1; } +gboolean +nm_strv_has_duplicate(const char *const *strv, gssize len, gboolean is_sorted) +{ + gsize l; + gsize i; + gsize j; + + l = len < 0 ? NM_PTRARRAY_LEN(strv) : (gsize) len; + + if (is_sorted) { +#if NM_MORE_ASSERTS > 10 + for (i = 1; i < l; i++) + nm_assert(nm_strcmp0(strv[i - 1], strv[i]) <= 0); +#endif + for (i = 1; i < l; i++) { + if (nm_streq0(strv[i - 1], strv[i])) + return TRUE; + } + } else { + for (i = 1; i < l; i++) { + for (j = 0; j < i; j++) { + if (nm_streq0(strv[j], strv[i])) + return TRUE; + } + } + } + + return FALSE; +} + char ** _nm_utils_strv_cleanup(char ** strv, gboolean strip_whitespace, diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 024b68d676..0299dd8dca 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -710,6 +710,8 @@ nm_utils_strsplit_set(const char *str, const char *delimiters) gssize nm_utils_strv_find_first(char **list, gssize len, const char *needle); +gboolean nm_strv_has_duplicate(const char *const *list, gssize len, gboolean is_sorted); + char **_nm_utils_strv_cleanup(char ** strv, gboolean strip_whitespace, gboolean skip_empty, From b0acbe504f319456fc36fea2530c75b109f079ea Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 13:43:34 +0200 Subject: [PATCH 03/16] glib-aux: add nm_strvarray_get_idx() helper --- src/libnm-glib-aux/nm-shared-utils.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 0299dd8dca..ac45bf4a67 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2747,6 +2747,15 @@ nm_strvarray_add(GArray *array, const char *str) g_array_append_val(array, s); } +static inline const char * +nm_strvarray_get_idx(GArray *array, guint idx) +{ + nm_assert(array); + nm_assert(idx < array->len); + + return g_array_index(array, const char *, idx); +} + static inline const char *const * nm_strvarray_get_strv_non_empty(GArray *arr, guint *length) { From 851267b6e768b53728018a07019cd28546a51082 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 14:19:20 +0200 Subject: [PATCH 04/16] glib-aux: add nm_strvarray_find_first() helper --- src/libnm-glib-aux/nm-shared-utils.h | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index ac45bf4a67..d34307d7bf 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2795,8 +2795,8 @@ nm_strvarray_set_strv(GArray **array, const char *const *strv) nm_strvarray_add(*array, strv[0]); } -static inline gboolean -nm_strvarray_remove_first(GArray *strv, const char *needle) +static inline gssize +nm_strvarray_find_first(GArray *strv, const char *needle) { guint i; @@ -2804,13 +2804,25 @@ nm_strvarray_remove_first(GArray *strv, const char *needle) if (strv) { for (i = 0; i < strv->len; i++) { - if (nm_streq(needle, g_array_index(strv, const char *, i))) { - g_array_remove_index(strv, i); - return TRUE; - } + if (nm_streq(needle, g_array_index(strv, const char *, i))) + return i; } } - return FALSE; + return -1; +} + +static inline gboolean +nm_strvarray_remove_first(GArray *strv, const char *needle) +{ + gssize idx; + + nm_assert(needle); + + idx = nm_strvarray_find_first(strv, needle); + if (idx < 0) + return FALSE; + g_array_remove_index(strv, idx); + return TRUE; } /*****************************************************************************/ From 8c6be1909fb4b3cfd3853aa51da2c37a82520af4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 14:32:08 +0200 Subject: [PATCH 05/16] glib-aux: add nm_strvarray_get_strv_non_empty_dup() helper --- src/libnm-glib-aux/nm-shared-utils.h | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index d34307d7bf..f2231fefb7 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2768,6 +2768,21 @@ nm_strvarray_get_strv_non_empty(GArray *arr, guint *length) return &g_array_index(arr, const char *, 0); } +static inline char ** +nm_strvarray_get_strv_non_empty_dup(GArray *arr, guint *length) +{ + const char *const *strv; + + if (!arr || arr->len == 0) { + NM_SET_OUT(length, 0); + return NULL; + } + + NM_SET_OUT(length, arr->len); + strv = &g_array_index(arr, const char *, 0); + return nm_utils_strv_dup(strv, arr->len, TRUE); +} + static inline const char *const * nm_strvarray_get_strv(GArray **arr, guint *length) { From 6ce7b3ca0f463c8c3bb50f9ab8fda38d0b3bf40f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 10:05:27 +0200 Subject: [PATCH 06/16] glib-aux: add nm_uuid_is_valid_normalized() helper --- src/libnm-glib-aux/nm-uuid.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libnm-glib-aux/nm-uuid.h b/src/libnm-glib-aux/nm-uuid.h index 0ead364090..29e32ddf87 100644 --- a/src/libnm-glib-aux/nm-uuid.h +++ b/src/libnm-glib-aux/nm-uuid.h @@ -41,6 +41,14 @@ nm_uuid_is_valid(const char *str) return str && nm_uuid_parse_full(str, NULL, NULL); } +static inline gboolean +nm_uuid_is_normalized(const char *str) +{ + gboolean is_normalized; + + return str && nm_uuid_parse_full(str, NULL, &is_normalized) && is_normalized; +} + /*****************************************************************************/ gboolean nm_uuid_is_valid_nmlegacy(const char *str); From 25f4d23e13b65ff766a8b7d7fdb6339cbb92d972 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 10:22:51 +0200 Subject: [PATCH 07/16] glib-aux: change nm_uuid_is_valid_full() to nm_uuid_is_normalized_full() Most of the time, we care about whether we have a normalized UUID. nm_uuid_is_valid_full() only exists for a particular case where we want to use the function in a header, without including "nm-uuid.h". In that case, we actually also care about normalized UUIDs. --- src/core/settings/nm-settings-storage.h | 6 +++--- src/libnm-glib-aux/nm-uuid.c | 6 +++--- src/libnm-glib-aux/nm-uuid.h | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/settings/nm-settings-storage.h b/src/core/settings/nm-settings-storage.h index d873a485bd..2e5e1c5579 100644 --- a/src/core/settings/nm-settings-storage.h +++ b/src/core/settings/nm-settings-storage.h @@ -60,14 +60,14 @@ nm_settings_storage_get_plugin(const NMSettingsStorage *self) return self->_plugin; } -gboolean nm_uuid_is_valid_full(const char *str); +gboolean nm_uuid_is_normalized_full(const char *str); static inline const char * nm_settings_storage_get_uuid(const NMSettingsStorage *self) { g_return_val_if_fail(NM_IS_SETTINGS_STORAGE(self), NULL); - nm_assert(nm_uuid_is_valid_full(self->_uuid)); + nm_assert(nm_uuid_is_normalized_full(self->_uuid)); return self->_uuid; } @@ -76,7 +76,7 @@ nm_settings_storage_get_uuid_opt(const NMSettingsStorage *self) { g_return_val_if_fail(NM_IS_SETTINGS_STORAGE(self), NULL); - nm_assert(!self->_uuid || nm_uuid_is_valid_full(self->_uuid)); + nm_assert(!self->_uuid || nm_uuid_is_normalized_full(self->_uuid)); return self->_uuid; } diff --git a/src/libnm-glib-aux/nm-uuid.c b/src/libnm-glib-aux/nm-uuid.c index 977a9e79a9..2c6e218a7c 100644 --- a/src/libnm-glib-aux/nm-uuid.c +++ b/src/libnm-glib-aux/nm-uuid.c @@ -130,13 +130,13 @@ nm_uuid_generate_random(NMUuid *out_uuid) /*****************************************************************************/ gboolean -nm_uuid_is_valid_full(const char *str) +nm_uuid_is_normalized_full(const char *str) { - /* The only reason why this exists is that nm_uuid_is_valid() is an inline function. + /* The only reason why this exists is that nm_uuid_is_normalized() is an inline function. * If you need to forward declare the function, that won't work. * * Usually, you wouldn't use this variant! */ - return nm_uuid_is_valid(str); + return nm_uuid_is_normalized(str); } /*****************************************************************************/ diff --git a/src/libnm-glib-aux/nm-uuid.h b/src/libnm-glib-aux/nm-uuid.h index 29e32ddf87..10302bd87a 100644 --- a/src/libnm-glib-aux/nm-uuid.h +++ b/src/libnm-glib-aux/nm-uuid.h @@ -33,14 +33,14 @@ gboolean nm_uuid_is_null(const NMUuid *uuid); /*****************************************************************************/ -gboolean nm_uuid_is_valid_full(const char *str); - static inline gboolean nm_uuid_is_valid(const char *str) { return str && nm_uuid_parse_full(str, NULL, NULL); } +gboolean nm_uuid_is_normalized_full(const char *str); + static inline gboolean nm_uuid_is_normalized(const char *str) { From 7e8e6836e0ec3bac1c5b3e681f87363cd46aec2d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 09:33:35 +0200 Subject: [PATCH 08/16] keyfile: fix comparison in nms_keyfile_nmmeta_read() "uuid" is returned from nms_keyfile_nmmeta_check_filename(), and contains "$UUID.nmmeta". We must compare only the first "uuid_len" bytes. Fixes: 064544cc0787 ('settings: support storing "shadowed-storage" to .nmmeta files') --- src/core/settings/plugins/keyfile/nms-keyfile-utils.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c index affe1c3d2a..2391a9fd77 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c @@ -137,7 +137,11 @@ nms_keyfile_nmmeta_read(const char * dirname, NMMETA_KF_GROUP_NAME_NMMETA, NMMETA_KF_KEY_NAME_NMMETA_UUID, NULL); - if (!nm_streq0(v_uuid, uuid)) + if (!v_uuid) + return FALSE; + if (strncmp(v_uuid, uuid, uuid_len) != 0) + return FALSE; + if (v_uuid[uuid_len] != '\0') return FALSE; loaded_path = g_key_file_get_string(kf, From 423e83b88037024424b2ac11e163e6bee8247678 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 09:55:03 +0200 Subject: [PATCH 09/16] keyfile: reject non-normalized UUIDs in nms_keyfile_nmmeta_check_filename() Since commit 207cf3d5d4c9 ('libnm: normalize "connection.uuid"') the "connection.uuid" is normalized to be a valid UUID and all lower case. That means, if we have .nmmeta files on disk with a previously valid, but now invalid UUID, the meta file is no longer going to match. Reject such file outright as invalid. If we really wanted to preserve backward compatibility, then we would have to also normalize the filename when we read it. However, that means, that suddenly we might have any number of compatible .nmmeta files that normalize to the same UUID, like the files 71088c75dec54119ab41be71bc10e736aaaabbbb.nmmeta F95D40B4-578A-5E68-8597-39392249442B.nmmeta f95d40b4-578a-5e68-8597-39392249442b.nmmeta Having multiple places for the nmmeta file is complicated to handle. Also, we often have the connection profile (and the normalized UUID) first, and then check whether it has a .nmmeta file. If we would support those unnormalized file names, we would have to visit all file names and try to normalize it, to find those with a matching UUID. Non-normalized UUIDs really should not be used and they already are not working anymore for the .nmmeta file. This commit only outright rejects them. This is a change in behavior, but the behavior change happened earlier when we started normalizing "connection.uuid". --- .../settings/plugins/keyfile/nms-keyfile-utils.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c index 2391a9fd77..44bb86829f 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c @@ -10,6 +10,7 @@ #include #include +#include "libnm-glib-aux/nm-uuid.h" #include "libnm-glib-aux/nm-io-utils.h" #include "libnm-core-intern/nm-keyfile-internal.h" #include "nm-utils.h" @@ -30,9 +31,9 @@ const char * nms_keyfile_nmmeta_check_filename(const char *filename, guint *out_uuid_len) { - const char *uuid; const char *s; gsize len; + char uuid[37]; s = strrchr(filename, '/'); if (s) @@ -50,17 +51,18 @@ nms_keyfile_nmmeta_check_filename(const char *filename, guint *out_uuid_len) len -= NM_STRLEN(NM_KEYFILE_PATH_SUFFIX_NMMETA); - if (!NM_IN_SET(len, 36, 40)) { + if (len != 36) { /* the remaining part of the filename has not the right length to * contain a UUID (according to nm_utils_is_uuid()). */ return NULL; } - uuid = nm_strndup_a(100, filename, len, NULL); - if (!nm_utils_is_uuid(uuid)) + memcpy(uuid, filename, 36); + uuid[36] = '\0'; + if (!nm_uuid_is_normalized(uuid)) return NULL; - NM_SET_OUT(out_uuid_len, len); + NM_SET_OUT(out_uuid_len, 36); return filename; } From 6f2ae46b37dd61405449a66891199b2a28082806 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 10:35:31 +0200 Subject: [PATCH 10/16] all: use nm_uuid_is_normalized() for checking valid UUID for "connection.uuid" "connection.uuid" gets normalized. When we check for a valid UUID, we expect it to be normalized. --- src/core/settings/nm-settings-storage.c | 5 +++-- src/core/settings/nm-settings.c | 9 +++++---- .../settings/plugins/ifcfg-rh/nms-ifcfg-rh-storage.c | 3 ++- src/core/settings/plugins/keyfile/nms-keyfile-plugin.c | 7 ++++--- src/core/settings/plugins/keyfile/nms-keyfile-storage.c | 5 +++-- src/core/settings/plugins/keyfile/nms-keyfile-utils.c | 8 ++++---- src/libnm-core-impl/nm-connection.c | 2 +- src/libnm-core-impl/tests/test-general.c | 2 +- 8 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/core/settings/nm-settings-storage.c b/src/core/settings/nm-settings-storage.c index c56ba075de..e284b822a8 100644 --- a/src/core/settings/nm-settings-storage.c +++ b/src/core/settings/nm-settings-storage.c @@ -7,6 +7,7 @@ #include "nm-settings-storage.h" +#include "libnm-glib-aux/nm-uuid.h" #include "nm-utils.h" #include "nm-settings-plugin.h" @@ -72,7 +73,7 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps case PROP_UUID: /* construct-only */ self->_uuid = g_value_dup_string(value); - nm_assert(!self->_uuid || nm_utils_is_uuid(self->_uuid)); + nm_assert(!self->_uuid || nm_uuid_is_normalized(self->_uuid)); break; case PROP_FILENAME: /* construct-only */ @@ -97,7 +98,7 @@ NMSettingsStorage * nm_settings_storage_new(NMSettingsPlugin *plugin, const char *uuid, const char *filename) { nm_assert(NM_IS_SETTINGS_PLUGIN(plugin)); - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); return g_object_new(NM_TYPE_SETTINGS_STORAGE, NM_SETTINGS_STORAGE_PLUGIN, diff --git a/src/core/settings/nm-settings.c b/src/core/settings/nm-settings.c index 1b1bfda88f..36a357748e 100644 --- a/src/core/settings/nm-settings.c +++ b/src/core/settings/nm-settings.c @@ -21,6 +21,7 @@ #endif #include "libnm-core-aux-intern/nm-common-macros.h" +#include "libnm-glib-aux/nm-uuid.h" #include "libnm-glib-aux/nm-keyfile-aux.h" #include "libnm-core-intern/nm-keyfile-internal.h" #include "nm-dbus-interface.h" @@ -139,7 +140,7 @@ nm_assert_storage_data_lst(CList *head) u = nm_settings_storage_get_uuid(sd->storage); if (!uuid) { uuid = u; - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); } else nm_assert(nm_streq0(uuid, u)); } @@ -182,7 +183,7 @@ _sett_conn_entry_new(const char *uuid) SettConnEntry *sett_conn_entry; gsize l_p_1; - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); l_p_1 = strlen(uuid) + 1; @@ -1461,7 +1462,7 @@ _add_connection_to_first_plugin(NMSettings * self, uuid = nm_connection_get_uuid(new_connection); - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); for (iter = priv->plugins; iter; iter = iter->next) { NMSettingsPlugin *plugin = NM_SETTINGS_PLUGIN(iter->data); @@ -2295,7 +2296,7 @@ nm_settings_delete_connection(NMSettings * self, nm_assert(NM_IS_SETTINGS_STORAGE(cur_storage)); uuid = nm_settings_storage_get_uuid(cur_storage); - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); sett_conn_entry = _sett_conn_entries_get(self, uuid); diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-storage.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-storage.c index 714357a8be..134bdf6844 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-storage.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-storage.c @@ -7,6 +7,7 @@ #include "nms-ifcfg-rh-storage.h" +#include "libnm-glib-aux/nm-uuid.h" #include "nm-utils.h" #include "libnm-core-intern/nm-core-internal.h" #include "nm-connection.h" @@ -88,7 +89,7 @@ static NMSIfcfgRHStorage * _storage_new(NMSIfcfgRHPlugin *plugin, const char *uuid, const char *filename) { nm_assert(NMS_IS_IFCFG_RH_PLUGIN(plugin)); - nm_assert(!uuid || nm_utils_is_uuid(uuid)); + nm_assert(!uuid || nm_uuid_is_normalized(uuid)); nm_assert(filename && filename[0] == '/'); return g_object_new(NMS_TYPE_IFCFG_RH_STORAGE, diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c b/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c index 5f7f26e58e..e8ae1814d8 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-plugin.c @@ -15,6 +15,7 @@ #include "libnm-std-aux/c-list-util.h" #include "libnm-glib-aux/nm-c-list.h" +#include "libnm-glib-aux/nm-uuid.h" #include "libnm-glib-aux/nm-io-utils.h" #include "nm-connection.h" @@ -233,7 +234,7 @@ _read_from_file(const char * full_filename, nm_assert(!connection || (_nm_connection_verify(connection, NULL) == NM_SETTING_VERIFY_SUCCESS)); - nm_assert(!connection || nm_utils_is_uuid(nm_connection_get_uuid(connection))); + nm_assert(!connection || nm_uuid_is_normalized(nm_connection_get_uuid(connection))); return connection; } @@ -260,7 +261,7 @@ _nm_assert_storage(gpointer plugin /* NMSKeyfilePlugin */, uuid = nms_keyfile_storage_get_uuid(storage); - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); nm_assert(((NMSKeyfileStorage *) storage)->is_meta_data || !(((NMSKeyfileStorage *) storage)->u.conn_data.connection) @@ -1106,7 +1107,7 @@ nms_keyfile_plugin_set_nmmeta_tombstone(NMSKeyfilePlugin * self, const char * dirname; nm_assert(NMS_IS_KEYFILE_PLUGIN(self)); - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); nm_assert(!out_storage || !*out_storage); nm_assert(!shadowed_storage || (set && in_memory)); diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-storage.c b/src/core/settings/plugins/keyfile/nms-keyfile-storage.c index c6b4b81f57..8c526c81b4 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-storage.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-storage.c @@ -7,6 +7,7 @@ #include "nms-keyfile-storage.h" +#include "libnm-glib-aux/nm-uuid.h" #include "nm-utils.h" #include "libnm-core-intern/nm-core-internal.h" #include "nms-keyfile-plugin.h" @@ -108,7 +109,7 @@ _storage_new(NMSKeyfilePlugin * plugin, NMSKeyfileStorage *self; nm_assert(NMS_IS_KEYFILE_PLUGIN(plugin)); - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); nm_assert(filename && filename[0] == '/'); self = g_object_new(NMS_TYPE_KEYFILE_STORAGE, @@ -135,7 +136,7 @@ nms_keyfile_storage_new_tombstone(NMSKeyfilePlugin * plugin, { NMSKeyfileStorage *self; - nm_assert(nm_utils_is_uuid(uuid)); + nm_assert(nm_uuid_is_normalized(uuid)); nm_assert(filename && filename[0] == '/'); nm_assert(nms_keyfile_nmmeta_check_filename(filename, NULL)); nm_assert(NM_IN_SET(storage_type, NMS_KEYFILE_STORAGE_TYPE_ETC, NMS_KEYFILE_STORAGE_TYPE_RUN)); diff --git a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c index 44bb86829f..e2c68bbecc 100644 --- a/src/core/settings/plugins/keyfile/nms-keyfile-utils.c +++ b/src/core/settings/plugins/keyfile/nms-keyfile-utils.c @@ -53,7 +53,7 @@ nms_keyfile_nmmeta_check_filename(const char *filename, guint *out_uuid_len) if (len != 36) { /* the remaining part of the filename has not the right length to - * contain a UUID (according to nm_utils_is_uuid()). */ + * contain a UUID (according to nm_uuid_is_normalized()). */ return NULL; } @@ -73,7 +73,7 @@ nms_keyfile_nmmeta_filename(const char *dirname, const char *uuid, gboolean temp char *s; nm_assert(dirname && dirname[0] == '/'); - nm_assert(nm_utils_is_uuid(uuid) && !strchr(uuid, '/')); + nm_assert(nm_uuid_is_normalized(uuid) && !strchr(uuid, '/')); if (g_snprintf(filename, sizeof(filename), @@ -82,7 +82,7 @@ nms_keyfile_nmmeta_filename(const char *dirname, const char *uuid, gboolean temp NM_KEYFILE_PATH_SUFFIX_NMMETA, temporary ? "~" : "") >= sizeof(filename)) { - /* valid uuids are limited in length (nm_utils_is_uuid). The buffer should always + /* valid uuids are limited in length (nm_uuid_is_normalized). The buffer should always * be large enough. */ nm_assert_not_reached(); } @@ -217,7 +217,7 @@ nms_keyfile_nmmeta_write(const char *dirname, int errsv; nm_assert(dirname && dirname[0] == '/'); - nm_assert(nm_utils_is_uuid(uuid) && !strchr(uuid, '/')); + nm_assert(nm_uuid_is_normalized(uuid) && !strchr(uuid, '/')); nm_assert(!loaded_path || loaded_path[0] == '/'); nm_assert(!shadowed_storage || loaded_path); diff --git a/src/libnm-core-impl/nm-connection.c b/src/libnm-core-impl/nm-connection.c index a8435c909c..5818a41f34 100644 --- a/src/libnm-core-impl/nm-connection.c +++ b/src/libnm-core-impl/nm-connection.c @@ -1740,7 +1740,7 @@ _nm_connection_ensure_normalized(NMConnection * connection, nm_assert(NM_IS_CONNECTION(connection)); nm_assert(!out_connection_clone || !*out_connection_clone); - nm_assert(!expected_uuid || nm_utils_is_uuid(expected_uuid)); + nm_assert(!expected_uuid || nm_uuid_is_normalized(expected_uuid)); if (expected_uuid) { if (nm_streq0(expected_uuid, nm_connection_get_uuid(connection))) diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index c911f0ec2c..d7af622d0c 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -7788,7 +7788,7 @@ static void __test_uuid(const char *expected_uuid, const char *str, gssize slen, char *uuid_test) { g_assert(uuid_test); - g_assert(nm_utils_is_uuid(uuid_test)); + g_assert(nm_uuid_is_normalized(uuid_test)); if (strcmp(uuid_test, expected_uuid)) { g_error("UUID test failed (1): text=%s, len=%lld, expected=%s, uuid_test=%s", From 75c6c4abf8d1516e4198dd086361f5cd96dca027 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 13:49:28 +0200 Subject: [PATCH 11/16] libnm: use nm_strvarray_get_idx() in "nm-setting-match.c" --- src/libnm-core-impl/nm-setting-match.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-match.c b/src/libnm-core-impl/nm-setting-match.c index 9efca1e817..b3e685dd1a 100644 --- a/src/libnm-core-impl/nm-setting-match.c +++ b/src/libnm-core-impl/nm-setting-match.c @@ -80,7 +80,7 @@ nm_setting_match_get_interface_name(NMSettingMatch *setting, int idx) g_return_val_if_fail(setting->interface_name && idx >= 0 && idx < setting->interface_name->len, NULL); - return g_array_index(setting->interface_name, const char *, idx); + return nm_strvarray_get_idx(setting->interface_name, idx); } /** @@ -225,7 +225,7 @@ nm_setting_match_get_kernel_command_line(NMSettingMatch *setting, guint idx) g_return_val_if_fail(setting->kernel_command_line && idx < setting->kernel_command_line->len, NULL); - return g_array_index(setting->kernel_command_line, const char *, idx); + return nm_strvarray_get_idx(setting->kernel_command_line, idx); } /** @@ -367,7 +367,7 @@ nm_setting_match_get_driver(NMSettingMatch *setting, guint idx) g_return_val_if_fail(setting->driver && idx < setting->driver->len, NULL); - return g_array_index(setting->driver, const char *, idx); + return nm_strvarray_get_idx(setting->driver, idx); } /** @@ -508,7 +508,7 @@ nm_setting_match_get_path(NMSettingMatch *setting, guint idx) g_return_val_if_fail(setting->path && idx < setting->path->len, NULL); - return g_array_index(setting->path, const char *, idx); + return nm_strvarray_get_idx(setting->path, idx); } /** @@ -707,7 +707,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) if (self->interface_name) { for (i = 0; i < self->interface_name->len; i++) { - if (nm_str_is_empty(g_array_index(self->interface_name, const char *, i))) { + if (nm_str_is_empty(nm_strvarray_get_idx(self->interface_name, i))) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -723,7 +723,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) if (self->kernel_command_line) { for (i = 0; i < self->kernel_command_line->len; i++) { - if (nm_str_is_empty(g_array_index(self->kernel_command_line, const char *, i))) { + if (nm_str_is_empty(nm_strvarray_get_idx(self->kernel_command_line, i))) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -739,7 +739,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) if (self->driver) { for (i = 0; i < self->driver->len; i++) { - if (nm_str_is_empty(g_array_index(self->driver, const char *, i))) { + if (nm_str_is_empty(nm_strvarray_get_idx(self->driver, i))) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -755,7 +755,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) if (self->path) { for (i = 0; i < self->path->len; i++) { - if (nm_str_is_empty(g_array_index(self->path, const char *, i))) { + if (nm_str_is_empty(nm_strvarray_get_idx(self->path, i))) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, From 46533cd15f7adde9602d39f3509894c17d94fba5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 14:33:13 +0200 Subject: [PATCH 12/16] libnm: use nm_strvarray_get_strv_non_empty_dup() in "nm-setting-match.c" --- src/libnm-core-impl/nm-setting-match.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-match.c b/src/libnm-core-impl/nm-setting-match.c index b3e685dd1a..9a0774db81 100644 --- a/src/libnm-core-impl/nm-setting-match.c +++ b/src/libnm-core-impl/nm-setting-match.c @@ -624,16 +624,17 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) switch (prop_id) { case PROP_INTERFACE_NAME: - g_value_set_boxed(value, nm_strvarray_get_strv_non_empty(self->interface_name, NULL)); + g_value_take_boxed(value, nm_strvarray_get_strv_non_empty_dup(self->interface_name, NULL)); break; case PROP_KERNEL_COMMAND_LINE: - g_value_set_boxed(value, nm_strvarray_get_strv_non_empty(self->kernel_command_line, NULL)); + g_value_take_boxed(value, + nm_strvarray_get_strv_non_empty_dup(self->kernel_command_line, NULL)); break; case PROP_DRIVER: - g_value_set_boxed(value, nm_strvarray_get_strv_non_empty(self->driver, NULL)); + g_value_take_boxed(value, nm_strvarray_get_strv_non_empty_dup(self->driver, NULL)); break; case PROP_PATH: - g_value_set_boxed(value, nm_strvarray_get_strv_non_empty(self->path, NULL)); + g_value_take_boxed(value, nm_strvarray_get_strv_non_empty_dup(self->path, NULL)); break; default: G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); From 92136135adb60cd79e83d5c5bb210026567b45f2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 14:00:46 +0200 Subject: [PATCH 13/16] libnm: don't reject empty strings in add/remove API For example for NM_SETTING_CONNECTION_SECONDARIES, the user can set the GObject property to a string list that includes empty strings. The C accessors (add/remove-by-value) should also accept any strings that are accepted otherwise. Asserting against empty strings is wrong. If the setting wants to reject empty strings, then it should use verify(). --- src/libnm-core-impl/nm-setting-connection.c | 6 ++---- src/libnm-core-impl/nm-setting-match.c | 24 +++++++-------------- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-connection.c b/src/libnm-core-impl/nm-setting-connection.c index 8693c85363..da605faf80 100644 --- a/src/libnm-core-impl/nm-setting-connection.c +++ b/src/libnm-core-impl/nm-setting-connection.c @@ -809,8 +809,7 @@ nm_setting_connection_add_secondary(NMSettingConnection *setting, const char *se GSList * iter; g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE); - g_return_val_if_fail(sec_uuid != NULL, FALSE); - g_return_val_if_fail(sec_uuid[0] != '\0', FALSE); + g_return_val_if_fail(sec_uuid, FALSE); priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting); for (iter = priv->secondaries; iter; iter = g_slist_next(iter)) { @@ -863,8 +862,7 @@ nm_setting_connection_remove_secondary_by_value(NMSettingConnection *setting, co GSList * iter; g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE); - g_return_val_if_fail(sec_uuid != NULL, FALSE); - g_return_val_if_fail(sec_uuid[0] != '\0', FALSE); + g_return_val_if_fail(sec_uuid, FALSE); priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting); for (iter = priv->secondaries; iter; iter = g_slist_next(iter)) { diff --git a/src/libnm-core-impl/nm-setting-match.c b/src/libnm-core-impl/nm-setting-match.c index 9a0774db81..e4f7c1a810 100644 --- a/src/libnm-core-impl/nm-setting-match.c +++ b/src/libnm-core-impl/nm-setting-match.c @@ -96,8 +96,7 @@ void nm_setting_match_add_interface_name(NMSettingMatch *setting, const char *interface_name) { g_return_if_fail(NM_IS_SETTING_MATCH(setting)); - g_return_if_fail(interface_name != NULL); - g_return_if_fail(interface_name[0] != '\0'); + g_return_if_fail(interface_name); nm_strvarray_add(nm_strvarray_ensure(&setting->interface_name), interface_name); _notify(setting, PROP_INTERFACE_NAME); @@ -138,8 +137,7 @@ gboolean nm_setting_match_remove_interface_name_by_value(NMSettingMatch *setting, const char *interface_name) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), FALSE); - g_return_val_if_fail(interface_name != NULL, FALSE); - g_return_val_if_fail(interface_name[0] != '\0', FALSE); + g_return_val_if_fail(interface_name, FALSE); if (nm_strvarray_remove_first(setting->interface_name, interface_name)) { _notify(setting, PROP_INTERFACE_NAME); @@ -241,8 +239,7 @@ void nm_setting_match_add_kernel_command_line(NMSettingMatch *setting, const char *kernel_command_line) { g_return_if_fail(NM_IS_SETTING_MATCH(setting)); - g_return_if_fail(kernel_command_line != NULL); - g_return_if_fail(kernel_command_line[0] != '\0'); + g_return_if_fail(kernel_command_line); nm_strvarray_add(nm_strvarray_ensure(&setting->kernel_command_line), kernel_command_line); _notify(setting, PROP_KERNEL_COMMAND_LINE); @@ -284,8 +281,7 @@ nm_setting_match_remove_kernel_command_line_by_value(NMSettingMatch *setting, const char * kernel_command_line) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), FALSE); - g_return_val_if_fail(kernel_command_line != NULL, FALSE); - g_return_val_if_fail(kernel_command_line[0] != '\0', FALSE); + g_return_val_if_fail(kernel_command_line, FALSE); if (nm_strvarray_remove_first(setting->kernel_command_line, kernel_command_line)) { _notify(setting, PROP_KERNEL_COMMAND_LINE); @@ -383,8 +379,7 @@ void nm_setting_match_add_driver(NMSettingMatch *setting, const char *driver) { g_return_if_fail(NM_IS_SETTING_MATCH(setting)); - g_return_if_fail(driver != NULL); - g_return_if_fail(driver[0] != '\0'); + g_return_if_fail(driver); nm_strvarray_add(nm_strvarray_ensure(&setting->driver), driver); _notify(setting, PROP_DRIVER); @@ -425,8 +420,7 @@ gboolean nm_setting_match_remove_driver_by_value(NMSettingMatch *setting, const char *driver) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), FALSE); - g_return_val_if_fail(driver != NULL, FALSE); - g_return_val_if_fail(driver[0] != '\0', FALSE); + g_return_val_if_fail(driver, FALSE); if (nm_strvarray_remove_first(setting->driver, driver)) { _notify(setting, PROP_DRIVER); @@ -524,8 +518,7 @@ void nm_setting_match_add_path(NMSettingMatch *setting, const char *path) { g_return_if_fail(NM_IS_SETTING_MATCH(setting)); - g_return_if_fail(path != NULL); - g_return_if_fail(path[0] != '\0'); + g_return_if_fail(path); nm_strvarray_add(nm_strvarray_ensure(&setting->path), path); _notify(setting, PROP_PATH); @@ -566,8 +559,7 @@ gboolean nm_setting_match_remove_path_by_value(NMSettingMatch *setting, const char *path) { g_return_val_if_fail(NM_IS_SETTING_MATCH(setting), FALSE); - g_return_val_if_fail(path != NULL, FALSE); - g_return_val_if_fail(path[0] != '\0', FALSE); + g_return_val_if_fail(path, FALSE); if (nm_strvarray_remove_first(setting->path, path)) { _notify(setting, PROP_PATH); From 3acf62f8be89416668b262a2519733817570d7fe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 14:19:23 +0200 Subject: [PATCH 14/16] libnm: use GArray to track "connection.secondaries" property instead of GSList GSList requires an additional allocation for the container struct for each element. Also, it does not have O(1) direct access. It's a pretty bad data structure, especially if the underlying data is in form of a strv array. Use a GArray instead and the nm_strvarray_*() helpers. --- src/libnm-core-impl/nm-setting-connection.c | 96 +++++++++++---------- src/libnm-core-impl/tests/test-general.c | 2 +- 2 files changed, 50 insertions(+), 48 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-connection.c b/src/libnm-core-impl/nm-setting-connection.c index da605faf80..e490cf5001 100644 --- a/src/libnm-core-impl/nm-setting-connection.c +++ b/src/libnm-core-impl/nm-setting-connection.c @@ -70,26 +70,26 @@ NM_GOBJECT_PROPERTIES_DEFINE(NMSettingConnection, PROP_MUD_URL, ); typedef struct { - GArray *permissions; - GSList *secondaries; /* secondary connections to activate with the base connection */ - char * id; - char * uuid; - char * stable_id; - char * interface_name; - char * type; - char * master; - char * slave_type; - char * zone; - char * mud_url; - guint64 timestamp; - int autoconnect_priority; - int autoconnect_retries; - int multi_connect; - int auth_retries; - int mdns; - int llmnr; - int wait_device_timeout; - guint gateway_ping_timeout; + GArray * permissions; + GArray * secondaries; + char * id; + char * uuid; + char * stable_id; + char * interface_name; + char * type; + char * master; + char * slave_type; + char * zone; + char * mud_url; + guint64 timestamp; + int autoconnect_priority; + int autoconnect_retries; + int multi_connect; + int auth_retries; + int mdns; + int llmnr; + int wait_device_timeout; + guint gateway_ping_timeout; NMSettingConnectionAutoconnectSlaves autoconnect_slaves; NMMetered metered; NMSettingConnectionLldp lldp; @@ -752,27 +752,37 @@ nm_setting_connection_get_num_secondaries(NMSettingConnection *setting) { g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), 0); - return g_slist_length(NM_SETTING_CONNECTION_GET_PRIVATE(setting)->secondaries); + return nm_g_array_len(NM_SETTING_CONNECTION_GET_PRIVATE(setting)->secondaries); } /** * nm_setting_connection_get_secondary: * @setting: the #NMSettingConnection - * @idx: the zero-based index of the secondary connection UUID entry + * @idx: the zero-based index of the secondary connection UUID entry. + * Access one past the length of secondaries is ok and will return + * %NULL. Otherwise, it is a user error. * - * Returns: the secondary connection UUID at index @idx + * Returns: the secondary connection UUID at index @idx or + * %NULL if @idx is the number of secondaries. **/ const char * nm_setting_connection_get_secondary(NMSettingConnection *setting, guint32 idx) { NMSettingConnectionPrivate *priv; + guint secondaries_len; g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), NULL); priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting); - g_return_val_if_fail(idx <= g_slist_length(priv->secondaries), NULL); - return (const char *) g_slist_nth_data(priv->secondaries, idx); + secondaries_len = nm_g_array_len(priv->secondaries); + if (idx >= secondaries_len) { + /* access one past the length is OK. */ + g_return_val_if_fail(idx == secondaries_len, NULL); + return NULL; + } + + return nm_strvarray_get_idx(priv->secondaries, idx); } /** @@ -806,18 +816,16 @@ gboolean nm_setting_connection_add_secondary(NMSettingConnection *setting, const char *sec_uuid) { NMSettingConnectionPrivate *priv; - GSList * iter; g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE); g_return_val_if_fail(sec_uuid, FALSE); priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting); - for (iter = priv->secondaries; iter; iter = g_slist_next(iter)) { - if (!strcmp(sec_uuid, (char *) iter->data)) - return FALSE; - } - priv->secondaries = g_slist_append(priv->secondaries, g_strdup(sec_uuid)); + if (nm_strvarray_find_first(priv->secondaries, sec_uuid) >= 0) + return FALSE; + + nm_strvarray_add(nm_strvarray_ensure(&priv->secondaries), sec_uuid); _notify(setting, PROP_SECONDARIES); return TRUE; } @@ -833,16 +841,14 @@ void nm_setting_connection_remove_secondary(NMSettingConnection *setting, guint32 idx) { NMSettingConnectionPrivate *priv; - GSList * elt; g_return_if_fail(NM_IS_SETTING_CONNECTION(setting)); priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting); - elt = g_slist_nth(priv->secondaries, idx); - g_return_if_fail(elt != NULL); - g_free(elt->data); - priv->secondaries = g_slist_delete_link(priv->secondaries, elt); + g_return_if_fail(idx < nm_g_array_len(priv->secondaries)); + + g_array_remove_index(priv->secondaries, idx); _notify(setting, PROP_SECONDARIES); } @@ -859,18 +865,15 @@ gboolean nm_setting_connection_remove_secondary_by_value(NMSettingConnection *setting, const char *sec_uuid) { NMSettingConnectionPrivate *priv; - GSList * iter; g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE); g_return_val_if_fail(sec_uuid, FALSE); priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting); - for (iter = priv->secondaries; iter; iter = g_slist_next(iter)) { - if (!strcmp(sec_uuid, (char *) iter->data)) { - priv->secondaries = g_slist_delete_link(priv->secondaries, iter); - _notify(setting, PROP_SECONDARIES); - return TRUE; - } + + if (nm_strvarray_remove_first(priv->secondaries, sec_uuid)) { + _notify(setting, PROP_SECONDARIES); + return TRUE; } return FALSE; } @@ -1620,7 +1623,7 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) g_value_set_enum(value, nm_setting_connection_get_autoconnect_slaves(setting)); break; case PROP_SECONDARIES: - g_value_take_boxed(value, _nm_utils_slist_to_strv(priv->secondaries, TRUE)); + g_value_take_boxed(value, nm_strvarray_get_strv_non_empty_dup(priv->secondaries, NULL)); break; case PROP_GATEWAY_PING_TIMEOUT: g_value_set_uint(value, priv->gateway_ping_timeout); @@ -1732,8 +1735,7 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps priv->autoconnect_slaves = g_value_get_enum(value); break; case PROP_SECONDARIES: - g_slist_free_full(priv->secondaries, g_free); - priv->secondaries = _nm_utils_strv_to_slist(g_value_get_boxed(value), TRUE); + nm_strvarray_set_strv(&priv->secondaries, g_value_get_boxed(value)); break; case PROP_GATEWAY_PING_TIMEOUT: priv->gateway_ping_timeout = g_value_get_uint(value); @@ -1812,7 +1814,7 @@ finalize(GObject *object) g_free(priv->slave_type); g_free(priv->mud_url); nm_clear_pointer(&priv->permissions, g_array_unref); - g_slist_free_full(priv->secondaries, g_free); + nm_clear_pointer(&priv->secondaries, g_array_unref); G_OBJECT_CLASS(nm_setting_connection_parent_class)->finalize(object); } diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index d7af622d0c..f9cc5e54e4 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -5060,7 +5060,7 @@ test_setting_connection_changed_signal(void) ASSERT_CHANGED(nm_setting_connection_add_secondary(s_con, uuid)); ASSERT_CHANGED(nm_setting_connection_remove_secondary(s_con, 0)); - NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(elt != NULL)); + NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < nm_g_array_len(priv->secondaries))); ASSERT_UNCHANGED(nm_setting_connection_remove_secondary(s_con, 1)); g_test_assert_expected_messages(); From 890df48d14c913f19f6d9040b1ab061b23d53463 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 15:15:43 +0200 Subject: [PATCH 15/16] libnm: verify and normalize "connection.secondaries" So far, we didn't verify the secondary connections at all. But these really are supposed to be UUIDs. As we now also normalize "connection.uuid" to be in a strict format, the user might have profiles with non-normalized UUIDs. In that case, the "connection.uuid" would be normalized, but "connection.secondaries" no longer matches. We can fix that by also normalizing "connection.secondaries". OK, this is not a very good reason, because it's unlikely to affect any users in practice ('though it's easy to reproduce). A better reason is that the secondary setting really should be well defined and verified. As we didn't do that so far, we cannot simply outright reject invalid settings. What this patch does instead, is silently changing the profile to only contain valid settings. That has it's own problems, like that the user setting an invalid value does not get an error nor the desired(?) outcome. But of all the bad choices, normalizing seems the most sensible one. Note that in practice, most client applications don't rely on setting arbitrary (invalid) "UUIDs". They simply expect to be able to set valid UUIDs, which they still are. For example, nm-connection-editor presents a drop down list of VPN profile, and nmcli also resolves connection IDs to the UUID. That is, clients already have an intimate understanding of this setting, and don't blindly set arbitrary values. Hence, this normalization is unlikely to hit users in practice. But what it gives is the guarantee that a verified connection only contains valid UUIDs. Now all UUIDs will be normalized, invalid entries removed, and the list made unique. --- src/libnm-core-impl/nm-connection-private.h | 2 + src/libnm-core-impl/nm-connection.c | 108 ++++++++++++++++++++ src/libnm-core-impl/nm-setting-connection.c | 9 ++ src/libnm-core-intern/nm-core-internal.h | 4 + 4 files changed, 123 insertions(+) diff --git a/src/libnm-core-impl/nm-connection-private.h b/src/libnm-core-impl/nm-connection-private.h index 532b17637b..0580625f4a 100644 --- a/src/libnm-core-impl/nm-connection-private.h +++ b/src/libnm-core-impl/nm-connection-private.h @@ -27,6 +27,8 @@ gboolean _nm_connection_detect_slave_type_full(NMSettingConnection *s_con, const char *_nm_connection_detect_bluetooth_type(NMConnection *self); +gboolean _nm_setting_connection_verify_secondaries(GArray *secondaries, GError **error); + gboolean _nm_connection_verify_required_interface_name(NMConnection *connection, GError **error); int _nm_setting_ovs_interface_verify_interface_type(NMSettingOvsInterface *self, diff --git a/src/libnm-core-impl/nm-connection.c b/src/libnm-core-impl/nm-connection.c index 5818a41f34..35a14a80b8 100644 --- a/src/libnm-core-impl/nm-connection.c +++ b/src/libnm-core-impl/nm-connection.c @@ -725,6 +725,113 @@ _normalize_connection_uuid(NMConnection *self) return TRUE; } +gboolean +_nm_setting_connection_verify_secondaries(GArray *secondaries, GError **error) +{ + const char *const *strv; + const guint len = nm_g_array_len(secondaries); + guint has_normalizable = FALSE; + gboolean has_invalid = FALSE; + gboolean has_duplicate = FALSE; + guint i; + + if (len == 0) + return TRUE; + + /* For historic reasons, the secondaries were not normalized/validated. + * + * Now, when we find any invalid/non-normalized values, we reject/normalize + * them. We also filter out duplicates. */ + + strv = nm_strvarray_get_strv_non_empty(secondaries, NULL); + + for (i = 0; i < len; i++) { + const char *uuid = strv[i]; + gboolean normalized; + + if (!nm_uuid_is_valid_nm(uuid, &normalized, NULL)) { + has_invalid = TRUE; + goto out; + } + if (normalized) + has_normalizable = TRUE; + } + if (has_normalizable) + goto out; + + if (len > 1) { + gs_free const char **strv_to_free = NULL; + const char ** strv2; + + strv2 = nm_utils_strv_dup_shallow_maybe_a(20, strv, len, &strv_to_free); + nm_utils_strv_sort(strv2, len); + has_duplicate = nm_strv_has_duplicate(strv2, len, TRUE); + } + +out: + if (has_invalid) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("has an invalid UUID")); + } else if (has_normalizable) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("has a UUID that requires normalization")); + } else if (has_duplicate) { + g_set_error_literal(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("has duplicate UUIDs")); + } else + return TRUE; + + g_prefix_error(error, + "%s.%s: ", + NM_SETTING_CONNECTION_SETTING_NAME, + NM_SETTING_CONNECTION_SECONDARIES); + return FALSE; +} + +static gboolean +_normalize_connection_secondaries(NMConnection *self) +{ + NMSettingConnection *s_con = nm_connection_get_setting_connection(self); + GArray * secondaries; + gs_strfreev char ** strv = NULL; + guint i; + guint j; + + nm_assert(s_con); + + secondaries = _nm_setting_connection_get_secondaries(s_con); + if (nm_g_array_len(secondaries) == 0) + return FALSE; + + if (_nm_setting_connection_verify_secondaries(secondaries, NULL)) + return FALSE; + + strv = nm_strvarray_get_strv_non_empty_dup(secondaries, NULL); + for (i = 0, j = 0; strv[i]; i++) { + gs_free char *s = g_steal_pointer(&strv[i]); + char uuid_normalized[37]; + gboolean uuid_is_normalized; + + if (!nm_uuid_is_valid_nm(s, &uuid_is_normalized, uuid_normalized)) + continue; + + if (nm_utils_strv_find_first(strv, j, uuid_is_normalized ? uuid_normalized : s) >= 0) + continue; + + strv[j++] = uuid_is_normalized ? g_strdup(uuid_normalized) : g_steal_pointer(&s); + } + strv[j] = NULL; + + g_object_set(s_con, NM_SETTING_CONNECTION_SECONDARIES, strv, NULL); + return TRUE; +} + static gboolean _normalize_connection_type(NMConnection *self) { @@ -1617,6 +1724,7 @@ _connection_normalize(NMConnection *connection, was_modified |= _normalize_connection_uuid(connection); was_modified |= _normalize_connection_type(connection); was_modified |= _normalize_connection_slave_type(connection); + was_modified |= _normalize_connection_secondaries(connection); was_modified |= _normalize_required_settings(connection); was_modified |= _normalize_invalid_slave_port_settings(connection); was_modified |= _normalize_ip_config(connection, parameters); diff --git a/src/libnm-core-impl/nm-setting-connection.c b/src/libnm-core-impl/nm-setting-connection.c index e490cf5001..110f2937b0 100644 --- a/src/libnm-core-impl/nm-setting-connection.c +++ b/src/libnm-core-impl/nm-setting-connection.c @@ -741,6 +741,12 @@ nm_setting_connection_get_autoconnect_slaves(NMSettingConnection *setting) return NM_SETTING_CONNECTION_GET_PRIVATE(setting)->autoconnect_slaves; } +GArray * +_nm_setting_connection_get_secondaries(NMSettingConnection *setting) +{ + return NM_SETTING_CONNECTION_GET_PRIVATE(setting)->secondaries; +} + /** * nm_setting_connection_get_num_secondaries: * @setting: the #NMSettingConnection @@ -1475,6 +1481,9 @@ after_interface_name: return NM_SETTING_VERIFY_NORMALIZABLE; } + if (!_nm_setting_connection_verify_secondaries(priv->secondaries, error)) + return NM_SETTING_VERIFY_NORMALIZABLE; + return TRUE; } diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index d4e7bf4e38..d01e432c68 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -515,6 +515,10 @@ GPtrArray *_nm_setting_bridge_port_get_vlans(NMSettingBridgePort *setting); /*****************************************************************************/ +GArray *_nm_setting_connection_get_secondaries(NMSettingConnection *setting); + +/*****************************************************************************/ + NMSettingBluetooth *_nm_connection_get_setting_bluetooth_for_nap(NMConnection *connection); /*****************************************************************************/ From 3699c31eb16377415069efa475ad61561017fa56 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 2 Jun 2021 21:34:56 +0200 Subject: [PATCH 16/16] libnm/tests: add test for normalizing "connection.secondaries" --- src/libnm-core-impl/tests/test-setting.c | 120 +++++++++++++++++++++++ 1 file changed, 120 insertions(+) diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index 8b1b358279..f4b9b7cb48 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -4465,6 +4465,123 @@ test_setting_metadata(void) /*****************************************************************************/ +static void +test_setting_connection_secondaries_verify(void) +{ + gs_unref_object NMConnection *con = NULL; + NMSettingConnection * s_con; + guint i_run; + guint i_word; + + con = nmtst_create_minimal_connection("test-sec", NULL, NM_SETTING_WIRED_SETTING_NAME, &s_con); + nmtst_connection_normalize(con); + + for (i_run = 0; i_run < 100; i_run++) { + guint word_len = nmtst_get_rand_word_length(NULL); + gs_unref_ptrarray GPtrArray *arr = NULL; + gs_unref_ptrarray GPtrArray *arr_norm = NULL; + gboolean was_normalized; + gboolean was_normalized2; + + /* create a random list of invalid, normalizable and normalized UUIDs. */ + arr = g_ptr_array_new(); + for (i_word = 0; i_word < word_len; i_word++) { + g_ptr_array_add(arr, + (char *) nmtst_rand_select((const char *) "", + "52c3feb9-3aa2-46f6-a07b-1765918699eb", + "52C3feb9-3aa2-46f6-a07b-1765918699eb", + "52C3feb9-3aa2-46f6-a07b-1765918699eb", + "f86dfb13df764894ab3836b7cdb9d82dde8b27c4", + "52C3feb93-aa2-46f6-a07b-1765918699eb", + "bogus")); + } + g_ptr_array_add(arr, NULL); + + /* set the new list of secondaries, and assert that the result is as expected. */ + + nmtst_assert_connection_verifies_without_normalization(con); + + 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)); \ + } \ + } \ + G_STMT_END + + _assert_secondaries(s_con, (const char *const *) arr->pdata); + + /* reimplement the normalization that we expect to happen and + * create an array @arr_norm with the expected result after normalization. */ + arr_norm = g_ptr_array_new_with_free_func(g_free); + for (i_word = 0; i_word < word_len; i_word++) { + const char *s = arr->pdata[i_word]; + gboolean is_normalized; + char uuid_normalized[37]; + + if (!nm_uuid_is_valid_nm(s, &is_normalized, uuid_normalized)) + continue; + + if (is_normalized) + s = uuid_normalized; + + if (nm_utils_strv_find_first((char **) arr_norm->pdata, arr_norm->len, s) >= 0) + continue; + + g_ptr_array_add(arr_norm, g_strdup(s)); + } + g_ptr_array_add(arr_norm, NULL); + + was_normalized = !nm_utils_strv_equal((char **) arr->pdata, (char **) arr_norm->pdata); + + if (was_normalized) + nmtst_assert_connection_verifies_and_normalizable(con); + else + nmtst_assert_connection_verifies_without_normalization(con); + + if (was_normalized || nmtst_get_rand_bool()) { + was_normalized2 = nmtst_connection_normalize(con); + g_assert(was_normalized == was_normalized2); + } + + _assert_secondaries(s_con, (const char *const *) arr_norm->pdata); + } +} + +/*****************************************************************************/ + NMTST_DEFINE(); int @@ -4483,6 +4600,9 @@ main(int argc, char **argv) test_8021x); g_test_add_data_func("/libnm/setting-8021x/pkcs12", "test-cert.p12, test", test_8021x); + g_test_add_func("/libnm/settings/test_setting_connection_secondaries_verify", + test_setting_connection_secondaries_verify); + g_test_add_func("/libnm/settings/bond/verify", test_bond_verify); g_test_add_func("/libnm/settings/bond/compare", test_bond_compare); g_test_add_func("/libnm/settings/bond/normalize", test_bond_normalize);