From ab42159bf307b0794156676e8451b0ffc89039b4 Mon Sep 17 00:00:00 2001 From: Marius Vlad Date: Fri, 13 May 2022 20:11:46 +0300 Subject: [PATCH] desktop-shell: Add missing weston_view_destroy() This fixes the following weston_view leak at compositor shutdown: #0 0x7f4250247987 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:154 #1 0x7f424fd37aa3 in zalloc ../include/libweston/zalloc.h:38 #2 0x7f424fd3a05f in weston_view_create ../libweston/compositor.c:386 #3 0x7f423be7be6a in weston_desktop_surface_create_desktop_view ../libweston-desktop/surface.c:364 #4 0x7f423be7c0a8 in weston_desktop_surface_create_view ../libweston-desktop/surface.c:404 #5 0x7f423beae91d in desktop_surface_added ../desktop-shell/shell.c:2273 #6 0x7f423be77db1 in weston_desktop_api_surface_added ../libweston-desktop/libweston-desktop.c:138 #7 0x7f423be80c73 in weston_desktop_xdg_toplevel_ensure_added ../libweston-desktop/xdg-shell.c:362 #8 0x7f423be8207a in weston_desktop_xdg_toplevel_committed ../libweston-desktop/xdg-shell.c:697 #9 0x7f423be84d45 in weston_desktop_xdg_surface_committed ../libweston-desktop/xdg-shell.c:1374 #10 0x7f423be7b382 in weston_desktop_surface_surface_committed ../libweston-desktop/surface.c:174 #11 0x7f424fd378a6 in wl_signal_emit /home/mvlad/install-amd64/include/wayland-server-core.h:481 #12 0x7f424fd510e2 in weston_surface_commit_state ../libweston/compositor.c:4062 #13 0x7f424fd51161 in weston_surface_commit ../libweston/compositor.c:4068 #14 0x7f424fd516ef in surface_commit ../libweston/compositor.c:4146 #15 0x7f424fb597e9 (/usr/lib/x86_64-linux-gnu/libffi.so.8+0x77e9) With commit 'libweston, desktop-shell: Add a wrapper for weston_surface reference' we've removed an explicit weston_view_destroy() call due to a UAF which would've happen if we had close animations enabled, upon terminating a client. In that patch I've incorrectly wrote this happened when animations are off, but in fact it happened when they're on, see the following trace: READ of size 8 at 0x616000026498 thread T0 #0 0x7f757fba8797 in weston_signal_emit_mutable ../shared/signal.c:52 #1 0x7f757fb4bba1 in weston_view_destroy ../libweston/compositor.c:2269 #2 0x7f756bca89c0 in desktop_shell_destroy_surface ../desktop-shell/shell.c:275 #3 0x7f756bcb379e in fade_out_done_idle_cb ../desktop-shell/shell.c:2228 #4 0x7f757faec1da in wl_event_loop_dispatch_idle ../src/event-loop.c:969 #5 0x7f757faec31d in wl_event_loop_dispatch ../src/event-loop.c:1032 #6 0x7f757faea114 in wl_display_run ../src/wayland-server.c:1408 #7 0x7f757ff777ba in wet_main ../compositor/main.c:3589 #8 0x55f765c8d17d in main ../compositor/executable.c:33 #9 0x7f757fd997fc in __libc_start_main ../csu/libc-start.c:332 #10 0x55f765c8d099 in _start (/home/mvlad/install-amd64/bin/weston+0x1099) 0x616000026498 is located 24 bytes inside of 608-byte region [0x616000026480,0x6160000266e0) freed by thread T0 here: #0 0x7f758004c4d7 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127 #1 0x7f757fb4bdc8 in weston_view_destroy ../libweston/compositor.c:2295 #2 0x7f757fb4c14d in weston_surface_unref ../libweston/compositor.c:2334 #3 0x7f756bca898b in desktop_shell_destroy_surface ../desktop-shell/shell.c:273 #4 0x7f756bcb379e in fade_out_done_idle_cb ../desktop-shell/shell.c:2228 #5 0x7f757faec1da in wl_event_loop_dispatch_idle ../src/event-loop.c:969 This patch re-introduces it to avoid leaking the view upon compositor shutdown, but it does it in tandem with weston_desktop_surface_unlink_view(), (which was added in a later patch) and before weston_surface_unref() call. This way we should be safe to terminate/close clients with additional views created by libweston-desktop (pop-ups), but also in other different situations. Verified it in the following circumstances: - terminating a client with close animation on - terminating a client with close animations off - shutting down the compositor with clients running, with and without close animations - terminating top-level clients with additional pop-ups with both with and without close animations Signed-off-by: Marius Vlad --- desktop-shell/shell.c | 1 + 1 file changed, 1 insertion(+) diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c index 4641c030..72b06463 100644 --- a/desktop-shell/shell.c +++ b/desktop-shell/shell.c @@ -268,6 +268,7 @@ desktop_shell_destroy_surface(struct shell_surface *shsurf) } wl_list_remove(&shsurf->children_link); weston_desktop_surface_unlink_view(shsurf->view); + weston_view_destroy(shsurf->view); wl_signal_emit(&shsurf->destroy_signal, shsurf); weston_surface_unref(shsurf->wsurface_anim_fade);