From 5ed1edc02a064d48017b56bfa929ec4c08795a10 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 11:44:47 +0100 Subject: [PATCH 1/4] device/wifi: fix memleak parsing SSID arguments for "RequestScan" Oddly enough, valgrind was not complaining about this leak... Fixes: 87b2d783b6c7 ('core: accept 'ssids':aay option in RequestScan() dictionary parameter') --- src/devices/wifi/nm-device-wifi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 75b9943c80..260a4ca06b 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -1075,7 +1075,6 @@ static GPtrArray * ssids_options_to_ptrarray (GVariant *value, GError **error) { GPtrArray *ssids = NULL; - GVariant *v; const guint8 *bytes; gsize len; int num_ssids, i; @@ -1092,6 +1091,8 @@ ssids_options_to_ptrarray (GVariant *value, GError **error) if (num_ssids) { ssids = g_ptr_array_new_full (num_ssids, (GDestroyNotify) g_bytes_unref); for (i = 0; i < num_ssids; i++) { + gs_unref_variant GVariant *v = NULL; + v = g_variant_get_child_value (value, i); bytes = g_variant_get_fixed_array (v, &len, sizeof (guint8)); if (len > 32) { From 7d8da6c9c1506f554a84a0edb5f26a7c7e833285 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 13:55:07 +0100 Subject: [PATCH 2/4] build: build intermediate library with core wifi for device-plugin and tests Don't build the same sources multiple times. The test code should statically link against the tested code, just like the device plugin that uses the code in production. --- Makefile.am | 34 +++++++++++++++++++------- src/devices/wifi/meson.build | 46 +++++++++++++++++++++--------------- 2 files changed, 52 insertions(+), 28 deletions(-) diff --git a/Makefile.am b/Makefile.am index f8020794b4..1673fc1fc3 100644 --- a/Makefile.am +++ b/Makefile.am @@ -3606,9 +3606,9 @@ EXTRA_DIST += \ if WITH_WIFI -core_plugins += src/devices/wifi/libnm-device-plugin-wifi.la +noinst_LTLIBRARIES += src/devices/wifi/libnm-wifi-base.la -src_devices_wifi_libnm_device_plugin_wifi_la_SOURCES = \ +src_devices_wifi_libnm_wifi_base_la_SOURCES = \ src/devices/wifi/nm-device-olpc-mesh.c \ src/devices/wifi/nm-device-olpc-mesh.h \ src/devices/wifi/nm-device-wifi-p2p.c \ @@ -3619,7 +3619,6 @@ src_devices_wifi_libnm_device_plugin_wifi_la_SOURCES = \ src/devices/wifi/nm-wifi-ap.h \ src/devices/wifi/nm-wifi-common.c \ src/devices/wifi/nm-wifi-common.h \ - src/devices/wifi/nm-wifi-factory.c \ src/devices/wifi/nm-wifi-p2p-peer.c \ src/devices/wifi/nm-wifi-p2p-peer.h \ src/devices/wifi/nm-wifi-utils.c \ @@ -3627,7 +3626,7 @@ src_devices_wifi_libnm_device_plugin_wifi_la_SOURCES = \ $(NULL) if WITH_IWD -src_devices_wifi_libnm_device_plugin_wifi_la_SOURCES += \ +src_devices_wifi_libnm_wifi_base_la_SOURCES += \ src/devices/wifi/nm-device-iwd.c \ src/devices/wifi/nm-device-iwd.h \ src/devices/wifi/nm-iwd-manager.c \ @@ -3635,6 +3634,19 @@ src_devices_wifi_libnm_device_plugin_wifi_la_SOURCES += \ $(NULL) endif +src_devices_wifi_libnm_wifi_base_la_CPPFLAGS = $(src_cppflags_device_plugin) + +src_devices_wifi_libnm_wifi_base_la_LIBADD = \ + $(GLIB_LIBS) + +$(src_devices_wifi_libnm_wifi_base_la_OBJECTS): $(libnm_core_lib_h_pub_mkenums) + +core_plugins += src/devices/wifi/libnm-device-plugin-wifi.la + +src_devices_wifi_libnm_device_plugin_wifi_la_SOURCES = \ + src/devices/wifi/nm-wifi-factory.c \ + $(NULL) + src_devices_wifi_libnm_device_plugin_wifi_la_CPPFLAGS = $(src_cppflags_device_plugin) src_devices_wifi_libnm_device_plugin_wifi_la_LDFLAGS = \ @@ -3642,8 +3654,11 @@ src_devices_wifi_libnm_device_plugin_wifi_la_LDFLAGS = \ -Wl,--version-script="$(srcdir)/linker-script-devices.ver" src_devices_wifi_libnm_device_plugin_wifi_la_LIBADD = \ + src/devices/wifi/libnm-wifi-base.la \ $(GLIB_LIBS) +$(src_devices_wifi_libnm_device_plugin_wifi_la_OBJECTS): $(libnm_core_lib_h_pub_mkenums) + check-local-devices-wifi: src/devices/wifi/libnm-device-plugin-wifi.la $(srcdir)/tools/check-exports.sh $(builddir)/src/devices/wifi/.libs/libnm-device-plugin-wifi.so "$(srcdir)/linker-script-devices.ver" $(call check_so_symbols,$(builddir)/src/devices/wifi/.libs/libnm-device-plugin-wifi.so) @@ -3654,14 +3669,15 @@ check_programs += src/devices/wifi/tests/test-devices-wifi src_devices_wifi_tests_test_devices_wifi_SOURCES = \ src/devices/wifi/tests/test-devices-wifi.c \ - src/devices/wifi/nm-wifi-ap.c \ - src/devices/wifi/nm-wifi-ap.h \ - src/devices/wifi/nm-wifi-utils.c \ - src/devices/wifi/nm-wifi-utils.h + $(NULL) src_devices_wifi_tests_test_devices_wifi_CPPFLAGS = $(src_cppflags_base_test) -src_devices_wifi_tests_test_devices_wifi_LDADD = src/libNetworkManagerTest.la +src_devices_wifi_tests_test_devices_wifi_LDADD = \ + src/libNetworkManagerTest.la \ + src/devices/wifi/libnm-wifi-base.la \ + $(NULL) + src_devices_wifi_tests_test_devices_wifi_LDFLAGS = $(SANITIZER_EXEC_LDFLAGS) $(src_devices_wifi_tests_test_devices_wifi_OBJECTS): $(libnm_core_lib_h_pub_mkenums) diff --git a/src/devices/wifi/meson.build b/src/devices/wifi/meson.build index 6566f20119..9f18a36b18 100644 --- a/src/devices/wifi/meson.build +++ b/src/devices/wifi/meson.build @@ -1,28 +1,36 @@ -common_sources = files( - 'nm-wifi-ap.c', - 'nm-wifi-p2p-peer.c', - 'nm-wifi-utils.c', -) - -sources = common_sources + files( - 'nm-device-olpc-mesh.c', - 'nm-device-wifi-p2p.c', - 'nm-device-wifi.c', - 'nm-wifi-common.c', - 'nm-wifi-factory.c', -) - +iwd_sources = files() if enable_iwd - sources += files( + iwd_sources += files( 'nm-device-iwd.c', 'nm-iwd-manager.c', ) endif +libnm_wifi_base = static_library( + 'nm-wifi-base', + sources: files( + 'nm-device-olpc-mesh.c', + 'nm-device-wifi-p2p.c', + 'nm-device-wifi.c', + 'nm-wifi-ap.c', + 'nm-wifi-common.c', + 'nm-wifi-p2p-peer.c', + 'nm-wifi-utils.c', + ) + iwd_sources, + dependencies: daemon_nm_default_dep, + c_args: daemon_c_flags, +) + +libnm_wifi_base_dep = declare_dependency( + link_with: libnm_wifi_base, +) + libnm_device_plugin_wifi = shared_module( 'nm-device-plugin-wifi', - sources: sources, - dependencies: daemon_nm_default_dep, + sources: files( + 'nm-wifi-factory.c', + ), + dependencies: [ daemon_nm_default_dep, libnm_wifi_base_dep ], c_args: daemon_c_flags, link_args: ldflags_linker_script_devices, link_depends: linker_script_devices, @@ -43,8 +51,8 @@ if enable_tests exe = executable( test_unit, - ['tests/' + test_unit + '.c'] + common_sources, - dependencies: libnetwork_manager_test_dep, + 'tests/' + test_unit + '.c', + dependencies: [ libnetwork_manager_test_dep, libnm_wifi_base_dep ], c_args: test_c_flags, ) From 023dc9646c468d276aac78bf4249a9dd80da6772 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 13:56:06 +0100 Subject: [PATCH 3/4] wifi/tests: add test for ssids_options_to_ptrarray() --- src/devices/wifi/nm-device-wifi.c | 8 +++ src/devices/wifi/nm-device-wifi.h | 2 + src/devices/wifi/tests/test-devices-wifi.c | 57 ++++++++++++++++++++++ 3 files changed, 67 insertions(+) diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 260a4ca06b..f4413029ec 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -1079,6 +1079,8 @@ ssids_options_to_ptrarray (GVariant *value, GError **error) gsize len; int num_ssids, i; + nm_assert (g_variant_is_of_type (value, G_VARIANT_TYPE ("aay"))); + num_ssids = g_variant_n_children (value); if (num_ssids > 32) { g_set_error_literal (error, @@ -1110,6 +1112,12 @@ ssids_options_to_ptrarray (GVariant *value, GError **error) return ssids; } +GPtrArray * +nmtst_ssids_options_to_ptrarray (GVariant *value, GError **error) +{ + return ssids_options_to_ptrarray (value, error); +} + static void dbus_request_scan_cb (NMDevice *device, GDBusMethodInvocation *context, diff --git a/src/devices/wifi/nm-device-wifi.h b/src/devices/wifi/nm-device-wifi.h index aaf47143b8..d861d24712 100644 --- a/src/devices/wifi/nm-device-wifi.h +++ b/src/devices/wifi/nm-device-wifi.h @@ -40,4 +40,6 @@ void _nm_device_wifi_request_scan (NMDeviceWifi *self, GVariant *options, GDBusMethodInvocation *invocation); +GPtrArray *nmtst_ssids_options_to_ptrarray (GVariant *value, GError **error); + #endif /* __NETWORKMANAGER_DEVICE_WIFI_H__ */ diff --git a/src/devices/wifi/tests/test-devices-wifi.c b/src/devices/wifi/tests/test-devices-wifi.c index a960e7a205..bde3630706 100644 --- a/src/devices/wifi/tests/test-devices-wifi.c +++ b/src/devices/wifi/tests/test-devices-wifi.c @@ -6,6 +6,7 @@ #include "nm-default.h" #include "devices/wifi/nm-wifi-utils.h" +#include "devices/wifi/nm-device-wifi.h" #include "nm-core-internal.h" #include "nm-test-utils-core.h" @@ -1337,6 +1338,60 @@ test_strength_all (void) /*****************************************************************************/ +static void +do_test_ssids_options_to_ptrarray (const char *const*ssids) +{ + GVariantBuilder builder; + gs_unref_variant GVariant *variant = NULL; + gs_unref_ptrarray GPtrArray *ssids_arr = NULL; + gs_free_error GError *error = NULL; + gsize len; + gsize i; + + g_assert (ssids); + + len = NM_PTRARRAY_LEN (ssids); + + g_variant_builder_init (&builder, G_VARIANT_TYPE ("aay")); + for (i = 0; i < len; i++) { + const char *ssid = ssids[i]; + + g_variant_builder_add (&builder, + "@ay", + g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, ssid, strlen (ssid), 1)); + } + variant = g_variant_builder_end (&builder); + + if (nmtst_get_rand_bool ()) + g_variant_ref_sink (variant); + + ssids_arr = nmtst_ssids_options_to_ptrarray (variant, &error); + g_assert (!error); + if (len == 0) { + g_assert (!ssids_arr); + return; + } + g_assert_cmpint (len, ==, ssids_arr->len); + for (i = 0; i < len; i++) { + const char *ssid = ssids[i]; + GBytes *bytes = ssids_arr->pdata[i]; + + g_assert (nm_utils_gbytes_equal_mem (bytes, + ssid, + strlen (ssid))); + } +} + +static void +test_ssids_options_to_ptrarray (void) +{ + do_test_ssids_options_to_ptrarray (NM_PTRARRAY_EMPTY (const char *)); + do_test_ssids_options_to_ptrarray (NM_MAKE_STRV ("ab")); + do_test_ssids_options_to_ptrarray (NM_MAKE_STRV ("ab", "cd", "fsdfdsf")); +} + +/*****************************************************************************/ + NMTST_DEFINE (); int @@ -1501,5 +1556,7 @@ main (int argc, char **argv) g_test_add_func ("/wifi/strength/all", test_strength_all); + g_test_add_func ("/wifi/ssids_options_to_ptrarray", test_ssids_options_to_ptrarray); + return g_test_run (); } From e6d256fe817293c82cfae60f9e7dc74db791a241 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 4 Jan 2020 11:44:47 +0100 Subject: [PATCH 4/4] device/wifi: cleanup ssids_options_to_ptrarray() - use proper gsize type to hold g_variant_n_children() - use cleanup attribute for GPtrArray - move variables inside nested scope where they are used --- src/devices/wifi/nm-device-wifi.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index f4413029ec..f334858cb4 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -1074,10 +1074,9 @@ _hw_addr_set_scanning (NMDeviceWifi *self, gboolean do_reset) static GPtrArray * ssids_options_to_ptrarray (GVariant *value, GError **error) { - GPtrArray *ssids = NULL; - const guint8 *bytes; - gsize len; - int num_ssids, i; + gs_unref_ptrarray GPtrArray *ssids = NULL; + gsize num_ssids; + gsize i; nm_assert (g_variant_is_of_type (value, G_VARIANT_TYPE ("aay"))); @@ -1094,6 +1093,8 @@ ssids_options_to_ptrarray (GVariant *value, GError **error) ssids = g_ptr_array_new_full (num_ssids, (GDestroyNotify) g_bytes_unref); for (i = 0; i < num_ssids; i++) { gs_unref_variant GVariant *v = NULL; + gsize len; + const guint8 *bytes; v = g_variant_get_child_value (value, i); bytes = g_variant_get_fixed_array (v, &len, sizeof (guint8)); @@ -1101,15 +1102,15 @@ ssids_options_to_ptrarray (GVariant *value, GError **error) g_set_error (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_NOT_ALLOWED, - "SSID at index %d more than 32 bytes", i); - g_ptr_array_unref (ssids); + "SSID at index %d more than 32 bytes", (int) i); return NULL; } g_ptr_array_add (ssids, g_bytes_new (bytes, len)); } } - return ssids; + + return g_steal_pointer (&ssids); } GPtrArray *