From eda2c5ac483844c802144478714324ee67789c2c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Jan 2022 17:39:39 +0100 Subject: [PATCH 1/5] tools: support --no-make-first option in "run-nm-test.sh" Why? Because I often use a command line like $ ./tools/run-nm-test.sh -m src/libnm-client-impl/tests/test-nm-client -p /libnm/device-connection-compatibility or even alias it to a one character command `x`. Usually I want to do the rebuild, and as `make` is so slow, it adds noticeable time running the command. Thus, sometimes I want to modify the command, for which I have to edit the command from the history, or toggle two separate commands. Add a `-M` flag that can reverse the effect of an earlier `-m`. An "enable" flag in general should just also have a "disable" flag. --- tools/run-nm-test.sh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tools/run-nm-test.sh b/tools/run-nm-test.sh index 230a7a66e1..b1d3dd939a 100755 --- a/tools/run-nm-test.sh +++ b/tools/run-nm-test.sh @@ -37,6 +37,7 @@ usage() { echo " --no-libtool: when running with valgrind, the script tries automatically to" echo " use libtool as necessary. This disables libtool usage" echo " --make-first|-m: before running the test, make it (only works with autotools build)" + echo " --no-make-first|-M: disable --make-first option" echo " --valgrind|-v: run under valgrind" echo " --no-valgrind|-V: disable running under valgrind (overrides NMTST_USE_VALGRIND=1)" echo " -d: set NMTST_DEBUG=d" @@ -165,6 +166,10 @@ else NMTST_MAKE_FIRST=1 shift ;; + --no-make-first|-M) + NMTST_MAKE_FIRST=0 + shift + ;; "--valgrind"|-v) NMTST_USE_VALGRIND=1 shift; From ee3e2e0bb688c27ee11f702a2457a0ce285961e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Jan 2022 17:48:45 +0100 Subject: [PATCH 2/5] glib-aux/tests: add nmtst_test_skip_slow() helper --- src/libnm-glib-aux/nm-test-utils.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/libnm-glib-aux/nm-test-utils.h b/src/libnm-glib-aux/nm-test-utils.h index 6cf5a777d7..caf43d5f48 100644 --- a/src/libnm-glib-aux/nm-test-utils.h +++ b/src/libnm-glib-aux/nm-test-utils.h @@ -709,6 +709,17 @@ nmtst_test_quick(void) return __nmtst_internal.test_quick; } +static inline gboolean +nmtst_test_skip_slow(void) +{ + if (!nmtst_test_quick()) + return FALSE; + + g_print("Skipping test: don't run long running test %s (NMTST_DEBUG=slow)\n", g_get_prgname()); + g_test_skip("Skip long running test"); + return TRUE; +} + #if GLIB_CHECK_VERSION(2, 34, 0) #undef g_test_expect_message #define g_test_expect_message(...) \ From 1f9de38a7b9cb033f3715eb115501fe6a3317946 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Jan 2022 18:09:28 +0100 Subject: [PATCH 3/5] tools/tests: set available-connections for vlan device in NM test stub --- tools/test-networkmanager-service.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/test-networkmanager-service.py b/tools/test-networkmanager-service.py index f990a0e48d..a7788510a5 100755 --- a/tools/test-networkmanager-service.py +++ b/tools/test-networkmanager-service.py @@ -1077,6 +1077,9 @@ class Device(ExportedObj): elif isinstance(self, WifiDevice): if con_inst.get_type() == NM.SETTING_WIRELESS_SETTING_NAME: return True + elif isinstance(self, VlanDevice): + if con_inst.get_type() == NM.SETTING_VLAN_SETTING_NAME: + return True return False def available_connections_get(self): @@ -1630,6 +1633,9 @@ class NetworkManager(ExportedObj): ac = ActiveConnection(device, con_inst, None) self.active_connection_add(ac) + + gl.manager.devices_available_connections_update() + return ExportedObj.to_path(ac) def active_connection_add(self, ac): From bc9aa72c885386d8f0942810e010d6ba6a03a4cd Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Jan 2022 17:54:58 +0100 Subject: [PATCH 4/5] libnm/tests: add unit test for checking dangling pointer in libnm When destroying NMClient, nm_device_get_active_connection() still return dangling pointers. Add a unit test for that bug. Obviously, the bug currently exists, so the relevant code is commented out. --- src/libnm-client-impl/tests/test-nm-client.c | 80 ++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/libnm-client-impl/tests/test-nm-client.c b/src/libnm-client-impl/tests/test-nm-client.c index 62b4c883ba..ff821d6be4 100644 --- a/src/libnm-client-impl/tests/test-nm-client.c +++ b/src/libnm-client-impl/tests/test-nm-client.c @@ -788,6 +788,32 @@ activate_cb(GObject *object, GAsyncResult *result, gpointer user_data) g_main_loop_quit(info->loop); } +static void +_dev_eth0_1_state_changed_cb(NMDevice *device, + NMDeviceState new_state, + NMDeviceState old_state, + NMDeviceStateReason reason, + int *p_count_call) +{ + const GPtrArray *arr; + + g_assert(p_count_call); + g_assert_cmpint(*p_count_call, ==, 0); + + (*p_count_call)++; + + g_assert(NM_IS_DEVICE_VLAN(device)); + + g_assert_cmpint(old_state, ==, NM_DEVICE_STATE_PREPARE); + g_assert_cmpint(new_state, ==, NM_DEVICE_STATE_UNKNOWN); + + arr = nm_device_get_available_connections(device); + g_assert(arr); + g_assert_cmpint(arr->len, ==, 0); + + // g_assert(!nm_device_get_active_connection(device)); +} + static void test_activate_virtual(void) { @@ -799,6 +825,9 @@ test_activate_virtual(void) TestACInfo info = {gl.loop, NULL, 0}; TestConnectionInfo conn_info = {gl.loop, NULL}; + if (nmtst_test_skip_slow()) + return; + sinfo = nmtstc_service_init(); if (!nmtstc_service_available(sinfo)) return; @@ -847,6 +876,57 @@ test_activate_virtual(void) g_object_remove_weak_pointer(G_OBJECT(info.device), (gpointer *) &info.device); nm_clear_g_signal_handler(info.device, &info.ac_signal_id); } + + if (nmtst_get_rand_bool()) { + /* OK, enough for this run. Let's see whether we can tear down + * successfully at this point. */ + return; + } + + { + NMDevice *dev_eth0_1; + NMActiveConnection *ac; + const GPtrArray *arr; + gulong sig_id; + int call_count = 0; + gboolean take_ref = nmtst_get_rand_bool(); + + /* ensure we got all the necessary events in place. */ + nmtst_main_loop_run(gl.loop, 50); + + dev_eth0_1 = nm_client_get_device_by_iface(client, "eth0.1"); + g_assert(NM_IS_DEVICE_VLAN(dev_eth0_1)); + if (take_ref) + g_object_ref(dev_eth0_1); + + arr = nm_device_get_available_connections(dev_eth0_1); + g_assert(arr); + g_assert_cmpint(arr->len, ==, 1); + + ac = nm_device_get_active_connection(dev_eth0_1); + g_assert(NM_IS_ACTIVE_CONNECTION(ac)); + + sig_id = g_signal_connect(dev_eth0_1, + "state-changed", + G_CALLBACK(_dev_eth0_1_state_changed_cb), + &call_count); + + g_clear_object(&client); + + g_assert_cmpint(call_count, ==, 1); + + if (take_ref) { + arr = nm_device_get_available_connections(dev_eth0_1); + g_assert(arr); + g_assert_cmpint(arr->len, ==, 0); + + // g_assert(!nm_device_get_active_connection(dev_eth0_1)); + + nm_clear_g_signal_handler(dev_eth0_1, &sig_id); + + g_object_unref(dev_eth0_1); + } + } } static void From 2afecaf908401296a2642fdd1d73a76b9c3fa11a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 08:41:43 +0100 Subject: [PATCH 5/5] libnm: fix dangling pointer in public API while destructing NMClient While (and after) NMClient gets destroyed, nm_device_get_active_connection() gives a dangling pointer. That can lead to a crash. This probably affects all NMLDBusPropertyO type properties. It's not clear how to fix that best. Usually, NMClient does updates in two phases, first it processes the D-Bus events and tracks internal data, then it emits all GObject signals and notifications. When an object gets removed from the NMClient cache, then the second phase is not fully processed, because the object is already removed from the cache. Thus, the property was not properly cleared leaving a dangling pointer. A simple fix is to always clear the pointer during the first phase. Note that effectively we do the same also for NMLDBusPropertyAO (by clearing the "pr_ao->arr"), so at least this is consistent. Somehow it seems that we should make sure that the "second" phase gets full processed in this case too. But it's complicated, and it's not clear how to do that. So this solution seems fine. https://bugzilla.redhat.com/show_bug.cgi?id=2039331 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/896 --- src/libnm-client-impl/nm-client.c | 1 + src/libnm-client-impl/tests/test-nm-client.c | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libnm-client-impl/nm-client.c b/src/libnm-client-impl/nm-client.c index 27382ec8b9..a2797f905a 100644 --- a/src/libnm-client-impl/nm-client.c +++ b/src/libnm-client-impl/nm-client.c @@ -1799,6 +1799,7 @@ nml_dbus_property_o_notify(NMClient *self, if (pr_o->obj_watcher && (!dbus_path || !nm_streq(dbus_path, pr_o->obj_watcher->dbobj->dbus_path->str))) { + pr_o->nmobj = NULL; _dbobjs_obj_watcher_unregister(self, g_steal_pointer(&pr_o->obj_watcher)); changed = TRUE; } diff --git a/src/libnm-client-impl/tests/test-nm-client.c b/src/libnm-client-impl/tests/test-nm-client.c index ff821d6be4..7e67d2a004 100644 --- a/src/libnm-client-impl/tests/test-nm-client.c +++ b/src/libnm-client-impl/tests/test-nm-client.c @@ -811,7 +811,7 @@ _dev_eth0_1_state_changed_cb(NMDevice *device, g_assert(arr); g_assert_cmpint(arr->len, ==, 0); - // g_assert(!nm_device_get_active_connection(device)); + g_assert(!nm_device_get_active_connection(device)); } static void @@ -920,7 +920,7 @@ test_activate_virtual(void) g_assert(arr); g_assert_cmpint(arr->len, ==, 0); - // g_assert(!nm_device_get_active_connection(dev_eth0_1)); + g_assert(!nm_device_get_active_connection(dev_eth0_1)); nm_clear_g_signal_handler(dev_eth0_1, &sig_id);