From 8eef82209343a96c92af027843c4ebfa816217e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Jan 2022 08:24:36 +0100 Subject: [PATCH 1/3] libnm: fix dangling pointer in "o" properties when unregistering NMObject When NMClient gets destroyed, it unrefs all NMObject. We need to unbreak cycles then, and the property getters must return NULL. In particular, for "o" type properties (NMLDBusPropertyO), this was not done correctly. For example, calling nm_device_get_active_connection() while/after destroying the NMClient can give a dangling pointer and assertion failure. This will also be covered by test_activate_virtual(). Probably a similar issue can happen, when a D-Bus object gets removed (without destroying NMClient altogether). The fix is that nml_dbus_property_o_clear() needs to clear "nmobj". That is correct, because the pointer is no longer valid and should not be there. And the unit test shows that in fact a pointer is left there, and clearing it fixes it. That was different from an earlier attempt to fix this (in commit 62b2aa85e875 ('Revert "libnm: fix dangling pointer in public API while destructing NMClient"')), where clearing the pointer at a different place broke things. That attempt was wrong, because nml_dbus_property_o_notify_changed() needs to be the one that sets/clears nmobj field during a regular update. But the case here is not a regular update, nml_dbus_property_o_clear() happens during unregister/cleanup, and then we need to clear the pointer. Fixes: ce0e898fb476 ('libnm: refactor caching of D-Bus objects in NMClient') https://bugzilla.redhat.com/show_bug.cgi?id=2039331 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/896 See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1064 https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1075 --- src/libnm-client-impl/nm-client.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libnm-client-impl/nm-client.c b/src/libnm-client-impl/nm-client.c index 27382ec8b9..d1db062d76 100644 --- a/src/libnm-client-impl/nm-client.c +++ b/src/libnm-client-impl/nm-client.c @@ -1837,6 +1837,7 @@ nml_dbus_property_o_clear(NMLDBusPropertyO *pr_o, NMClient *self) pr_o->meta_iface = NULL; pr_o->dbus_property_idx = 0; pr_o->is_ready = FALSE; + pr_o->nmobj = NULL; } void From 125b1f9a9a934a578af01ffd98eaf68e92c5cb5a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Jan 2022 21:41:43 +0100 Subject: [PATCH 2/3] libnm/tests: enable check for dangling pointer in test_activate_virtual() --- src/libnm-client-impl/tests/test-nm-client.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libnm-client-impl/tests/test-nm-client.c b/src/libnm-client-impl/tests/test-nm-client.c index 4841c96865..a43e98cd18 100644 --- a/src/libnm-client-impl/tests/test-nm-client.c +++ b/src/libnm-client-impl/tests/test-nm-client.c @@ -812,7 +812,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 @@ -921,7 +921,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); From 039f0b280381aff4af1eaeb56f67854cd6212acc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 27 Jan 2022 22:09:26 +0100 Subject: [PATCH 3/3] libnm: add code comments to NMClient --- src/libnm-client-impl/nm-client.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/libnm-client-impl/nm-client.c b/src/libnm-client-impl/nm-client.c index d1db062d76..d27e4d5bc2 100644 --- a/src/libnm-client-impl/nm-client.c +++ b/src/libnm-client-impl/nm-client.c @@ -1335,6 +1335,8 @@ nml_dbus_object_obj_changed_link(NMClient *self, nm_assert(NML_IS_DBUS_OBJECT(dbobj)); nm_assert(changed_type != NML_DBUS_OBJ_CHANGED_TYPE_NONE); + /* Links @dbobj in the "obj_changed_lst", with the new "changed_type". */ + if (!NM_FLAGS_ALL((NMLDBusObjChangedType) dbobj->obj_changed_type, changed_type)) NML_NMCLIENT_LOG_T(self, "[%s]: changed-type 0x%02x linked", @@ -1372,6 +1374,12 @@ nml_dbus_object_obj_changed_consume(NMClient *self, NMClientPrivate *priv; NMLDBusObjChangedType changed_type_res; + /* We have @dbobj which has some "obj_changed_type" set (consequently, + * it's linked in the "obj_changed_lst"). Here we consume the @changed_type, + * meaning, to clear those flags from "obj_change_type" (and return + * the flags that were cleared/present or NONE, if the current object + * doesn't have these changed-types. */ + nm_assert(NM_IS_CLIENT(self)); nm_assert(NML_IS_DBUS_OBJECT(dbobj)); nm_assert(changed_type != NML_DBUS_OBJ_CHANGED_TYPE_NONE); @@ -1383,6 +1391,8 @@ nml_dbus_object_obj_changed_consume(NMClient *self, dbobj->obj_changed_type &= ~changed_type; if (dbobj->obj_changed_type == NML_DBUS_OBJ_CHANGED_TYPE_NONE) { + /* No other "obj_change_type" left. Unlink the object from the + * "changed_type_list". */ c_list_unlink(&dbobj->obj_changed_lst); nm_assert(changed_type_res != NML_DBUS_OBJ_CHANGED_TYPE_NONE); NML_NMCLIENT_LOG_T(self, @@ -1394,6 +1404,9 @@ nml_dbus_object_obj_changed_consume(NMClient *self, priv = NM_CLIENT_GET_PRIVATE(self); + /* Actually, at this point, @dbobj is not linked in priv->obj_changed_lst_head, + * instead, it's linked on a temporary list. As we still have changes left after + * consuming "changed_type", we move it to priv->obj_changed_lst_head. */ nm_assert(!c_list_contains(&priv->obj_changed_lst_head, &dbobj->obj_changed_lst)); nm_c_list_move_tail(&priv->obj_changed_lst_head, &dbobj->obj_changed_lst); NML_NMCLIENT_LOG_T(self,