[1/1] libweston: add weston_view_set_output()

Submitted by Fabien Lahoudere on April 26, 2018, 9:08 a.m.

Details

Message ID 1524733690-30403-1-git-send-email-fabien.lahoudere@collabora.com
State Accepted
Commit e7a52fbb7d86c6810346ae0f058fb44a84d0754e
Headers show
Series "Series without cover letter" ( rev: 1 ) in Wayland (DEPRECATED)

Not browsing as part of any series.

Commit Message

Fabien Lahoudere April 26, 2018, 9:08 a.m.
From: Semi Malinen <semi.malinen@ge.com>

Instead of desktop shell assigning view outputs directly,
use a new method, weston_view_set_output(). The method can
set up an output destroy listener to make sure that views
do not have stale output pointers.

Without this patch it is possible to end up in a scenario
where, e.g. configure_static_view() accesses memory that
has already been freed. The scenario can be provoked by
repeatedly plugging and unplugging a display. The faulty
memory accesses are reported by valgrind.

Signed-off-by: Semi Malinen <semi.malinen@ge.com>
Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
---
 desktop-shell/shell.c  | 11 +++++++----
 libweston/compositor.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 libweston/compositor.h |  4 ++++
 3 files changed, 52 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index b846e30..1e153cb 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -580,7 +580,7 @@  create_focus_surface(struct weston_compositor *ec,
 		free(fsurf);
 		return NULL;
 	}
-	fsurf->view->output = output;
+	weston_view_set_output(fsurf->view, output);
 	fsurf->view->is_mapped = true;
 
 	weston_surface_set_size(surface, output->width, output->height);
@@ -2464,7 +2464,7 @@  map(struct desktop_shell *shell, struct shell_surface *shsurf,
 	shsurf->view->is_mapped = true;
 	if (shsurf->state.maximized) {
 		surface->output = shsurf->output;
-		shsurf->view->output = shsurf->output;
+		weston_view_set_output(shsurf->view, shsurf->output);
 	}
 
 	if (!shell->locked) {
@@ -2881,6 +2881,9 @@  configure_static_view(struct weston_view *ev, struct weston_layer *layer, int x,
 {
 	struct weston_view *v, *next;
 
+	if (!ev->output)
+		return;
+
 	wl_list_for_each_safe(v, next, &layer->view_list.link, layer_link.link) {
 		if (v->output == ev->output && v != ev) {
 			weston_view_unmap(v);
@@ -2970,7 +2973,7 @@  desktop_shell_set_background(struct wl_client *client,
 	surface->committed_private = shell;
 	weston_surface_set_label_func(surface, background_get_label);
 	surface->output = weston_head_from_resource(output_resource)->output;
-	view->output = surface->output;
+	weston_view_set_output(view, surface->output);
 
 	sh_output = find_shell_output_from_weston_output(shell, surface->output);
 	if (sh_output->background_surface) {
@@ -3067,7 +3070,7 @@  desktop_shell_set_panel(struct wl_client *client,
 	surface->committed_private = shell;
 	weston_surface_set_label_func(surface, panel_get_label);
 	surface->output = weston_head_from_resource(output_resource)->output;
-	view->output = surface->output;
+	weston_view_set_output(view, surface->output);
 
 	sh_output = find_shell_output_from_weston_output(shell, surface->output);
 	if (sh_output->panel_surface) {
diff --git a/libweston/compositor.c b/libweston/compositor.c
index dbf61ef..9be66fc 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -1021,6 +1021,45 @@  weston_surface_update_output_mask(struct weston_surface *es, uint32_t mask)
 	}
 }
 
+static void
+notify_view_output_destroy(struct wl_listener *listener, void *data)
+{
+	struct weston_view *view =
+		container_of(listener,
+		     struct weston_view, output_destroy_listener);
+
+	view->output = NULL;
+	view->output_destroy_listener.notify = NULL;
+}
+
+/** Set the primary output of the view
+ *
+ * \param view The view whose primary output to set
+ * \param output The new primary output for the view
+ *
+ * Set \a output to be the primary output of the \a view.
+ *
+ * Notice that the assignment may be temporary; the primary output could be
+ * automatically changed. Hence, one cannot rely on the value persisting.
+ *
+ * Passing NULL as /a output will set the primary output to NULL.
+ */
+WL_EXPORT void
+weston_view_set_output(struct weston_view *view, struct weston_output *output)
+{
+	if (view->output_destroy_listener.notify) {
+		wl_list_remove(&view->output_destroy_listener.link);
+		view->output_destroy_listener.notify = NULL;
+	}
+	view->output = output;
+	if (output) {
+		view->output_destroy_listener.notify =
+			notify_view_output_destroy;
+		wl_signal_add(&output->destroy_signal,
+			      &view->output_destroy_listener);
+	}
+}
+
 /** Recalculate which output(s) the surface has views displayed on
  *
  * \param es  The surface to remap to outputs
@@ -1112,7 +1151,7 @@  weston_view_assign_output(struct weston_view *ev)
 	}
 	pixman_region32_fini(&region);
 
-	ev->output = new_output;
+	weston_view_set_output(ev, new_output);
 	ev->output_mask = mask;
 
 	weston_surface_assign_output(ev->surface);
@@ -1822,7 +1861,7 @@  weston_view_unmap(struct weston_view *view)
 		return;
 
 	weston_view_damage_below(view);
-	view->output = NULL;
+	weston_view_set_output(view, NULL);
 	view->plane = NULL;
 	view->is_mapped = false;
 	weston_layer_entry_remove(&view->layer_link);
diff --git a/libweston/compositor.h b/libweston/compositor.h
index ca23407..bfad890 100644
--- a/libweston/compositor.h
+++ b/libweston/compositor.h
@@ -1194,6 +1194,7 @@  struct weston_view {
 	 * view, inheriting the primary output for related views in shells, etc.
 	 */
 	struct weston_output *output;
+	struct wl_listener output_destroy_listener;
 
 	/*
 	 * A more complete representation of all outputs this surface is
@@ -1409,6 +1410,9 @@  void
 weston_version(int *major, int *minor, int *micro);
 
 void
+weston_view_set_output(struct weston_view *view, struct weston_output *output);
+
+void
 weston_view_update_transform(struct weston_view *view);
 
 void

Comments

On Thu, 26 Apr 2018 11:08:10 +0200
Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:

> From: Semi Malinen <semi.malinen@ge.com>
> 
> Instead of desktop shell assigning view outputs directly,
> use a new method, weston_view_set_output(). The method can
> set up an output destroy listener to make sure that views
> do not have stale output pointers.
> 
> Without this patch it is possible to end up in a scenario
> where, e.g. configure_static_view() accesses memory that
> has already been freed. The scenario can be provoked by
> repeatedly plugging and unplugging a display. The faulty
> memory accesses are reported by valgrind.
> 
> Signed-off-by: Semi Malinen <semi.malinen@ge.com>
> Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com>
> ---
>  desktop-shell/shell.c  | 11 +++++++----
>  libweston/compositor.c | 43 +++++++++++++++++++++++++++++++++++++++++--
>  libweston/compositor.h |  4 ++++
>  3 files changed, 52 insertions(+), 6 deletions(-)

Pushed with my R-b:
   03dc95a1..e7a52fbb  master -> master


Thanks,
pq