From aa9536a992c5a676c4b223c29866c9da7a63b53c Mon Sep 17 00:00:00 2001 From: Pekka Paalanen Date: Wed, 24 Jun 2015 16:09:17 +0300 Subject: [PATCH] text_backend: make destructor call explicit We used to rely on the order in which the weston_compositor::destroy_signal callbacks happened, to not access freed memory. Don't know when, but this broke at least with ivi-shell, which caused crashes in random places on compositor shutdown. Valgrind found the following: Invalid write of size 8 at 0xC2EDC69: unbind_input_panel (input-panel-ivi.c:340) by 0x4E3B6BB: destroy_resource (wayland-server.c:537) by 0x4E3E085: for_each_helper.isra.0 (wayland-util.c:359) by 0x4E3E60D: wl_map_for_each (wayland-util.c:365) by 0x4E3BEC7: wl_client_destroy (wayland-server.c:675) by 0x4182F2: text_backend_notifier_destroy (text-backend.c:1047) by 0x4084FB: wl_signal_emit (wayland-server-core.h:264) by 0x4084FB: main (compositor.c:5465) Address 0x67ea360 is 208 bytes inside a block of size 232 free'd at 0x4C2A6BC: free (vg_replace_malloc.c:473) by 0x4084FB: wl_signal_emit (wayland-server-core.h:264) by 0x4084FB: main (compositor.c:5465) Invalid write of size 8 at 0x4E3E0D7: wl_list_remove (wayland-util.c:57) by 0xC2EDEE9: destroy_input_panel_surface (input-panel-ivi.c:191) by 0x4E3B6BB: destroy_resource (wayland-server.c:537) by 0x4E3BC7B: wl_resource_destroy (wayland-server.c:550) by 0x40DB8B: wl_signal_emit (wayland-server-core.h:264) by 0x40DB8B: weston_surface_destroy (compositor.c:1883) by 0x40DB8B: weston_surface_destroy (compositor.c:1873) by 0x4E3B6BB: destroy_resource (wayland-server.c:537) by 0x4E3E085: for_each_helper.isra.0 (wayland-util.c:359) by 0x4E3E60D: wl_map_for_each (wayland-util.c:365) by 0x4E3BEC7: wl_client_destroy (wayland-server.c:675) by 0x4182F2: text_backend_notifier_destroy (text-backend.c:1047) by 0x4084FB: wl_signal_emit (wayland-server-core.h:264) by 0x4084FB: main (compositor.c:5465) Address 0x67ea370 is 224 bytes inside a block of size 232 free'd at 0x4C2A6BC: free (vg_replace_malloc.c:473) by 0x4084FB: wl_signal_emit (wayland-server-core.h:264) by 0x4084FB: main (compositor.c:5465) Invalid write of size 8 at 0x4E3E0E7: wl_list_remove (wayland-util.c:58) by 0xC2EDEE9: destroy_input_panel_surface (input-panel-ivi.c:191) by 0x4E3B6BB: destroy_resource (wayland-server.c:537) by 0x4E3BC7B: wl_resource_destroy (wayland-server.c:550) by 0x40DB8B: wl_signal_emit (wayland-server-core.h:264) by 0x40DB8B: weston_surface_destroy (compositor.c:1883) by 0x40DB8B: weston_surface_destroy (compositor.c:1873) by 0x4E3B6BB: destroy_resource (wayland-server.c:537) by 0x4E3E085: for_each_helper.isra.0 (wayland-util.c:359) by 0x4E3E60D: wl_map_for_each (wayland-util.c:365) by 0x4E3BEC7: wl_client_destroy (wayland-server.c:675) by 0x4182F2: text_backend_notifier_destroy (text-backend.c:1047) by 0x4084FB: wl_signal_emit (wayland-server-core.h:264) by 0x4084FB: main (compositor.c:5465) Address 0x67ea368 is 216 bytes inside a block of size 232 free'd at 0x4C2A6BC: free (vg_replace_malloc.c:473) by 0x4084FB: wl_signal_emit (wayland-server-core.h:264) by 0x4084FB: main (compositor.c:5465) Looking at the first of these, unbind_input_panel() gets called when the text-backend destroys its helper client which has bound to input_panel interface. This happens after the shell's destroy_signal callback has been called, so the shell has already been freed. The other two errors come from wl_list_remove(&input_panel_surface->link); which has gone stale when the shell was destroyed (shell->input_panel.surfaces list). Rather than creating even more destroy listeners and hooking them up in spaghetti, modify text-backend to not hook up to the compositor destroy signal. Instead, make it the text_backend_init() callers' responsibility to also call text_backend_destroy() appropriately, before the shell goes away. This fixed all the above Valgrind errors, and avoid a crash with ivi-shell when exiting Weston. Also using desktop-shell exhibited similar Valgrind errors which are fixed by this patch, but those didn't happen to cause any crashes AFAIK. Signed-off-by: Pekka Paalanen Reviewed-By: Derek Foreman --- desktop-shell/shell.c | 4 +++- desktop-shell/shell.h | 2 ++ ivi-shell/ivi-shell.c | 4 +++- ivi-shell/ivi-shell.h | 2 ++ src/compositor.h | 7 ++++++- src/text-backend.c | 18 +++++------------- 6 files changed, 21 insertions(+), 16 deletions(-) diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c index 0fe46582..ed7e8961 100644 --- a/desktop-shell/shell.c +++ b/desktop-shell/shell.c @@ -6347,6 +6347,7 @@ shell_destroy(struct wl_listener *listener, void *data) wl_list_remove(&shell->idle_listener.link); wl_list_remove(&shell->wake_listener.link); + text_backend_destroy(shell->text_backend); input_panel_destroy(shell); wl_list_for_each_safe(shell_output, tmp, &shell->output_list, link) { @@ -6513,7 +6514,8 @@ module_init(struct weston_compositor *ec, if (input_panel_setup(shell) < 0) return -1; - if (text_backend_init(ec) < 0) + shell->text_backend = text_backend_init(ec); + if (!shell->text_backend) return -1; shell_configuration(shell); diff --git a/desktop-shell/shell.h b/desktop-shell/shell.h index f76f1810..c0585a22 100644 --- a/desktop-shell/shell.h +++ b/desktop-shell/shell.h @@ -148,6 +148,8 @@ struct desktop_shell { bool showing_input_panels; bool prepare_event_sent; + struct text_backend *text_backend; + struct { struct weston_surface *surface; pixman_box32_t cursor_rectangle; diff --git a/ivi-shell/ivi-shell.c b/ivi-shell/ivi-shell.c index 896ba1b4..f958fd46 100644 --- a/ivi-shell/ivi-shell.c +++ b/ivi-shell/ivi-shell.c @@ -343,6 +343,7 @@ shell_destroy(struct wl_listener *listener, void *data) container_of(listener, struct ivi_shell, destroy_listener); struct ivi_shell_surface *ivisurf, *next; + text_backend_destroy(shell->text_backend); input_panel_destroy(shell); wl_list_for_each_safe(ivisurf, next, &shell->ivi_surface_list, link) { @@ -439,7 +440,8 @@ module_init(struct weston_compositor *compositor, if (input_panel_setup(shell) < 0) goto out_settings; - if (text_backend_init(compositor) < 0) + shell->text_backend = text_backend_init(compositor); + if (!shell->text_backend) goto out_settings; if (wl_global_create(compositor->wl_display, diff --git a/ivi-shell/ivi-shell.h b/ivi-shell/ivi-shell.h index bd2571eb..9a05eb2a 100644 --- a/ivi-shell/ivi-shell.h +++ b/ivi-shell/ivi-shell.h @@ -35,6 +35,8 @@ struct ivi_shell struct wl_list ivi_surface_list; /* struct ivi_shell_surface::link */ + struct text_backend *text_backend; + struct wl_listener show_input_panel_listener; struct wl_listener hide_input_panel_listener; struct wl_listener update_input_panel_listener; diff --git a/src/compositor.h b/src/compositor.h index 3586f5bf..9069fe1c 100644 --- a/src/compositor.h +++ b/src/compositor.h @@ -1456,9 +1456,14 @@ weston_screenshooter_shoot(struct weston_output *output, struct weston_buffer *b struct clipboard * clipboard_create(struct weston_seat *seat); -int +struct text_backend; + +struct text_backend * text_backend_init(struct weston_compositor *ec); +void +text_backend_destroy(struct text_backend *text_backend); + struct weston_process; typedef void (*weston_process_cleanup_func_t)(struct weston_process *process, int status); diff --git a/src/text-backend.c b/src/text-backend.c index 55013a26..9485f7ec 100644 --- a/src/text-backend.c +++ b/src/text-backend.c @@ -109,7 +109,6 @@ struct text_backend { } input_method; struct wl_listener seat_created_listener; - struct wl_listener destroy_listener; }; static void @@ -1037,21 +1036,17 @@ text_backend_configuration(struct text_backend *text_backend) free(client); } -static void -text_backend_notifier_destroy(struct wl_listener *listener, void *data) +WL_EXPORT void +text_backend_destroy(struct text_backend *text_backend) { - struct text_backend *text_backend = - container_of(listener, struct text_backend, destroy_listener); - if (text_backend->input_method.client) wl_client_destroy(text_backend->input_method.client); free(text_backend->input_method.path); - free(text_backend); } -WL_EXPORT int +WL_EXPORT struct text_backend * text_backend_init(struct weston_compositor *ec) { struct text_backend *text_backend; @@ -1059,7 +1054,7 @@ text_backend_init(struct weston_compositor *ec) text_backend = zalloc(sizeof(*text_backend)); if (text_backend == NULL) - return -1; + return NULL; text_backend->compositor = ec; @@ -1071,10 +1066,7 @@ text_backend_init(struct weston_compositor *ec) wl_signal_add(&ec->seat_created_signal, &text_backend->seat_created_listener); - text_backend->destroy_listener.notify = text_backend_notifier_destroy; - wl_signal_add(&ec->destroy_signal, &text_backend->destroy_listener); - text_input_manager_create(ec); - return 0; + return text_backend; }