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

[Bryce Harrington]

On Sat, Jul 18, 2015 at 08:07:11AM +0200, Mario Kleiner wrote:
> 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>

Thanks, pushed:
To ssh://git.freedesktop.org/git/wayland/weston
   004b4a1..2611ebd  master -> master


> ---
>  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
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel