From 6f85d3e0b9bd80a74f4c20888103544f95be12a4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 28 Mar 2018 12:31:43 +0200 Subject: [PATCH] libnm: fix crash creating checkpoint during find_checkpoint_info() Now that the D-Bus signals in server are reordered, creating a checkpoint in libnm crashes: $ examples/python/gi/checkpoint.py create 4 #0 0x00007ffff6d011ee in __strcmp_sse2_unaligned () at /lib64/libc.so.6 #1 0x00007fffeb611c90 in find_checkpoint_info (manager=manager@entry=0x5555559e5110 [NMManager], path=0x7fffdc0092f0 "/org/freedesktop/NetworkManager/Checkpoint/6") at libnm/nm-manager.c:153 #2 0x00007fffeb611d8f in checkpoint_added (manager=0x5555559e5110 [NMManager], checkpoint=checkpoint@entry=0x555555a122d0 [NMCheckpoint]) at libnm/nm-manager.c:1194 #3 0x00007fffef7db929 in g_cclosure_marshal_VOID__OBJECTv (closure=0x5555559e4b30, return_value=, instance=, args=, marshal_data=, n_params=, param_types=0x5555559e2fc0) at gmarshal.c:2102 #4 0x00007fffef7d8976 in _g_closure_invoke_va (closure=0x5555559e4b30, return_value=0x0, instance=0x5555559e5110, args=0x7fffffffc1c8, n_params=1, param_types=0x5555559e2fc0) at gclosure.c:867 #5 0x00007fffef7f3ff4 in g_signal_emit_valist (instance=instance@entry=0x5555559e5110, signal_id=signal_id@entry=97, detail=0, var_args=var_args@entry=0x7fffffffc1c8) at gsignal.c:3300 #6 0x00007fffef7f4b48 in g_signal_emit_by_name (instance=instance@entry=0x5555559e5110, detailed_signal=detailed_signal@entry=0x7fffffffc310 "checkpoint-added") at gsignal.c:3487 #7 0x00007fffeb6156d1 in deferred_notify_cb (data=0x5555559e5110) at libnm/nm-object.c:219 #8 0x00007fffeb615ae7 in object_property_maybe_complete (self=0x5555559e5110 [NMManager]) at libnm/nm-object.c:555 #9 0x00007fffeb615e5d in object_created (obj=, path=, user_data=) at libnm/nm-object.c:576 #10 0x00007fffeb61648b in handle_object_array_property (pi=, value=0x7fffdc075070, property_name=0x7fffeb67f117 "checkpoints", self=0x5555559e5110 [NMManager]) at libnm/nm-object.c:671 #11 0x00007fffeb61648b in handle_property_changed (self=self@entry=0x5555559e5110 [NMManager], dbus_name=, value=) at libnm/nm-object.c:740 #12 0x00007fffeb6166e9 in properties_changed (proxy=, changed_properties=, invalidated_properties=, user_data=0x5555559e5110) at libnm/nm-object.c:772 ... That is, because NetworkManager now first emits signals that the checkpoint object was created, before answering the D-Bus request. That makes more sense, but leads to this crash. The ugliness of how libnm handles object visibility is considerable. libnm hides objects until they are fully initialized. So, when the async create-checkpoint operation returns, the object might not yet be ready to be exposed. We need to delay the result. It would be better if the API would simply return the created path. --- libnm/nm-manager.c | 40 ++++++++++++++++++++++++++++++---------- 1 file changed, 30 insertions(+), 10 deletions(-) diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c index 7c65f3906..b8a0b9c04 100644 --- a/libnm/nm-manager.c +++ b/libnm/nm-manager.c @@ -150,7 +150,7 @@ find_checkpoint_info (NMManager *manager, const char *path) for (iter = priv->added_checkpoints; iter; iter = g_slist_next (iter)) { info = iter->data; - if (nm_streq (path, info->path)) + if (nm_streq0 (path, info->path)) return info; } @@ -802,7 +802,21 @@ nm_manager_get_device_by_path (NMManager *manager, const char *object_path) return candidate; } } + return NULL; +} +static NMCheckpoint * +get_checkpoint_by_path (NMManager *manager, const char *object_path) +{ + NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (manager); + NMCheckpoint *candidate; + guint i; + + for (i = 0; i < priv->checkpoints->len; i++) { + candidate = priv->checkpoints->pdata[i]; + if (nm_streq (nm_object_get_path (NM_OBJECT (candidate)), object_path)) + return candidate; + } return NULL; } @@ -1194,11 +1208,6 @@ checkpoint_added (NMManager *manager, NMCheckpoint *checkpoint) checkpoint_info_complete (manager, info, checkpoint, NULL); } -static void -checkpoint_removed (NMManager *manager, NMCheckpoint *checkpoint) -{ -} - gboolean nm_manager_deactivate_connection (NMManager *manager, NMActiveConnection *active, @@ -1329,15 +1338,27 @@ checkpoint_created_cb (GObject *object, gpointer user_data) { CheckpointInfo *info = user_data; - GError *error = NULL; + NMManager *self = info->manager; + gs_free_error GError *error = NULL; + NMCheckpoint *checkpoint; nmdbus_manager_call_checkpoint_create_finish (NMDBUS_MANAGER (object), &info->path, result, &error); if (error) { g_dbus_error_strip_remote_error (error); - checkpoint_info_complete (info->manager, info, NULL, error); - g_clear_error (&error); + checkpoint_info_complete (self, info, NULL, error); + return; } + + checkpoint = get_checkpoint_by_path (self, info->path); + if (!checkpoint) { + /* this is really problematic. The async request returned, but + * we don't yet have a visible (fully initialized) NMCheckpoint instance + * to return. Wait longer for it to appear. However, it's ugly. */ + return; + } + + checkpoint_info_complete (self, info, checkpoint, NULL); } void @@ -1830,7 +1851,6 @@ nm_manager_class_init (NMManagerClass *manager_class) manager_class->active_connection_added = active_connection_added; manager_class->active_connection_removed = active_connection_removed; manager_class->checkpoint_added = checkpoint_added; - manager_class->checkpoint_removed = checkpoint_removed; /* properties */