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=<optimized out>, instance=<optimized out>, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, 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=<optimized out>, path=<optimized out>, user_data=<optimized out>) at libnm/nm-object.c:576
    #10 0x00007fffeb61648b in handle_object_array_property (pi=<optimized out>, 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=<optimized out>, value=<optimized out>) at libnm/nm-object.c:740
    #12 0x00007fffeb6166e9 in properties_changed (proxy=<optimized out>, changed_properties=<optimized out>, invalidated_properties=<optimized out>, 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.

1 files changed, 30 insertions, 10 deletions

diff --git a/libnm/nm-manager.c b/libnm/nm-manager.c
index 7c65f3906b..b8a0b9c046 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 */