From 5cf4d3c7448af0ffe8de918414eedf8b3bb96910 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Feb 2022 19:12:42 +0100 Subject: [PATCH 1/4] glib-aux: hide API g_alloca0() and g_newa0() For one, this API is only available since 2.72, thus we must not use it (unless we would add a compat implementation to nm-glib.h). But also, g_alloca0() evaluates the size argument multiple times, making it non-function like. That seems highly undesirable and error prone. Also, we should be very careful about alloca() and the potential for stack overflow. We use alloca() at times, but usually with macros that are named "*_a()" (to make the danger clearer) and compile time checks for the size. These glib functions make this slightly less safe. Just prevent us from using this API. --- src/libnm-glib-aux/nm-glib.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libnm-glib-aux/nm-glib.h b/src/libnm-glib-aux/nm-glib.h index 4ab9cbbda8..f3be3b32a4 100644 --- a/src/libnm-glib-aux/nm-glib.h +++ b/src/libnm-glib-aux/nm-glib.h @@ -717,4 +717,14 @@ _nm_deprecated("Don't use this API") void _nm_forbidden_glib_api_n(gconstpointer /*****************************************************************************/ +/* g_alloca0() evaluates the "size" argument multiple times. That seems an error + * prone API (as it's not function-like). + * + * We could fix it by using an expression statement. But it doesn't seem + * worth it, so hide it to prevent its use. */ +#undef g_alloca0 +#undef g_newa0 + +/*****************************************************************************/ + #endif /* __NM_GLIB_H__ */ From caf50b96bd34bcf22805fa940d70d99027a1497f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Feb 2022 18:48:47 +0100 Subject: [PATCH 2/4] cli: minor cleanup initializing APInfo in "devices.c" --- src/nmcli/devices.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/nmcli/devices.c b/src/nmcli/devices.c index e23ef681bf..2fab1243a9 100644 --- a/src/nmcli/devices.c +++ b/src/nmcli/devices.c @@ -1261,7 +1261,7 @@ static void fill_output_access_point(gpointer data, gpointer user_data) { NMAccessPoint *ap = NM_ACCESS_POINT(data); - APInfo *info = (APInfo *) user_data; + APInfo *info = user_data; NmcOutputField *arr; gboolean active = FALSE; NM80211ApFlags flags; @@ -3032,7 +3032,6 @@ wifi_print_aps(NMDeviceWifi *wifi, { NMAccessPoint *ap = NULL; const GPtrArray *aps; - APInfo *info; guint i; NmcOutputField *arr; const char *base_hdr = _("Wi-Fi scan list"); @@ -3061,23 +3060,23 @@ wifi_print_aps(NMDeviceWifi *wifi, ap = candidate_ap; } if (ap) { + APInfo info = { + .nmc = nmc, + .index = 1, + .output_flags = 0, + .active_bssid = NULL, + .device = nm_device_get_iface(NM_DEVICE(wifi)), + .output_data = out.output_data, + }; + /* Add headers (field names) */ arr = nmc_dup_fields_array(tmpl, NMC_OF_FLAG_MAIN_HEADER_ADD | NMC_OF_FLAG_FIELD_NAMES); g_ptr_array_add(out.output_data, arr); - info = g_malloc0(sizeof(APInfo)); - info->nmc = nmc; - info->index = 1; - info->output_flags = 0; - info->active_bssid = NULL; - info->device = nm_device_get_iface(NM_DEVICE(wifi)); - info->output_data = out.output_data; - - fill_output_access_point(ap, info); + fill_output_access_point(ap, &info); print_data_prepare_width(out.output_data); print_data(&nmc->nmc_config, &nmc->pager_data, out_indices, header_name, 0, &out); - g_free(info); *bssid_found = TRUE; empty_line = TRUE; From dd42af636a24659f4b46ed48784f6092e5015cec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Feb 2022 18:49:25 +0100 Subject: [PATCH 3/4] cli: change "IN-USE" property to only honor the exact access point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On the D-Bus API, the current access point is referred exactly, by its D-Bus path. Likewise, in libnm's NMClient cache, the access point instance is unique in representing the D-Bus object (meaning, we can directly use pointer equality). Let's not compare the active AP based on the BSSID. It can happen that the scan list contains the same BSSID multiple times (for example on different bands). In that case, the output should only highlight one AP as in-use: $ nmcli device wifi list IN-USE BSSID SSID MODE CHAN RATE SIGNAL BARS SECURITY * E4:0f:4b:2a:c3:d1 MYSSID1 Infra 6 270 Mbit/s 100 ▂▄▆█ WPA2 * E4:0f:4b:2a:c3:d1 MYSSID1 Infra 6 270 Mbit/s 87 ▂▄▆█ WPA2 --- src/nmcli/devices.c | 44 ++++++++++++++++---------------------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/src/nmcli/devices.c b/src/nmcli/devices.c index 2fab1243a9..f5519fe3fb 100644 --- a/src/nmcli/devices.c +++ b/src/nmcli/devices.c @@ -1249,12 +1249,12 @@ sort_access_points(const GPtrArray *aps) } typedef struct { - NmCli *nmc; - int index; - guint32 output_flags; - const char *active_bssid; - const char *device; - GPtrArray *output_data; + NmCli *nmc; + int index; + guint32 output_flags; + NMAccessPoint *active_ap; + const char *device; + GPtrArray *output_data; } APInfo; static void @@ -1263,7 +1263,7 @@ fill_output_access_point(gpointer data, gpointer user_data) NMAccessPoint *ap = NM_ACCESS_POINT(data); APInfo *info = user_data; NmcOutputField *arr; - gboolean active = FALSE; + gboolean active; NM80211ApFlags flags; NM80211ApSecurityFlags wpa_flags, rsn_flags; guint32 freq, bitrate; @@ -1284,11 +1284,7 @@ fill_output_access_point(gpointer data, gpointer user_data) const char *sig_bars; NMMetaColor color; - if (info->active_bssid) { - const char *current_bssid = nm_access_point_get_bssid(ap); - if (current_bssid && !strcmp(current_bssid, info->active_bssid)) - active = TRUE; - } + active = (info->active_ap == ap); /* Get AP properties */ flags = nm_access_point_get_flags(ap); @@ -1679,18 +1675,14 @@ show_device_info(NMDevice *device, NmCli *nmc) /* Wireless specific information */ if ((NM_IS_DEVICE_WIFI(device))) { - NMAccessPoint *active_ap = NULL; - const char *active_bssid = NULL; - /* section AP */ if (!g_ascii_strcasecmp(nmc_fields_dev_show_sections[section_idx]->name, nmc_fields_dev_show_sections[4]->name)) { + NMAccessPoint *active_ap = NULL; NMC_OUTPUT_DATA_DEFINE_SCOPED(out); - if (state == NM_DEVICE_STATE_ACTIVATED) { - active_ap = nm_device_wifi_get_active_access_point(NM_DEVICE_WIFI(device)); - active_bssid = active_ap ? nm_access_point_get_bssid(active_ap) : NULL; - } + if (state == NM_DEVICE_STATE_ACTIVATED) + active_ap = nm_device_wifi_get_active_access_point(NM_DEVICE_WIFI(device)); tmpl = (const NMMetaAbstractInfo *const *) nmc_fields_dev_wifi_list; out_indices = @@ -1708,7 +1700,7 @@ show_device_info(NMDevice *device, NmCli *nmc) .nmc = nmc, .index = 1, .output_flags = NMC_OF_FLAG_SECTION_PREFIX, - .active_bssid = active_bssid, + .active_ap = active_ap, .device = nm_device_get_iface(device), .output_data = out.output_data, }; @@ -2991,14 +2983,11 @@ find_ap_on_device(NMDevice *device, const char *bssid, const char *ssid, gboolea static void show_access_point_info(NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) { - NMAccessPoint *active_ap = NULL; - const char *active_bssid = NULL; + NMAccessPoint *active_ap = NULL; NmcOutputField *arr; - if (nm_device_get_state(NM_DEVICE(wifi)) == NM_DEVICE_STATE_ACTIVATED) { - active_ap = nm_device_wifi_get_active_access_point(wifi); - active_bssid = active_ap ? nm_access_point_get_bssid(active_ap) : NULL; - } + if (nm_device_get_state(NM_DEVICE(wifi)) == NM_DEVICE_STATE_ACTIVATED) + active_ap = nm_device_wifi_get_active_access_point(wifi); arr = nmc_dup_fields_array((const NMMetaAbstractInfo *const *) nmc_fields_dev_wifi_list, NMC_OF_FLAG_MAIN_HEADER_ADD | NMC_OF_FLAG_FIELD_NAMES); @@ -3010,7 +2999,7 @@ show_access_point_info(NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) .nmc = nmc, .index = 1, .output_flags = 0, - .active_bssid = active_bssid, + .active_ap = active_ap, .device = nm_device_get_iface(NM_DEVICE(wifi)), .output_data = out->output_data, }; @@ -3064,7 +3053,6 @@ wifi_print_aps(NMDeviceWifi *wifi, .nmc = nmc, .index = 1, .output_flags = 0, - .active_bssid = NULL, .device = nm_device_get_iface(NM_DEVICE(wifi)), .output_data = out.output_data, }; From 33584f2134759c5fa95eed8a3d43e047b6e510e6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Feb 2022 19:20:37 +0100 Subject: [PATCH 4/4] cli: make APInfo parameter to fill_output_access_point() const It's helpful to control when data/state gets mutated. In particular, when passing on a pointer via several hops. C can help with that at compile time via "const". But the "index" field of APInfo is actually mutable, as it counts the lines. So most of the data is immutable, but the index. Make APInfo const. But to do that, the mutable part must be moved to a separate place. Also, start with the counter initialized to zero instead of one. It is just nicer. --- src/nmcli/devices.c | 47 ++++++++++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/src/nmcli/devices.c b/src/nmcli/devices.c index f5519fe3fb..ded2a9eb8c 100644 --- a/src/nmcli/devices.c +++ b/src/nmcli/devices.c @@ -1250,18 +1250,16 @@ sort_access_points(const GPtrArray *aps) typedef struct { NmCli *nmc; - int index; - guint32 output_flags; NMAccessPoint *active_ap; const char *device; GPtrArray *output_data; + int *p_index; + guint32 output_flags; } APInfo; static void -fill_output_access_point(gpointer data, gpointer user_data) +fill_output_access_point(NMAccessPoint *ap, const APInfo *info) { - NMAccessPoint *ap = NM_ACCESS_POINT(data); - APInfo *info = user_data; NmcOutputField *arr; gboolean active; NM80211ApFlags flags; @@ -1344,7 +1342,7 @@ fill_output_access_point(gpointer data, gpointer user_data) arr = nmc_dup_fields_array((const NMMetaAbstractInfo *const *) nmc_fields_dev_wifi_list, info->output_flags); - ap_name = g_strdup_printf("AP[%d]", info->index++); /* AP */ + ap_name = g_strdup_printf("AP[%d]", ++(*info->p_index)); /* AP */ set_val_str(arr, 0, ap_name); set_val_str(arr, 1, ssid_str); set_val_str(arr, 2, ssid_hex_str); @@ -1377,6 +1375,12 @@ fill_output_access_point(gpointer data, gpointer user_data) g_ptr_array_add(info->output_data, arr); } +static void +fill_output_access_point_void(gpointer data, gpointer user_data) +{ + fill_output_access_point(data, user_data); +} + static char * bluetooth_caps_to_string(NMBluetoothCapabilities caps) { @@ -1695,10 +1699,11 @@ show_device_info(NMDevice *device, NmCli *nmc) g_ptr_array_add(out.output_data, arr); { - gs_unref_ptrarray GPtrArray *aps = NULL; - APInfo info = { + gs_unref_ptrarray GPtrArray *aps = NULL; + int info_index = 0; + const APInfo info = { .nmc = nmc, - .index = 1, + .p_index = &info_index, .output_flags = NMC_OF_FLAG_SECTION_PREFIX, .active_ap = active_ap, .device = nm_device_get_iface(device), @@ -1707,7 +1712,7 @@ show_device_info(NMDevice *device, NmCli *nmc) aps = sort_access_points( nm_device_wifi_get_access_points(NM_DEVICE_WIFI(device))); - g_ptr_array_foreach(aps, fill_output_access_point, &info); + g_ptr_array_foreach(aps, fill_output_access_point_void, (gpointer) &info); } print_data_prepare_width(out.output_data); @@ -2994,10 +2999,11 @@ show_access_point_info(NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) g_ptr_array_add(out->output_data, arr); { - gs_unref_ptrarray GPtrArray *aps = NULL; - APInfo info = { + gs_unref_ptrarray GPtrArray *aps = NULL; + int info_index = 0; + const APInfo info = { .nmc = nmc, - .index = 1, + .p_index = &info_index, .output_flags = 0, .active_ap = active_ap, .device = nm_device_get_iface(NM_DEVICE(wifi)), @@ -3005,7 +3011,7 @@ show_access_point_info(NMDeviceWifi *wifi, NmCli *nmc, NmcOutputData *out) }; aps = sort_access_points(nm_device_wifi_get_access_points(wifi)); - g_ptr_array_foreach(aps, fill_output_access_point, &info); + g_ptr_array_foreach(aps, fill_output_access_point_void, (gpointer) &info); } print_data_prepare_width(out->output_data); @@ -3049,12 +3055,13 @@ wifi_print_aps(NMDeviceWifi *wifi, ap = candidate_ap; } if (ap) { - APInfo info = { - .nmc = nmc, - .index = 1, - .output_flags = 0, - .device = nm_device_get_iface(NM_DEVICE(wifi)), - .output_data = out.output_data, + int info_index = 0; + const APInfo info = { + .nmc = nmc, + .p_index = &info_index, + .output_flags = 0, + .device = nm_device_get_iface(NM_DEVICE(wifi)), + .output_data = out.output_data, }; /* Add headers (field names) */