On 21/06/15 02:25 PM, 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.
Tricky. :) I like the solution, looks good to me... However there are some > 80 column lines that should probably be wrapped. That done, Reviewed-By: Derek Foreman <[email protected]> > > Signed-off-by: Mario Kleiner <[email protected]> > Cc: Richard Hughes <[email protected]> > --- > src/cms-colord.c | 53 +++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 33 insertions(+), 20 deletions(-) > > diff --git a/src/cms-colord.c b/src/cms-colord.c > index 2adc886..b0dbb7b 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,32 @@ 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 > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
