From 641e7177975189929df2ce36c055fe0e660b7fd6 Mon Sep 17 00:00:00 2001 From: Jan Vaclav Date: Mon, 22 Jan 2024 13:53:27 +0100 Subject: [PATCH 1/4] libnm-core: fix mac comparison in _remove_mac_blacklist_item_by_value The comparison checking for MAC address equality had previously been flipped around. Fixes: b084ad7f2b0d ('libnm-core: canonicalize hardware addresses in settings') --- src/libnm-core-impl/nm-setting-wireless.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting-wireless.c b/src/libnm-core-impl/nm-setting-wireless.c index 152bbde3d9..244dcdccb7 100644 --- a/src/libnm-core-impl/nm-setting-wireless.c +++ b/src/libnm-core-impl/nm-setting-wireless.c @@ -600,7 +600,7 @@ nm_setting_wireless_remove_mac_blacklist_item_by_value(NMSettingWireless *settin priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); for (i = 0; i < priv->mac_address_blacklist->len; i++) { candidate = nm_g_array_index(priv->mac_address_blacklist, char *, i); - if (!nm_utils_hwaddr_matches(mac, -1, candidate, -1)) { + if (nm_utils_hwaddr_matches(mac, -1, candidate, -1)) { g_array_remove_index(priv->mac_address_blacklist, i); _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); return TRUE; From 66887fafef6a20d3180411ac8f73de9ddb421b20 Mon Sep 17 00:00:00 2001 From: Jan Vaclav Date: Wed, 22 Nov 2023 12:07:41 +0100 Subject: [PATCH 2/4] libnm-core: add MAC address handling to _property_direct_set_strv function https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1803 --- src/libnm-core-impl/nm-setting.c | 25 ++++++++++++++++++++++-- src/libnm-core-intern/nm-core-internal.h | 4 ++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index 758338e5ce..fc3f42f8bc 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -8,6 +8,8 @@ #include "nm-setting.h" +#include + #include "libnm-core-intern/nm-core-internal.h" #include "libnm-glib-aux/nm-ref-string.h" #include "libnm-glib-aux/nm-secret-utils.h" @@ -739,10 +741,29 @@ _property_direct_set_strv(const NMSettInfoSetting *sett_info, if (!property_info->direct_strv_preserve_empty && strv && !strv[0]) strv = NULL; - if (nm_strvarray_equal_strv(p_val->arr, strv, -1)) - return FALSE; + if (property_info->direct_set_strv_normalize_hwaddr) { + gs_unref_array GArray *arr = NULL; + if (strv) { + nm_strvarray_ensure(&arr); + for (; strv[0]; strv++) { + nm_strvarray_add_take(arr, + _nm_utils_hwaddr_canonical_or_invalid(strv[0], ETH_ALEN)); + } + } + + if (nm_strvarray_equal(p_val->arr, arr)) + return FALSE; + + NM_SWAP(&p_val->arr, &arr); + return TRUE; + } + + if (nm_strvarray_equal_strv(p_val->arr, strv, -1)) { + return FALSE; + } nm_strvarray_set_strv_full(&p_val->arr, strv, property_info->direct_strv_preserve_empty); + return TRUE; } diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index 8e2573370b..f64cdd873c 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -815,6 +815,10 @@ struct _NMSettInfoProperty { * normalize the string via g_ascii_strdown(). */ bool direct_set_string_ascii_strdown : 1; + /* If TRUE, this is a NM_VALUE_TYPE_STRV direct property holding MAC addresses, + * and the setter will normalize them via _nm_utils_hwaddr_canonical_or_invalid(). */ + bool direct_set_strv_normalize_hwaddr : 1; + /* If TRUE, this is a NM_VALUE_TYPE_STRING direct property, and the setter will * normalize the string via g_strstrip(). */ bool direct_set_string_strip : 1; From 63c4827af23cf741d47d2e393bb491cfec12c6c5 Mon Sep 17 00:00:00 2001 From: Jan Vaclav Date: Mon, 8 Jan 2024 12:45:56 +0100 Subject: [PATCH 3/4] libnm-core: add direct_strv_not_null property This property indicates that a non-null strv array is expected, and an empty strv array should be returned instead of NULL if it hadn't been created yet. --- src/libnm-core-impl/nm-setting.c | 2 +- src/libnm-core-intern/nm-core-internal.h | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting.c b/src/libnm-core-impl/nm-setting.c index fc3f42f8bc..f3dc6442a3 100644 --- a/src/libnm-core-impl/nm-setting.c +++ b/src/libnm-core-impl/nm-setting.c @@ -859,7 +859,7 @@ _nm_setting_property_get_property_direct(GObject *object, value, nm_strvarray_get_strv_full_dup(p_val->arr, NULL, - FALSE, + property_info->direct_strv_not_null, property_info->direct_strv_preserve_empty)); return; } diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index f64cdd873c..b9af77572f 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -868,6 +868,10 @@ struct _NMSettInfoProperty { * an empty array. */ bool direct_strv_preserve_empty : 1; + /* This flag indicates that an empty strv array should be returned + * instead of NULL if it hadn't been created yet. */ + bool direct_strv_not_null : 1; + /* Usually, properties that are set to the default value for the GParamSpec * are not serialized to GVariant (and NULL is returned by to_dbus_data(). * Set this flag to force always converting the property even if the value From 49322bb66cfd9708757808864a4abaea8584fcfd Mon Sep 17 00:00:00 2001 From: Jan Vaclav Date: Mon, 8 Jan 2024 12:46:53 +0100 Subject: [PATCH 4/4] libnm-core: replace mac_address_blacklist type GArray with NMValueStrv This replaces the underlying type of mac_address_blacklist, which is currently GArray, with a more re-usable NMValueStrv, which allows us to implement it as a direct property. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1803 --- src/libnm-core-impl/nm-setting-wireless.c | 183 ++++++++++------------ 1 file changed, 83 insertions(+), 100 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-wireless.c b/src/libnm-core-impl/nm-setting-wireless.c index 244dcdccb7..93461a99da 100644 --- a/src/libnm-core-impl/nm-setting-wireless.c +++ b/src/libnm-core-impl/nm-setting-wireless.c @@ -46,24 +46,24 @@ NM_GOBJECT_PROPERTIES_DEFINE(NMSettingWireless, PROP_AP_ISOLATION, ); typedef struct { - GBytes *ssid; - GArray *mac_address_blacklist; - GPtrArray *seen_bssids; - char *mode; - char *band; - char *bssid; - char *device_mac_address; - char *cloned_mac_address; - char *generate_mac_address_mask; - int ap_isolation; - guint32 mac_address_randomization; - guint32 channel; - guint32 rate; - guint32 tx_power; - guint32 mtu; - guint32 powersave; - guint32 wake_on_wlan; - bool hidden; + GBytes *ssid; + GPtrArray *seen_bssids; + char *mode; + char *band; + char *bssid; + char *device_mac_address; + char *cloned_mac_address; + char *generate_mac_address_mask; + NMValueStrv mac_address_blacklist; + int ap_isolation; + guint32 mac_address_randomization; + guint32 channel; + guint32 rate; + guint32 tx_power; + guint32 mtu; + guint32 powersave; + guint32 wake_on_wlan; + bool hidden; } NMSettingWirelessPrivate; /** @@ -470,12 +470,11 @@ nm_setting_wireless_get_generate_mac_address_mask(NMSettingWireless *setting) const char *const * nm_setting_wireless_get_mac_address_blacklist(NMSettingWireless *setting) { - NMSettingWirelessPrivate *priv; - g_return_val_if_fail(NM_IS_SETTING_WIRELESS(setting), NULL); - priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - return nm_g_array_data(priv->mac_address_blacklist); + return nm_strvarray_get_strv_notnull( + NM_SETTING_WIRELESS_GET_PRIVATE(setting)->mac_address_blacklist.arr, + NULL); } /** @@ -489,7 +488,7 @@ nm_setting_wireless_get_num_mac_blacklist_items(NMSettingWireless *setting) { g_return_val_if_fail(NM_IS_SETTING_WIRELESS(setting), 0); - return NM_SETTING_WIRELESS_GET_PRIVATE(setting)->mac_address_blacklist->len; + return nm_g_array_len(NM_SETTING_WIRELESS_GET_PRIVATE(setting)->mac_address_blacklist.arr); } /** @@ -505,19 +504,11 @@ nm_setting_wireless_get_num_mac_blacklist_items(NMSettingWireless *setting) const char * nm_setting_wireless_get_mac_blacklist_item(NMSettingWireless *setting, guint32 idx) { - NMSettingWirelessPrivate *priv; - g_return_val_if_fail(NM_IS_SETTING_WIRELESS(setting), NULL); - priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - - if (idx == priv->mac_address_blacklist->len) { - return NULL; - } - - g_return_val_if_fail(idx < priv->mac_address_blacklist->len, NULL); - - return nm_g_array_index(priv->mac_address_blacklist, const char *, idx); + return nm_strvarray_get_idxnull_or_greturn( + NM_SETTING_WIRELESS_GET_PRIVATE(setting)->mac_address_blacklist.arr, + idx); } /** @@ -534,24 +525,28 @@ gboolean nm_setting_wireless_add_mac_blacklist_item(NMSettingWireless *setting, const char *mac) { NMSettingWirelessPrivate *priv; + guint8 mac_bin[ETH_ALEN]; const char *candidate; - int i; + guint i; + guint len; g_return_val_if_fail(NM_IS_SETTING_WIRELESS(setting), FALSE); g_return_val_if_fail(mac != NULL, FALSE); - if (!nm_utils_hwaddr_valid(mac, ETH_ALEN)) + if (!_nm_utils_hwaddr_aton_exact(mac, mac_bin, ETH_ALEN)) return FALSE; priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - for (i = 0; i < priv->mac_address_blacklist->len; i++) { - candidate = nm_g_array_index(priv->mac_address_blacklist, char *, i); - if (nm_utils_hwaddr_matches(mac, -1, candidate, -1)) + len = nm_g_array_len(priv->mac_address_blacklist.arr); + for (i = 0; i < len; i++) { + candidate = nm_g_array_index(priv->mac_address_blacklist.arr, char *, i); + if (nm_utils_hwaddr_matches(mac_bin, ETH_ALEN, candidate, -1)) return FALSE; } mac = nm_utils_hwaddr_canonical(mac, ETH_ALEN); - g_array_append_val(priv->mac_address_blacklist, mac); + nm_g_array_append_simple(nm_strvarray_ensure(&priv->mac_address_blacklist.arr), + nm_utils_hwaddr_ntoa(mac_bin, ETH_ALEN)); _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); return TRUE; } @@ -571,9 +566,13 @@ nm_setting_wireless_remove_mac_blacklist_item(NMSettingWireless *setting, guint3 g_return_if_fail(NM_IS_SETTING_WIRELESS(setting)); priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - g_return_if_fail(idx < priv->mac_address_blacklist->len); + if (!priv->mac_address_blacklist.arr) { + return; + } - g_array_remove_index(priv->mac_address_blacklist, idx); + g_return_if_fail(idx < priv->mac_address_blacklist.arr->len); + + g_array_remove_index(priv->mac_address_blacklist.arr, idx); _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); } @@ -591,21 +590,29 @@ gboolean nm_setting_wireless_remove_mac_blacklist_item_by_value(NMSettingWireless *setting, const char *mac) { NMSettingWirelessPrivate *priv; + guint8 mac_bin[ETH_ALEN]; const char *candidate; - int i; + guint i; g_return_val_if_fail(NM_IS_SETTING_WIRELESS(setting), FALSE); g_return_val_if_fail(mac != NULL, FALSE); + if (!_nm_utils_hwaddr_aton_exact(mac, mac_bin, ETH_ALEN)) + return FALSE; + priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - for (i = 0; i < priv->mac_address_blacklist->len; i++) { - candidate = nm_g_array_index(priv->mac_address_blacklist, char *, i); - if (nm_utils_hwaddr_matches(mac, -1, candidate, -1)) { - g_array_remove_index(priv->mac_address_blacklist, i); - _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); - return TRUE; + + if (priv->mac_address_blacklist.arr) { + for (i = 0; i < priv->mac_address_blacklist.arr->len; i++) { + candidate = nm_g_array_index(priv->mac_address_blacklist.arr, char *, i); + if (nm_utils_hwaddr_matches(mac_bin, ETH_ALEN, candidate, -1)) { + g_array_remove_index(priv->mac_address_blacklist.arr, i); + _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); + return TRUE; + } } } + return FALSE; } @@ -620,8 +627,8 @@ nm_setting_wireless_clear_mac_blacklist_items(NMSettingWireless *setting) { g_return_if_fail(NM_IS_SETTING_WIRELESS(setting)); - g_array_set_size(NM_SETTING_WIRELESS_GET_PRIVATE(setting)->mac_address_blacklist, 0); - _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); + if (nm_strvarray_clear(&NM_SETTING_WIRELESS_GET_PRIVATE(setting)->mac_address_blacklist.arr)) + _notify(setting, PROP_MAC_ADDRESS_BLACKLIST); } /** @@ -1041,20 +1048,22 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) return FALSE; } - for (i = 0; i < priv->mac_address_blacklist->len; i++) { - const char *mac = nm_g_array_index(priv->mac_address_blacklist, const char *, i); + if (priv->mac_address_blacklist.arr) { + for (i = 0; i < priv->mac_address_blacklist.arr->len; i++) { + const char *mac = nm_g_array_index(priv->mac_address_blacklist.arr, const char *, i); - if (!nm_utils_hwaddr_valid(mac, ETH_ALEN)) { - g_set_error(error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("'%s' is not a valid MAC address"), - mac); - g_prefix_error(error, - "%s.%s: ", - NM_SETTING_WIRELESS_SETTING_NAME, - NM_SETTING_WIRELESS_MAC_ADDRESS_BLACKLIST); - return FALSE; + if (!nm_utils_hwaddr_valid(mac, ETH_ALEN)) { + g_set_error(error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("'%s' is not a valid MAC address"), + mac); + g_prefix_error(error, + "%s.%s: ", + NM_SETTING_WIRELESS_SETTING_NAME, + NM_SETTING_WIRELESS_MAC_ADDRESS_BLACKLIST); + return FALSE; + } } } @@ -1216,12 +1225,6 @@ nm_setting_wireless_get_wake_on_wlan(NMSettingWireless *setting) return NM_SETTING_WIRELESS_GET_PRIVATE(setting)->wake_on_wlan; } -static void -clear_blacklist_item(char **item_p) -{ - g_free(*item_p); -} - /*****************************************************************************/ static void @@ -1234,9 +1237,6 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec) case PROP_CLONED_MAC_ADDRESS: g_value_set_string(value, nm_setting_wireless_get_cloned_mac_address(setting)); break; - case PROP_MAC_ADDRESS_BLACKLIST: - g_value_set_boxed(value, nm_g_array_data(priv->mac_address_blacklist)); - break; case PROP_SEEN_BSSIDS: g_value_take_boxed( value, @@ -1255,8 +1255,6 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps { NMSettingWireless *self = NM_SETTING_WIRELESS(object); NMSettingWirelessPrivate *priv = NM_SETTING_WIRELESS_GET_PRIVATE(self); - const char *const *blacklist; - const char *mac; gboolean bool_val; _PropertyEnums prop1 = PROP_0; _PropertyEnums prop2 = PROP_0; @@ -1281,18 +1279,6 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps nm_gobject_notify_together(self, prop1, prop2); break; - case PROP_MAC_ADDRESS_BLACKLIST: - blacklist = g_value_get_boxed(value); - g_array_set_size(priv->mac_address_blacklist, 0); - if (blacklist && blacklist[0]) { - gsize i; - - for (i = 0; blacklist[i]; i++) { - mac = _nm_utils_hwaddr_canonical_or_invalid(blacklist[i], ETH_ALEN); - g_array_append_val(priv->mac_address_blacklist, mac); - } - } - break; case PROP_SEEN_BSSIDS: { gs_unref_ptrarray GPtrArray *arr_old = NULL; @@ -1321,13 +1307,7 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps static void nm_setting_wireless_init(NMSettingWireless *setting) -{ - NMSettingWirelessPrivate *priv = NM_SETTING_WIRELESS_GET_PRIVATE(setting); - - /* We use GArray rather than GPtrArray so it will automatically be NULL-terminated */ - priv->mac_address_blacklist = g_array_new(TRUE, FALSE, sizeof(char *)); - g_array_set_clear_func(priv->mac_address_blacklist, (GDestroyNotify) clear_blacklist_item); -} +{} /** * nm_setting_wireless_new: @@ -1348,7 +1328,6 @@ finalize(GObject *object) NMSettingWirelessPrivate *priv = NM_SETTING_WIRELESS_GET_PRIVATE(object); g_free(priv->cloned_mac_address); - g_array_unref(priv->mac_address_blacklist); nm_g_ptr_array_unref(priv->seen_bssids); G_OBJECT_CLASS(nm_setting_wireless_parent_class)->finalize(object); @@ -1733,11 +1712,15 @@ nm_setting_wireless_class_init(NMSettingWirelessClass *klass) * is listed. * ---end--- */ - _nm_setting_property_define_gprop_strv_oldstyle(properties_override, - obj_properties, - NM_SETTING_WIRELESS_MAC_ADDRESS_BLACKLIST, - PROP_MAC_ADDRESS_BLACKLIST, - NM_SETTING_PARAM_FUZZY_IGNORE); + _nm_setting_property_define_direct_strv(properties_override, + obj_properties, + NM_SETTING_WIRELESS_MAC_ADDRESS_BLACKLIST, + PROP_MAC_ADDRESS_BLACKLIST, + NM_SETTING_PARAM_FUZZY_IGNORE, + NMSettingWirelessPrivate, + mac_address_blacklist, + .direct_set_strv_normalize_hwaddr = TRUE, + .direct_strv_not_null = TRUE); /** * NMSettingWireless:seen-bssids: