From 8df487e0ff3b79e7411a19d302a475217f7d1016 Mon Sep 17 00:00:00 2001 From: Marius Vlad Date: Mon, 14 Feb 2022 22:42:22 +0200 Subject: [PATCH] libweston, desktop-shell: Add a wrapper for weston_surface reference Similar to how we do it with drm_fb ref counts, increase a reference count and return the same object. Plug-in in desktop-shell when we map up the view in order to survive a weston_surface destruction. Astute readers will notice that this patch removes weston_view_destroy() while keeping the balance between removing and adding a weston_surface_unref() call in desktop_shell_destroy_surface(). The reason is to let weston_surface_unref() handle destruction on its own. If multiple references are taken, then weston_surface_unref() doesn't destroy the view, it just decreases the reference, with a latter call to weston_surface_unref() to determine if the view should be destroyed as well. This situation happens if we have close animation enabled, were we have more than one reference taken: one when mapping the view/surface and when when the surface itself was created, (what we call, a weak reference). If only a single reference is taken (for instance if we don't have close animations enabled) then this weston_surface_unref() call is inert as that reference is not set-up, leaving libweston to handle the view destruction. Following that with a weston_view_destroy() explicit call would cause a UAF as the view was previous destroyed by a weston_surface_unref() call. A side-effect of not keeping the weston_view_destroy() call would happen when tearing down the compositor. If close animations are enabled, weston_surface_unref() would not destroy the view, and because weston_view_destroy() no longer exists, we would still have the view in the other layers by the time we check-up if layers have views present. Signed-off-by: Marius Vlad (cherry picked from commit bd8314078de81cf19bfc75e23c12d730dc18c63e) While the original patch was adding a weston_surface_ref() wrapper, this patch doesn't add any wrapper to avoid a minor version bump, but instead achieves a similar outcome by inlining the wrapper version being added by the original commit. Further more, as the original patch was dependent on another patch, any mention of weston_surface_unref() in the commit description should instead be replaced with weston_surface_destroy(). Signed-off-by: Marius Vlad --- desktop-shell/shell.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c index 4ca78979..31258e81 100644 --- a/desktop-shell/shell.c +++ b/desktop-shell/shell.c @@ -101,6 +101,7 @@ struct shell_surface { struct weston_desktop_surface *desktop_surface; struct weston_view *view; + struct weston_surface *wsurface_anim_fade; int32_t last_width, last_height; struct desktop_shell *shell; @@ -263,8 +264,8 @@ desktop_shell_destroy_surface(struct shell_surface *shsurf) wl_list_remove(&shsurf->children_link); wl_signal_emit(&shsurf->destroy_signal, shsurf); + weston_surface_destroy(shsurf->wsurface_anim_fade); - weston_view_destroy(shsurf->view); if (shsurf->output_destroy_listener.notify) { wl_list_remove(&shsurf->output_destroy_listener.link); shsurf->output_destroy_listener.notify = NULL; @@ -2376,8 +2377,6 @@ desktop_surface_removed(struct weston_desktop_surface *desktop_surface, weston_fade_run(shsurf->view, 1.0, 0.0, 300.0, fade_out_done, shsurf); return; - } else { - weston_surface_destroy(surface); } } @@ -2500,8 +2499,12 @@ desktop_surface_committed(struct weston_desktop_surface *desktop_surface, if (!weston_surface_is_mapped(surface)) { map(shell, shsurf, sx, sy); surface->is_mapped = true; - if (shsurf->shell->win_close_animation_type == ANIMATION_FADE) - ++surface->ref_count; + /* as we need to survive the weston_surface destruction we'll + * need to take another reference */ + if (shsurf->shell->win_close_animation_type == ANIMATION_FADE) { + surface->ref_count++; + shsurf->wsurface_anim_fade = surface; + } return; }