[PATCH] cms-colord: Fix crash at compositor shutdown. (v2)

[Mario Kleiner]

cms-colord used the weston_compositor destroy signal to
trigger its final colord_module_destroy cleanup, and the
wl_output destroy signal to trigger per output cleanup.

The problem is that the compositor destroy signal gets
emitted before the output destroy signals at compositor
shutdown, colord_module_destroy would free all its
shared data structures and then later on the output
destroy callback would try to access those shared
data structures when handling output destruction
-> Use after free -> Crash, usually with VT switching
dead and thereby an unuseable system requiring a reboot.

Solve this by moving the output destruction handling into
the colord_cms_output_destroy() cleanup function for
colord-cms own hash dictionary of all active outputs.

The output destroy callback just removes the corresponding
output from the dictionary and triggers proper cleanup if
an output is unplugged during runtime. During compositor
shutdown, the dictionary as a whole is released before
releasing all other shared data structures, thereby
triggering cleanup of all remaining outputs.

Tested to fix crashes on x11 and drm backends.

v2: Formatting: Wrap lines to < 80 characters, as suggested
    by Derek. Thanks.

Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
---
 src/cms-colord.c | 54 ++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/src/cms-colord.c b/src/cms-colord.c
index 2adc886..1e61feb 100644
--- a/src/cms-colord.c
+++ b/src/cms-colord.c
@@ -220,26 +220,9 @@ colord_notifier_output_destroy(struct wl_listener *listener, void *data)
 		container_of(listener, struct cms_output, destroy_listener);
 	struct weston_output *o = (struct weston_output *) data;
 	struct cms_colord *cms = ocms->cms;
-
-	gboolean ret;
 	gchar *device_id;
-	GError *error = NULL;
 
-	colord_idle_cancel_for_output(cms, o);
 	device_id = get_output_id(cms, o);
-	weston_log("colord: output removed %s\n", device_id);
-	g_signal_handlers_disconnect_by_data(ocms->device, ocms);
-	ret = cd_client_delete_device_sync (cms->client,
-					    ocms->device,
-					    NULL,
-					    &error);
-
-	if (!ret) {
-		weston_log("colord: failed to delete device: %s\n", error->message);
-		g_error_free(error);
-		goto out;
-	}
-out:
 	g_hash_table_remove (cms->devices, device_id);
 	g_free (device_id);
 }
@@ -445,15 +428,17 @@ colord_load_pnp_ids(struct cms_colord *cms)
 static void
 colord_module_destroy(struct cms_colord *cms)
 {
-	g_free(cms->pnp_ids_data);
-	g_hash_table_unref(cms->pnp_ids);
-
 	if (cms->loop) {
 		g_main_loop_quit(cms->loop);
 		g_main_loop_unref(cms->loop);
 	}
 	if (cms->thread)
 		g_thread_join(cms->thread);
+
+	/* cms->devices must be destroyed before other resources, as
+	 * the other resources are needed during output cleanup in
+	 * cms->devices unref.
+	 */
 	if (cms->devices)
 		g_hash_table_unref(cms->devices);
 	if (cms->client)
@@ -462,6 +447,10 @@ colord_module_destroy(struct cms_colord *cms)
 		close(cms->readfd);
 	if (cms->writefd)
 		close(cms->writefd);
+
+	g_free(cms->pnp_ids_data);
+	g_hash_table_unref(cms->pnp_ids);
+
 	free(cms);
 }
 
@@ -477,8 +466,33 @@ static void
 colord_cms_output_destroy(gpointer data)
 {
 	struct cms_output *ocms = (struct cms_output *) data;
+	struct cms_colord *cms = ocms->cms;
+	struct weston_output *o = ocms->o;
+	gboolean ret;
+	gchar *device_id;
+	GError *error = NULL;
+
+	colord_idle_cancel_for_output(cms, o);
+	device_id = get_output_id(cms, o);
+	weston_log("colord: output unplugged %s\n", device_id);
+
+	wl_list_remove(&ocms->destroy_listener.link);
+	g_signal_handlers_disconnect_by_data(ocms->device, ocms);
+
+	ret = cd_client_delete_device_sync (cms->client,
+					    ocms->device,
+					    NULL,
+					    &error);
+
+	if (!ret) {
+		weston_log("colord: failed to delete device: %s\n",
+			   error->message);
+		g_error_free(error);
+	}
+
 	g_object_unref(ocms->device);
 	g_slice_free(struct cms_output, ocms);
+	g_free (device_id);
 }
 
 WL_EXPORT int
-- 
1.9.1