From e4aa3f4b2d6ca94cfc3050e17ffe5eeaf0e91187 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 14:27:52 +0100 Subject: [PATCH 01/19] libnm: add "bridge_role" option to "ethernet.s390-options" https://bugzilla.redhat.com/show_bug.cgi?id=1935842 --- src/libnm-core-impl/nm-setting-wired.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index 7addb36c56..11fbcec41c 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -75,6 +75,7 @@ G_DEFINE_TYPE(NMSettingWired, nm_setting_wired, NM_TYPE_SETTING) /*****************************************************************************/ static const char *valid_s390_opts[] = { + "bridge_role", "broadcast_mode", "buffer_count", "canonical_macaddr", From ca869bff9f79025362cc7a55c5ebdc8e8cf10424 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 14:41:11 +0100 Subject: [PATCH 02/19] shared: add nm_utils_strv_find_binary_search() helper --- src/libnm-glib-aux/nm-shared-utils.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index e28a6f825e..db816a8c10 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2020,6 +2020,24 @@ gssize nm_utils_ptrarray_find_binary_search(gconstpointer * list, gssize * out_idx_first, gssize * out_idx_last); +#define nm_utils_strv_find_binary_search(strv, len, needle) \ + ({ \ + const char *const *const _strv = NM_CAST_STRV_CC(strv); \ + const gsize _len = (len); \ + const char *const _needle = (needle); \ + \ + nm_assert(_len == 0 || _strv); \ + nm_assert(_needle); \ + \ + nm_utils_ptrarray_find_binary_search((gconstpointer *) _strv, \ + _len, \ + _needle, \ + nm_strcmp_with_data, \ + NULL, \ + NULL, \ + NULL); \ + }) + gssize nm_utils_array_find_binary_search(gconstpointer list, gsize elem_size, gsize len, From 07610da4ca19e17bff154dc54d3a80f5ab5a7179 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 20:08:00 +0100 Subject: [PATCH 03/19] shared: add mutable union field NMUtilsNamedValue.{name,value_str}_mutable NMUtilsNamedValue is a generic tuple that we use for different purposes. Often we require a mutable string, so add a union alias for that case. --- src/libnm-glib-aux/nm-shared-utils.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index db816a8c10..588541e3fe 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -1727,9 +1727,11 @@ typedef struct { union { NMUtilsNamedEntry named_entry; const char * name; + char * name_mutable; }; union { const char *value_str; + char * value_str_mutable; gpointer value_ptr; }; } NMUtilsNamedValue; From ec6e9f0cac8f1d307a59a4541e0b9783c0a7fcca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 20:28:30 +0100 Subject: [PATCH 04/19] core: minor cleanup in NMDeviceEthernet:update_connection() to set s390 options We should have variables of the correct type and cast where necessary, and not use void pointers. --- src/core/devices/nm-device-ethernet.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/devices/nm-device-ethernet.c b/src/core/devices/nm-device-ethernet.c index c517c81c89..c96cd158f0 100644 --- a/src/core/devices/nm-device-ethernet.c +++ b/src/core/devices/nm-device-ethernet.c @@ -1786,7 +1786,8 @@ update_connection(NMDevice *device, NMConnection *connection) const char * mac = nm_device_get_hw_address(device); const char * mac_prop = NM_SETTING_WIRED_MAC_ADDRESS; GHashTableIter iter; - gpointer key, value; + const char * key; + const char * value; if (!s_wired) { s_wired = (NMSettingWired *) nm_setting_wired_new(); @@ -1824,8 +1825,8 @@ update_connection(NMDevice *device, NMConnection *connection) _nm_setting_wired_clear_s390_options(s_wired); g_hash_table_iter_init(&iter, priv->s390_options); - while (g_hash_table_iter_next(&iter, &key, &value)) - nm_setting_wired_add_s390_option(s_wired, (const char *) key, (const char *) value); + while (g_hash_table_iter_next(&iter, (gpointer *) &key, (gpointer *) &value)) + nm_setting_wired_add_s390_option(s_wired, key, value); } static void From 80c98b60da364e8449547bdb2901c85c721496be Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 14:22:24 +0100 Subject: [PATCH 05/19] libnm: make list of valid s390s options const A const global variable is stored in immutable memory. You thus get a crash trying to modify it, which is desirable. The user is really not supposed to modify this buffer, even if nm_setting_wired_get_valid_s390_options() wrongly returns a non-const pointer. --- src/libnm-core-impl/nm-setting-wired.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index 11fbcec41c..2bd123450b 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -74,7 +74,7 @@ G_DEFINE_TYPE(NMSettingWired, nm_setting_wired, NM_TYPE_SETTING) /*****************************************************************************/ -static const char *valid_s390_opts[] = { +static const char *const valid_s390_opts[] = { "bridge_role", "broadcast_mode", "buffer_count", @@ -680,7 +680,7 @@ _nm_setting_wired_clear_s390_options(NMSettingWired *setting) const char ** nm_setting_wired_get_valid_s390_options(NMSettingWired *setting) { - return valid_s390_opts; + return (const char **) valid_s390_opts; } /** From b9d73cfb2db8de0d3c400e63fce68cc80c50a1d0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 14:34:25 +0100 Subject: [PATCH 06/19] libnm: only check once for valid static array in valid_s390_opts_check() No need to check every time. The buffer is a const global buffer, so checking it once is enough. --- src/libnm-core-impl/nm-setting-wired.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index 2bd123450b..834a4ec54c 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -111,23 +111,22 @@ static const char *const valid_s390_opts[] = { static gboolean valid_s390_opts_check(const char *option) { -#if NM_MORE_ASSERTS > 5 - nm_assert(NM_PTRARRAY_LEN(valid_s390_opts) + 1 == G_N_ELEMENTS(valid_s390_opts)); - { + if (NM_MORE_ASSERT_ONCE(10)) { gsize i; + nm_assert(NM_PTRARRAY_LEN(valid_s390_opts) + 1u == G_N_ELEMENTS(valid_s390_opts)); + for (i = 0; i < G_N_ELEMENTS(valid_s390_opts); i++) { - if (i == G_N_ELEMENTS(valid_s390_opts) - 1) + if (i == G_N_ELEMENTS(valid_s390_opts) - 1u) nm_assert(!valid_s390_opts[i]); else { nm_assert(valid_s390_opts[i]); nm_assert(valid_s390_opts[i][0] != '\0'); if (i > 0) - g_assert(strcmp(valid_s390_opts[i - 1], valid_s390_opts[i]) < 0); + nm_assert(strcmp(valid_s390_opts[i - 1], valid_s390_opts[i]) < 0); } } } -#endif return option && (nm_utils_array_find_binary_search(valid_s390_opts, From dccfe1df349fbf33e5c2e5bae70c5d6e812c5aae Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 14:41:24 +0100 Subject: [PATCH 07/19] libnm: use nm_utils_strv_find_binary_search() in valid_s390_opts_check() --- src/libnm-core-impl/nm-setting-wired.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index 834a4ec54c..f17fa6b7bd 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -129,12 +129,9 @@ valid_s390_opts_check(const char *option) } return option - && (nm_utils_array_find_binary_search(valid_s390_opts, - sizeof(const char *), - G_N_ELEMENTS(valid_s390_opts) - 1, - &option, - nm_strcmp_p_with_data, - NULL) + && (nm_utils_strv_find_binary_search(valid_s390_opts, + G_N_ELEMENTS(valid_s390_opts) - 1, + option) >= 0); } From 1794d80028448fdf1e6c5d35df7cdbcf635fb962 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 19:06:55 +0100 Subject: [PATCH 08/19] libnm: mark option parameters for nm_setting_wired_get_num_s390_options() as (allow-none) --- src/libnm-core-impl/nm-setting-wired.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index f17fa6b7bd..5a0e004cdd 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -464,9 +464,9 @@ nm_setting_wired_get_num_s390_options(NMSettingWired *setting) * @setting: the #NMSettingWired * @idx: index of the desired option, from 0 to * nm_setting_wired_get_num_s390_options() - 1 - * @out_key: (out) (transfer none): on return, the key name of the s390 specific + * @out_key: (allow-none) (out) (transfer none): on return, the key name of the s390 specific * option; this value is owned by the setting and should not be modified - * @out_value: (out) (transfer none): on return, the value of the key of the + * @out_value: (allow-none) (out) (transfer none): on return, the value of the key of the * s390 specific option; this value is owned by the setting and should not be * modified * From 9f93b0495b5c13b4e253cdfc0d669d36259f8cf6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 19:07:20 +0100 Subject: [PATCH 09/19] libnm: improve error message for verify() failure for ethernet.s390-options --- src/libnm-core-impl/nm-setting-wired.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index 5a0e004cdd..f9a508fea6 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -806,14 +806,24 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) nm_assert(v->name); - if (!valid_s390_opts_check(v->name) || v->value_str[0] == '\0' - || strlen(v->value_str) > 200) { + if (!valid_s390_opts_check(v->name)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("invalid '%s' or its value '%s'"), - v->name, - v->value_str); + _("invalid key '%s'"), + v->name); + g_prefix_error(error, + "%s.%s: ", + NM_SETTING_WIRED_SETTING_NAME, + NM_SETTING_WIRED_S390_OPTIONS); + return FALSE; + } + if (v->value_str[0] == '\0' || strlen(v->value_str) > 200u) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("invalid value for key '%s'"), + v->name); g_prefix_error(error, "%s.%s: ", NM_SETTING_WIRED_SETTING_NAME, From fb0ac2e700dab59f24bf6c9ede7f616c6673c26b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 20:09:07 +0100 Subject: [PATCH 10/19] libnm: cleanup nm_setting_wired_add_s390_option() - integers are unsigned. Mark the constants as such. - assert that we don't overflow G_MAXUINT32. Note that nm_setting_wired_get_s390_option()'s index argument is of type guint32. So with that API you cannot track more than G_MAXUINT32 elements. - use nm_utils_strdup_reset(). It's less code, but it's also self-assignment safe (contrary to the previous code). --- src/libnm-core-impl/nm-setting-wired.c | 38 ++++++++++++++------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index f9a508fea6..f86bd95102 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -553,7 +553,6 @@ nm_setting_wired_add_s390_option(NMSettingWired *setting, const char *key, const { NMSettingWiredPrivate *priv; gssize idx; - NMUtilsNamedValue * v; g_return_val_if_fail(NM_IS_SETTING_WIRED(setting), FALSE); g_return_val_if_fail(value, FALSE); @@ -569,14 +568,16 @@ nm_setting_wired_add_s390_option(NMSettingWired *setting, const char *key, const if (idx < 0) { gsize dst_idx = ~idx; - if (priv->s390_options.n_alloc < priv->s390_options.len + 1) { - priv->s390_options.n_alloc = NM_MAX(4, (priv->s390_options.len + 1) * 2); + g_return_val_if_fail(priv->s390_options.len < G_MAXUINT32 - 1u, FALSE); + + if (priv->s390_options.n_alloc < ((gsize) priv->s390_options.len) + 1u) { + priv->s390_options.n_alloc = NM_MAX(4u, (((gsize) priv->s390_options.len) + 1u) * 2u); priv->s390_options.arr = g_realloc(priv->s390_options.arr, priv->s390_options.n_alloc * sizeof(NMUtilsNamedValue)); } if (dst_idx < priv->s390_options.len) { - memmove(&priv->s390_options.arr[dst_idx + 1], + memmove(&priv->s390_options.arr[dst_idx + 1u], &priv->s390_options.arr[dst_idx], (priv->s390_options.len - dst_idx) * sizeof(NMUtilsNamedValue)); } @@ -586,11 +587,8 @@ nm_setting_wired_add_s390_option(NMSettingWired *setting, const char *key, const }; priv->s390_options.len++; } else { - v = &priv->s390_options.arr[idx]; - if (nm_streq(value, v->value_str)) + if (!nm_utils_strdup_reset(&priv->s390_options.arr[idx].value_str_mutable, value)) return TRUE; - g_free((char *) v->value_str); - v->value_str = g_strdup(value); } _notify(setting, PROP_S390_OPTIONS); @@ -628,10 +626,10 @@ nm_setting_wired_remove_s390_option(NMSettingWired *setting, const char *key) g_free((char *) priv->s390_options.arr[dst_idx].name); g_free((char *) priv->s390_options.arr[dst_idx].value_str); - if (dst_idx + 1 != priv->s390_options.len) { + if (dst_idx + 1u != priv->s390_options.len) { memmove(&priv->s390_options.arr[dst_idx], - &priv->s390_options.arr[dst_idx + 1], - (priv->s390_options.len - dst_idx - 1) * sizeof(NMUtilsNamedValue)); + &priv->s390_options.arr[dst_idx + 1u], + (priv->s390_options.len - dst_idx - 1u) * sizeof(NMUtilsNamedValue)); } priv->s390_options.len--; @@ -1094,23 +1092,27 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps hash = g_value_get_boxed(value); - priv->s390_options.n_alloc = hash ? g_hash_table_size(hash) : 0u; + priv->s390_options.n_alloc = nm_g_hash_table_size(hash); - if (priv->s390_options.n_alloc > 0) { + if (priv->s390_options.n_alloc > 0u) { gboolean invalid_content = FALSE; GHashTableIter iter; const char * key; const char * val; - guint i, j; + guint j; + guint i; priv->s390_options.arr = g_new(NMUtilsNamedValue, priv->s390_options.n_alloc); + g_hash_table_iter_init(&iter, hash); while (g_hash_table_iter_next(&iter, (gpointer *) &key, (gpointer *) &val)) { if (!key || !val) { invalid_content = TRUE; continue; } + nm_assert(priv->s390_options.len < priv->s390_options.n_alloc); + priv->s390_options.arr[priv->s390_options.len] = (NMUtilsNamedValue){ .name = g_strdup(key), .value_str = g_strdup(val), @@ -1123,10 +1125,10 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps NULL, NULL); /* prune duplicate keys. This is only possible if @hash does not use - * g_str_equal() as compare function (which would be a bug). - * Still, handle this, because we use later binary sort and rely - * on unique names. One bug here, should not bork the remainder - * of the program. */ + * g_str_equal() as compare function (which would be a bug). + * Still, handle this, because we use later binary sort and rely + * on unique names. One bug here, should not bork the remainder + * of the program. */ j = 1; for (i = 1; i < priv->s390_options.len; i++) { if (nm_streq(priv->s390_options.arr[j - 1].name, From ddc41d427af9285573a2a4b8da136f3abf95c09f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 20:31:54 +0100 Subject: [PATCH 11/19] libnm: add internal _nm_setting_wired_is_valid_s390_option() helper --- src/libnm-core-impl/nm-setting-wired.c | 6 +++--- src/libnm-core-intern/nm-core-internal.h | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index f86bd95102..af6e2eec0d 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -108,8 +108,8 @@ static const char *const valid_s390_opts[] = { NULL, }; -static gboolean -valid_s390_opts_check(const char *option) +gboolean +_nm_setting_wired_is_valid_s390_option(const char *option) { if (NM_MORE_ASSERT_ONCE(10)) { gsize i; @@ -804,7 +804,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) nm_assert(v->name); - if (!valid_s390_opts_check(v->name)) { + if (!_nm_setting_wired_is_valid_s390_option(v->name)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 8b3ae0c097..4b9d12a70f 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -309,7 +309,8 @@ typedef gpointer (*NMUtilsCopyFunc)(gpointer); const char ** _nm_ip_address_get_attribute_names(const NMIPAddress *addr, gboolean sorted, guint *out_length); -void _nm_setting_wired_clear_s390_options(NMSettingWired *setting); +void _nm_setting_wired_clear_s390_options(NMSettingWired *setting); +gboolean _nm_setting_wired_is_valid_s390_option(const char *option); gboolean _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error); const char ** From e25c458b6f37c4207d27f13bd73e7582a5d0834f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 20:36:03 +0100 Subject: [PATCH 12/19] libnm: add _nm_setting_wired_is_valid_s390_option_value() validation function --- src/libnm-core-impl/nm-setting-wired.c | 2 +- src/libnm-core-intern/nm-core-internal.h | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index af6e2eec0d..bcbf064020 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -816,7 +816,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) NM_SETTING_WIRED_S390_OPTIONS); return FALSE; } - if (v->value_str[0] == '\0' || strlen(v->value_str) > 200u) { + if (!_nm_setting_wired_is_valid_s390_option_value(v->value_str)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 4b9d12a70f..d8e69ad831 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -312,6 +312,14 @@ _nm_ip_address_get_attribute_names(const NMIPAddress *addr, gboolean sorted, gui void _nm_setting_wired_clear_s390_options(NMSettingWired *setting); gboolean _nm_setting_wired_is_valid_s390_option(const char *option); +#define NM_SETTING_WIRED_S390_OPTION_MAX_LEN 200u + +static inline gboolean +_nm_setting_wired_is_valid_s390_option_value(const char *option) +{ + return option && option[0] != '\0' && strlen(option) <= NM_SETTING_WIRED_S390_OPTION_MAX_LEN; +} + gboolean _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error); const char ** _nm_ip_route_get_attribute_names(const NMIPRoute *route, gboolean sorted, guint *out_length); From 74a4ee16f5c1097dfd6efef52e2d12c99c88c8f7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 20:36:06 +0100 Subject: [PATCH 13/19] initrd: silently ignore invalid "ethernet.s390-options" --- src/nm-initrd-generator/nmi-cmdline-reader.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index c39745e518..a461890cda 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -978,7 +978,13 @@ reader_parse_rd_znet(Reader *reader, char *argument, gboolean net_ifnames) key = g_strndup(tmp, val - tmp); val[0] = '\0'; val++; - nm_setting_wired_add_s390_option(s_wired, key, val); + if (!_nm_setting_wired_is_valid_s390_option(key) + || !_nm_setting_wired_is_valid_s390_option_value(val)) { + /* Invalid setting. Silently ignore, but also ensure we + * didn't already set it. */ + nm_setting_wired_remove_s390_option(s_wired, key); + } else + nm_setting_wired_add_s390_option(s_wired, key, val); } } } From 7fde244ed20f83c570ed21058bb0ad018e4dcb84 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 15 Mar 2021 17:07:48 +0100 Subject: [PATCH 14/19] libnm: don't assert against valid s390-option keys in nm_setting_wired_add_s390_option() Asserting against user input is not nice, because it always requires the caller to check the value first. Don't do that. Also, don't even check. You can set NM_SETTING_WIRED_S390_OPTIONS property to any values (except duplicated keys). The C add function should not be more limited than that. This is also right because we have verify() which checks for valid settings. And it does so beyond only checking the keys. So you could set NM_SETTING_WIRED_S390_OPTIONS properties to invalid keys. And you could use nm_setting_wired_add_s390_option() to set invalid values. No need to let nm_setting_wired_add_s390_option() check for valid keys. --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 14 ++++++------- src/libnm-core-impl/nm-keyfile.c | 2 ++ src/libnm-core-impl/nm-setting-wired.c | 20 ++++++++----------- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 9100beb8ac..07334dbb77 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -5118,15 +5118,15 @@ make_wired_setting(shvarFile *ifcfg, const char *file, NMSetting8021x **s_8021x, for (i = 0; options && options[i]; i++) { const char *line = options[i]; const char *equals; - gboolean valid = FALSE; equals = strchr(line, '='); - if (equals) { - ((char *) equals)[0] = '\0'; - valid = nm_setting_wired_add_s390_option(s_wired, line, equals + 1); - } - if (!valid) - PARSE_WARNING("invalid s390 OPTION '%s'", line); + if (!equals) + continue; + + /* Here we don't verify the key/value further. If the file contains invalid keys, + * we will later reject the connection as invalid. */ + ((char *) equals)[0] = '\0'; + nm_setting_wired_add_s390_option(s_wired, line, equals + 1); } found = TRUE; } diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index 1de18180a3..bc5398e7ba 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -2286,6 +2286,8 @@ wired_s390_options_parser_full(KeyfileReaderInfo * info, if (!value) continue; + /* Here we don't verify the key/value further. If the file contains invalid keys, + * we will later reject the connection as invalid. */ nm_setting_wired_add_s390_option(s_wired, nm_keyfile_key_decode(keys[i], &key_to_free), value); diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index bcbf064020..ca310fee7c 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -524,7 +524,7 @@ nm_setting_wired_get_s390_option_by_key(NMSettingWired *setting, const char *key gssize idx; g_return_val_if_fail(NM_IS_SETTING_WIRED(setting), NULL); - g_return_val_if_fail(key && key[0], NULL); + g_return_val_if_fail(key, NULL); priv = NM_SETTING_WIRED_GET_PRIVATE(setting); @@ -540,13 +540,13 @@ nm_setting_wired_get_s390_option_by_key(NMSettingWired *setting, const char *key * @key: key name for the option * @value: value for the option * - * Add an option to the table. The option is compared to an internal list - * of allowed options. Key names may contain only alphanumeric characters - * (ie [a-zA-Z0-9]). Adding a new key replaces any existing key/value pair that - * may already exist. + * Add an option to the table. If the key already exists, the value gets + * replaced. * - * Returns: %TRUE if the option was valid and was added to the internal option - * list, %FALSE if it was not. + * Before 1.32, the function would assert that the key is valid. Since then, + * an invalid key gets silently added but renders the profile as invalid. + * + * Returns: since 1.32 this always returns %TRUE. **/ gboolean nm_setting_wired_add_s390_option(NMSettingWired *setting, const char *key, const char *value) @@ -555,13 +555,9 @@ nm_setting_wired_add_s390_option(NMSettingWired *setting, const char *key, const gssize idx; g_return_val_if_fail(NM_IS_SETTING_WIRED(setting), FALSE); + g_return_val_if_fail(key, FALSE); g_return_val_if_fail(value, FALSE); - if (!valid_s390_opts_check(key)) { - g_return_val_if_fail(key, FALSE); - return FALSE; - } - priv = NM_SETTING_WIRED_GET_PRIVATE(setting); idx = nm_utils_named_value_list_find(priv->s390_options.arr, priv->s390_options.len, key, TRUE); From 6e4cdae25638284a77c8487fd062b46b1f5dfd9f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 16 Mar 2021 09:29:53 +0100 Subject: [PATCH 15/19] all: split "range" variant of nm_utils_ptrarray_find_binary_search() nm_utils_ptrarray_find_binary_search() had two additional output arguments: the first and last index -- in case the sorted list contains duplicates. That's nice, and was used in the past. But now, those output arguments are no longer used. So drop them from nm_utils_ptrarray_find_binary_search(). Actually, we could now also drop the previous variant nm_utils_ptrarray_find_binary_search_range(), as it's only used by unit tests. However, although not rocket science, getting this right is not entirely trivial, so lets keep the code in case we need it again. --- src/core/nm-config.c | 8 +--- src/libnm-client-impl/nm-libnm-utils.c | 2 - src/libnm-core-impl/nm-keyfile.c | 2 - src/libnm-core-impl/nm-utils.c | 2 - src/libnm-core-impl/tests/test-general.c | 50 ++++++++++++++++-------- src/libnm-glib-aux/nm-shared-utils.c | 43 ++++++++++++++++++-- src/libnm-glib-aux/nm-shared-utils.h | 14 ++++--- 7 files changed, 83 insertions(+), 38 deletions(-) diff --git a/src/core/nm-config.c b/src/core/nm-config.c index 4d61f1aa22..778ba25900 100644 --- a/src/core/nm-config.c +++ b/src/core/nm-config.c @@ -433,13 +433,7 @@ nm_config_set_no_auto_default_for_device(NMConfig *self, NMDevice *device) len = NM_PTRARRAY_LEN(no_auto_default_current); - idx = nm_utils_ptrarray_find_binary_search((gconstpointer *) no_auto_default_current, - len, - spec, - nm_strcmp_with_data, - NULL, - NULL, - NULL); + idx = nm_utils_strv_find_binary_search(no_auto_default_current, len, spec); if (idx >= 0) { /* @spec is already blocked. We don't have to update our in-memory representation. * Maybe we should write to no_auto_default_file anew, but let's save that too. */ diff --git a/src/libnm-client-impl/nm-libnm-utils.c b/src/libnm-client-impl/nm-libnm-utils.c index fe1b911b68..3cc88ef4d8 100644 --- a/src/libnm-client-impl/nm-libnm-utils.c +++ b/src/libnm-client-impl/nm-libnm-utils.c @@ -751,8 +751,6 @@ nml_dbus_meta_iface_get(const char *dbus_iface_name) G_N_ELEMENTS(_nml_dbus_meta_ifaces), &dbus_iface_name[NM_STRLEN(COMMON_PREFIX)], _strcmp_common_prefix, - NULL, - NULL, NULL); } else return NULL; diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c index bc5398e7ba..bd89345506 100644 --- a/src/libnm-core-impl/nm-keyfile.c +++ b/src/libnm-core-impl/nm-keyfile.c @@ -3087,8 +3087,6 @@ _parse_info_find(NMSetting * setting, NM_PTRARRAY_LEN(pis->properties), &property_name, nm_strcmp_p_with_data, - NULL, - NULL, NULL); if (idx >= 0) pip = pis->properties[idx]; diff --git a/src/libnm-core-impl/nm-utils.c b/src/libnm-core-impl/nm-utils.c index 1296b6db12..202f160ead 100644 --- a/src/libnm-core-impl/nm-utils.c +++ b/src/libnm-core-impl/nm-utils.c @@ -5325,8 +5325,6 @@ _nm_variant_attribute_spec_find_binary_search(const NMVariantAttributeSpec *cons len, &name, nm_strcmp_p_with_data, - NULL, - NULL, NULL); if (idx < 0) return NULL; diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index 0fb130997c..f75401693e 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -8609,7 +8609,7 @@ static void _test_find_binary_search_do(const int *array, gsize len) { gsize i; - gssize idx, idx_first, idx_last; + gssize idx, idx2, idx_first, idx_last; gs_free gconstpointer *parray = g_new(gconstpointer, len); const int NEEDLE = 0; gconstpointer pneedle = GINT_TO_POINTER(NEEDLE); @@ -8620,17 +8620,26 @@ _test_find_binary_search_do(const int *array, gsize len) expected_result = nm_utils_ptrarray_find_first(parray, len, pneedle); - idx = nm_utils_ptrarray_find_binary_search(parray, - len, - pneedle, - _test_find_binary_search_cmp, - NULL, - &idx_first, - &idx_last); + idx = nm_utils_ptrarray_find_binary_search_range(parray, + len, + pneedle, + _test_find_binary_search_cmp, + NULL, + &idx_first, + &idx_last); + + idx2 = nm_utils_ptrarray_find_binary_search(parray, + len, + pneedle, + _test_find_binary_search_cmp, + NULL); + g_assert_cmpint(idx, ==, idx2); + if (expected_result >= 0) { g_assert_cmpint(expected_result, ==, idx); } else { - gssize idx2 = ~idx; + idx2 = ~idx; + g_assert_cmpint(idx, <, 0); g_assert(idx2 >= 0); @@ -8787,13 +8796,13 @@ test_nm_utils_ptrarray_find_binary_search_with_duplicates(void) for (i = 0; i < i_len + BIN_SEARCH_W_DUPS_JITTER; i++) { gconstpointer p = GINT_TO_POINTER(i); - idx = nm_utils_ptrarray_find_binary_search(arr, - i_len, - p, - _test_bin_search2_cmp, - NULL, - &idx_first, - &idx_last); + idx = nm_utils_ptrarray_find_binary_search_range(arr, + i_len, + p, + _test_bin_search2_cmp, + NULL, + &idx_first, + &idx_last); idx_first2 = nm_utils_ptrarray_find_first(arr, i_len, p); @@ -8805,6 +8814,13 @@ test_nm_utils_ptrarray_find_binary_search_with_duplicates(void) NULL); g_assert_cmpint(idx, ==, idx2); + idx2 = nm_utils_ptrarray_find_binary_search(arr, + i_len, + p, + _test_bin_search2_cmp, + NULL); + g_assert_cmpint(idx, ==, idx2); + if (idx_first2 < 0) { g_assert_cmpint(idx, <, 0); g_assert_cmpint(idx, ==, idx_first); @@ -10813,7 +10829,7 @@ main(int argc, char **argv) g_test_add_func("/core/general/_nm_utils_ascii_str_to_int64", test_nm_utils_ascii_str_to_int64); g_test_add_func("/core/general/nm_utils_is_power_of_two", test_nm_utils_is_power_of_two); - g_test_add_func("/core/general/nm_utils_ptrarray_find_binary_search", + g_test_add_func("/core/general/nm_utils_ptrarray_find_binary_search_range", test_nm_utils_ptrarray_find_binary_search); g_test_add_func("/core/general/nm_utils_ptrarray_find_binary_search_with_duplicates", test_nm_utils_ptrarray_find_binary_search_with_duplicates); diff --git a/src/libnm-glib-aux/nm-shared-utils.c b/src/libnm-glib-aux/nm-shared-utils.c index d3effc551d..fe9994459f 100644 --- a/src/libnm-glib-aux/nm-shared-utils.c +++ b/src/libnm-glib-aux/nm-shared-utils.c @@ -3833,9 +3833,46 @@ nm_utils_ptrarray_find_binary_search(gconstpointer * list, gsize len, gconstpointer needle, GCompareDataFunc cmpfcn, - gpointer user_data, - gssize * out_idx_first, - gssize * out_idx_last) + gpointer user_data) +{ + gssize imin, imax, imid; + int cmp; + + g_return_val_if_fail(list || !len, ~((gssize) 0)); + g_return_val_if_fail(cmpfcn, ~((gssize) 0)); + + imin = 0; + if (len > 0) { + imax = len - 1; + + while (imin <= imax) { + imid = imin + (imax - imin) / 2; + + cmp = cmpfcn(list[imid], needle, user_data); + if (cmp == 0) + return imid; + + if (cmp < 0) + imin = imid + 1; + else + imax = imid - 1; + } + } + + /* return the inverse of @imin. This is a negative number, but + * also is ~imin the position where the value should be inserted. */ + imin = ~imin; + return imin; +} + +gssize +nm_utils_ptrarray_find_binary_search_range(gconstpointer * list, + gsize len, + gconstpointer needle, + GCompareDataFunc cmpfcn, + gpointer user_data, + gssize * out_idx_first, + gssize * out_idx_last) { gssize imin, imax, imid, i2min, i2max, i2mid; int cmp; diff --git a/src/libnm-glib-aux/nm-shared-utils.h b/src/libnm-glib-aux/nm-shared-utils.h index 588541e3fe..e33c1c19d6 100644 --- a/src/libnm-glib-aux/nm-shared-utils.h +++ b/src/libnm-glib-aux/nm-shared-utils.h @@ -2018,9 +2018,15 @@ gssize nm_utils_ptrarray_find_binary_search(gconstpointer * list, gsize len, gconstpointer needle, GCompareDataFunc cmpfcn, - gpointer user_data, - gssize * out_idx_first, - gssize * out_idx_last); + gpointer user_data); + +gssize nm_utils_ptrarray_find_binary_search_range(gconstpointer * list, + gsize len, + gconstpointer needle, + GCompareDataFunc cmpfcn, + gpointer user_data, + gssize * out_idx_first, + gssize * out_idx_last); #define nm_utils_strv_find_binary_search(strv, len, needle) \ ({ \ @@ -2035,8 +2041,6 @@ gssize nm_utils_ptrarray_find_binary_search(gconstpointer * list, _len, \ _needle, \ nm_strcmp_with_data, \ - NULL, \ - NULL, \ NULL); \ }) From b377a7d0c9f142a87f6b2a7766032de77b6e55d3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 16 Mar 2021 09:58:48 +0100 Subject: [PATCH 16/19] trivial: fix whitespace for comments in "libnm-glib-aux/nm-macros-internal.h" --- src/libnm-glib-aux/nm-macros-internal.h | 36 ++++++++++++------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/libnm-glib-aux/nm-macros-internal.h b/src/libnm-glib-aux/nm-macros-internal.h index 37dde0173b..ce89e235c1 100644 --- a/src/libnm-glib-aux/nm-macros-internal.h +++ b/src/libnm-glib-aux/nm-macros-internal.h @@ -520,9 +520,9 @@ NM_G_ERROR_MSG(GError *error) #ifndef _NM_CC_SUPPORT_GENERIC /* In the meantime, NetworkManager requires C11 and _Generic() should always be available. - * However, shared/nm-utils may also be used in VPN/applet, which possibly did not yet - * bump the C standard requirement. Leave this for the moment, but eventually we can - * drop it. */ + * However, shared/nm-utils may also be used in VPN/applet, which possibly did not yet + * bump the C standard requirement. Leave this for the moment, but eventually we can + * drop it. */ #if (defined(__GNUC__) && (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 9))) \ || (defined(__clang__)) #define _NM_CC_SUPPORT_GENERIC 1 @@ -682,21 +682,21 @@ NM_G_ERROR_MSG(GError *error) #if _NM_CC_SUPPORT_GENERIC /* these macros cast (value) to - * - "const char **" (for "MC", mutable-const) - * - "const char *const*" (for "CC", const-const) - * The point is to do this cast, but only accepting pointers - * that are compatible already. - * - * The problem is, if you add a function like g_strdupv(), the input - * argument is not modified (CC), but you want to make it work also - * for "char **". C doesn't allow this form of casting (for good reasons), - * so the function makes a choice like g_strdupv(char**). That means, - * every time you want to call it with a const argument, you need to - * explicitly cast it. - * - * These macros do the cast, but they only accept a compatible input - * type, otherwise they will fail compilation. - */ + * - "const char **" (for "MC", mutable-const) + * - "const char *const*" (for "CC", const-const) + * The point is to do this cast, but only accepting pointers + * that are compatible already. + * + * The problem is, if you add a function like g_strdupv(), the input + * argument is not modified (CC), but you want to make it work also + * for "char **". C doesn't allow this form of casting (for good reasons), + * so the function makes a choice like g_strdupv(char**). That means, + * every time you want to call it with a const argument, you need to + * explicitly cast it. + * + * These macros do the cast, but they only accept a compatible input + * type, otherwise they will fail compilation. + */ #define NM_CAST_STRV_MC(value) \ (_Generic ((value), \ const char * *: (const char * *) (value), \ From a9e4d020cbd6e9c6086eee74544dea7ec189a8fe Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 16 Mar 2021 11:56:14 +0100 Subject: [PATCH 17/19] shared/tests: add nmtst_rand_select_str() helper --- src/libnm-glib-aux/nm-test-utils.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 410a55821a..61e4ef63db 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -977,6 +977,8 @@ nmtst_rand_buf(GRand *rand, gpointer buffer, gsize buffer_length) #define nmtst_rand_select(...) _nmtst_rand_select(NM_UNIQ, __VA_ARGS__) +#define nmtst_rand_select_str(x, ...) nmtst_rand_select((const char *) (x), ##__VA_ARGS__) + static inline void * nmtst_rand_perm(GRand *rand, void *dst, const void *src, gsize elmt_size, gsize n_elmt) { From bb132cd6de5299bbe83bf5072c966c706a9dd67b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 16 Mar 2021 11:32:31 +0100 Subject: [PATCH 18/19] libnm: verify ethernet.s390-options.bridge_role value I don't want to fix this for all "ethernet.s390-options" options, but at least strictly validate the newly introduced option. --- src/libnm-core-impl/nm-setting-wired.c | 27 +++++++++++++++++++- src/libnm-core-impl/tests/test-setting.c | 23 +++++++++-------- src/libnm-core-intern/nm-core-internal.h | 11 +++----- src/nm-initrd-generator/nmi-cmdline-reader.c | 2 +- 4 files changed, 43 insertions(+), 20 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wired.c b/src/libnm-core-impl/nm-setting-wired.c index ca310fee7c..0d79f0a5c2 100644 --- a/src/libnm-core-impl/nm-setting-wired.c +++ b/src/libnm-core-impl/nm-setting-wired.c @@ -135,6 +135,31 @@ _nm_setting_wired_is_valid_s390_option(const char *option) >= 0); } +gboolean +_nm_setting_wired_is_valid_s390_option_value(const char *name, const char *option) +{ + nm_assert(name); + + if (!option) + return FALSE; + + /* For historic reasons, the s390-options values were not validated beyond + * simple length check (below). + * + * Here, for certain (recently added) options we add strict validation. + * As this is only done for a few hand picked options, do it right here. + * + * Maybe we should find a backward compatible way to validate all options. + * In that case, the validation should become more elaborate, like we do + * for bond options. */ + + if (nm_streq(name, "bridge_role")) { + return NM_IN_STRSET(option, "primary", "secondary", "none"); + } + + return option[0] != '\0' && strlen(option) <= NM_SETTING_WIRED_S390_OPTION_MAX_LEN; +} + /** * nm_setting_wired_get_port: * @setting: the #NMSettingWired @@ -812,7 +837,7 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) NM_SETTING_WIRED_S390_OPTIONS); return FALSE; } - if (!_nm_setting_wired_is_valid_s390_option_value(v->value_str)) { + if (!_nm_setting_wired_is_valid_s390_option_value(v->name, v->value_str)) { g_set_error(error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, diff --git a/src/libnm-core-impl/tests/test-setting.c b/src/libnm-core-impl/tests/test-setting.c index ea68a02683..0f80826bcb 100644 --- a/src/libnm-core-impl/tests/test-setting.c +++ b/src/libnm-core-impl/tests/test-setting.c @@ -2904,17 +2904,20 @@ _rndt_wired_add_s390_options(NMSettingWired *s_wired, char **out_keyfile_entries opt_vals = g_new0(char *, n_opts + 1); opt_found = g_new0(bool, n_opts + 1); for (i = 0; i < n_opts; i++) { - guint p = nmtst_get_rand_uint32() % 1000; - - if (p < 200) - opt_vals[i] = nm_strdup_int(i); + if (nm_streq(opt_keys[i], "bridge_role")) + opt_vals[i] = g_strdup(nmtst_rand_select_str("primary", "secondary", "none")); else { - opt_vals[i] = g_strdup_printf("%s%s%s%s-%zu", - ((p % 5) % 2) ? "\n" : "", - ((p % 7) % 2) ? "\t" : "", - ((p % 11) % 2) ? "x" : "", - ((p % 13) % 2) ? "=" : "", - i); + guint p = nmtst_get_rand_uint32() % 1000; + if (p < 200) + opt_vals[i] = nm_strdup_int(i); + else { + opt_vals[i] = g_strdup_printf("%s%s%s%s-%zu", + ((p % 5) % 2) ? "\n" : "", + ((p % 7) % 2) ? "\t" : "", + ((p % 11) % 2) ? "x" : "", + ((p % 13) % 2) ? "=" : "", + i); + } } } diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index d8e69ad831..d026520d79 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -309,16 +309,11 @@ typedef gpointer (*NMUtilsCopyFunc)(gpointer); const char ** _nm_ip_address_get_attribute_names(const NMIPAddress *addr, gboolean sorted, guint *out_length); -void _nm_setting_wired_clear_s390_options(NMSettingWired *setting); -gboolean _nm_setting_wired_is_valid_s390_option(const char *option); - #define NM_SETTING_WIRED_S390_OPTION_MAX_LEN 200u -static inline gboolean -_nm_setting_wired_is_valid_s390_option_value(const char *option) -{ - return option && option[0] != '\0' && strlen(option) <= NM_SETTING_WIRED_S390_OPTION_MAX_LEN; -} +void _nm_setting_wired_clear_s390_options(NMSettingWired *setting); +gboolean _nm_setting_wired_is_valid_s390_option(const char *option); +gboolean _nm_setting_wired_is_valid_s390_option_value(const char *name, const char *option); gboolean _nm_ip_route_attribute_validate_all(const NMIPRoute *route, GError **error); const char ** diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index a461890cda..2f50426d8d 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -979,7 +979,7 @@ reader_parse_rd_znet(Reader *reader, char *argument, gboolean net_ifnames) val[0] = '\0'; val++; if (!_nm_setting_wired_is_valid_s390_option(key) - || !_nm_setting_wired_is_valid_s390_option_value(val)) { + || !_nm_setting_wired_is_valid_s390_option_value(key, val)) { /* Invalid setting. Silently ignore, but also ensure we * didn't already set it. */ nm_setting_wired_remove_s390_option(s_wired, key); From c91dfb850ea1360d462ffdd1d9535b9c6da02ded Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 16 Mar 2021 11:37:56 +0100 Subject: [PATCH 19/19] initrd: avoid cloning string in reader_parse_rd_znet() The code did: key = g_strndup(tmp, val - tmp); val[0] = '\0'; That is pointless. If we strndup the key, we don't need to truncate the string at the '='. It might be nicer not to mutate the input string, however, the entire code with "argument" parsing is about mutating the input string, so that is something we apparently are fine with. As such, don't clone the string anymore. --- src/nm-initrd-generator/nmi-cmdline-reader.c | 30 +++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index 2f50426d8d..d6b84c2e57 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -969,23 +969,25 @@ reader_parse_rd_znet(Reader *reader, char *argument, gboolean net_ifnames) NULL); while ((tmp = get_word(&argument, ',')) != NULL) { - char *val; + const char *key; + char * val; val = strchr(tmp, '='); - if (val) { - gs_free char *key = NULL; - - key = g_strndup(tmp, val - tmp); - val[0] = '\0'; - val++; - if (!_nm_setting_wired_is_valid_s390_option(key) - || !_nm_setting_wired_is_valid_s390_option_value(key, val)) { - /* Invalid setting. Silently ignore, but also ensure we - * didn't already set it. */ - nm_setting_wired_remove_s390_option(s_wired, key); - } else - nm_setting_wired_add_s390_option(s_wired, key, val); + if (!val) { + /* an invalid (or empty) entry. Ignore. */ + continue; } + + key = tmp; + val[0] = '\0'; + val++; + if (!_nm_setting_wired_is_valid_s390_option(key) + || !_nm_setting_wired_is_valid_s390_option_value(key, val)) { + /* Invalid setting. Silently ignore, but also ensure we + * didn't already set it. */ + nm_setting_wired_remove_s390_option(s_wired, key); + } else + nm_setting_wired_add_s390_option(s_wired, key, val); } }