From 2afecaf908401296a2642fdd1d73a76b9c3fa11a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jan 2022 08:41:43 +0100 Subject: [PATCH] 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 27382ec8b..a2797f905 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 ff821d6be..7e67d2a00 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);