Re: [PATCH weston] compositor-wayland: Fix a use after free

2016-11-24 Thread Dima Ryazanov
Oh damn, I was looking for an API to cancel a callback and couldn't find
anything; didn't realize all I needed to do is destroy it. That makes it so
much simpler. I'll send out a follow-up patch.

Thanks!


On Thu, Nov 24, 2016 at 4:05 AM, Daniel Stone  wrote:

> Hi Dima,
>
> On 24 November 2016 at 06:07, Dima Ryazanov  wrote:
> > When a window is being closed, the frame_done callback often runs after
> > the output is already destroyed, i.e:
> >
> >   wayland_output_start_repaint_loop
> >   input_handle_button
> > wayland_output_destroy
> >   frame_done
> >
> > To fix this, destroy the output from an idle handler (same as
> compositor-x11),
> > and also stop creating new frame_done callbacks.
>
> Hm. The reason for moving destroy to an idle handler in X11, is that
> handle_event may be called 'in the middle of repaint'. I suspect - and
> Derek, please correct me if I'm wrong - that this was because we may
> start the X11 repaint loop, send a request, await a reply, and whilst
> waiting for that reply, receive a DELETE_WINDOW event which causes us
> to delete. Which would be bad.
>
> If that's right, then it's a different root cause. We'll never
> dispatch any Wayland events during repaint; we don't do so ourselves,
> and the only thing that could cause to is EGL, which must only be
> dispatching on its own separate event queue.
>
> So, if the root cause is the frame callback arriving after destroy:
> why not just destroy the frame callback, so we never receive it, and
> can destroy immediately?
>
> Cheers,
> Daniel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston] compositor-wayland: Fix a use after free

2016-11-24 Thread Daniel Stone
Hi Dima,

On 24 November 2016 at 06:07, Dima Ryazanov  wrote:
> When a window is being closed, the frame_done callback often runs after
> the output is already destroyed, i.e:
>
>   wayland_output_start_repaint_loop
>   input_handle_button
> wayland_output_destroy
>   frame_done
>
> To fix this, destroy the output from an idle handler (same as compositor-x11),
> and also stop creating new frame_done callbacks.

Hm. The reason for moving destroy to an idle handler in X11, is that
handle_event may be called 'in the middle of repaint'. I suspect - and
Derek, please correct me if I'm wrong - that this was because we may
start the X11 repaint loop, send a request, await a reply, and whilst
waiting for that reply, receive a DELETE_WINDOW event which causes us
to delete. Which would be bad.

If that's right, then it's a different root cause. We'll never
dispatch any Wayland events during repaint; we don't do so ourselves,
and the only thing that could cause to is EGL, which must only be
dispatching on its own separate event queue.

So, if the root cause is the frame callback arriving after destroy:
why not just destroy the frame callback, so we never receive it, and
can destroy immediately?

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


[PATCH weston] compositor-wayland: Fix a use after free

2016-11-23 Thread Dima Ryazanov
When a window is being closed, the frame_done callback often runs after
the output is already destroyed, i.e:

  wayland_output_start_repaint_loop
  input_handle_button
wayland_output_destroy
  frame_done

To fix this, destroy the output from an idle handler (same as compositor-x11),
and also stop creating new frame_done callbacks.

Signed-off-by: Dima Ryazanov 
---
 libweston/compositor-wayland.c | 59 +++---
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
index d9cbde5..fc5daf4 100644
--- a/libweston/compositor-wayland.c
+++ b/libweston/compositor-wayland.c
@@ -128,6 +128,13 @@ struct wayland_output {
 
struct weston_mode mode;
uint32_t scale;
+
+   bool destroy_pending;
+};
+
+struct output_destroy_data {
+   struct wayland_backend *backend;
+   struct wayland_output *output;
 };
 
 struct wayland_parent_output {
@@ -460,6 +467,9 @@ wayland_output_start_repaint_loop(struct weston_output 
*output_base)
to_wayland_backend(output->base.compositor);
struct wl_callback *callback;
 
+   if (output->destroy_pending)
+   return;
+
/* If this is the initial frame, we need to attach a buffer so that
 * the compositor can map the surface and include it in its render
 * loop. If the surface doesn't end up in the render loop, the frame
@@ -485,6 +495,9 @@ wayland_output_repaint_gl(struct weston_output *output_base,
struct weston_compositor *ec = output->base.compositor;
struct wl_callback *callback;
 
+   if (output->destroy_pending)
+   return 0;
+
callback = wl_surface_frame(output->parent.surface);
wl_callback_add_listener(callback, &frame_listener, output);
 
@@ -696,6 +709,41 @@ wayland_output_destroy(struct weston_output *base)
free(output);
 }
 
+static void
+output_destroy_cb(void *data)
+{
+   struct output_destroy_data *dd = data;
+
+   assert(dd->output->destroy_pending);
+
+   wayland_output_destroy(&dd->output->base);
+   if (wl_list_empty(&dd->backend->compositor->output_list))
+   weston_compositor_exit(dd->backend->compositor);
+
+   free(dd);
+}
+
+static void
+handle_window_closed(struct wayland_backend *backend, struct wayland_output 
*output)
+{
+   struct wl_event_loop *loop;
+   struct output_destroy_data *data;
+
+   assert(!output->destroy_pending);
+
+   data = malloc(sizeof *data);
+   if (!data)
+   return;  /* Not much we can do here. */
+
+   data->backend = backend;
+   data->output = output;
+
+   loop = wl_display_get_event_loop(backend->compositor->wl_display);
+   wl_event_loop_add_idle(loop, output_destroy_cb, data);
+
+   output->destroy_pending = true;
+}
+
 static const struct wl_shell_surface_listener shell_surface_listener;
 
 static int
@@ -1600,13 +1648,9 @@ input_handle_button(void *data, struct wl_pointer 
*pointer,
}
 
if (frame_status(input->output->frame) & FRAME_STATUS_CLOSE) {
-   wayland_output_destroy(&input->output->base);
+   handle_window_closed(input->backend, input->output);
input->output = NULL;
input->keyboard_focus = NULL;
-
-   if 
(wl_list_empty(&input->backend->compositor->output_list))
-   
weston_compositor_exit(input->backend->compositor);
-
return;
}
 
@@ -1964,12 +2008,9 @@ input_handle_touch_up(void *data, struct wl_touch 
*wl_touch,
frame_touch_up(output->frame, input, id);
 
if (frame_status(output->frame) & FRAME_STATUS_CLOSE) {
-   wayland_output_destroy(&output->base);
+   handle_window_closed(input->backend, output);
input->touch_focus = NULL;
input->keyboard_focus = NULL;
-   if 
(wl_list_empty(&input->backend->compositor->output_list))
-   
weston_compositor_exit(input->backend->compositor);
-
return;
}
if (frame_status(output->frame) & FRAME_STATUS_REPAINT)
-- 
2.9.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel