<div dir="ltr"><div>Neil,<br>Looks good except for the two comments below<br></div>--Jason<br><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, May 19, 2014 at 12:23 PM, Neil Roberts <span dir="ltr"><<a href="mailto:neil@linux.intel.com" target="_blank">neil@linux.intel.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Previously when an output was unplugged the clients' resources for it<br>
were left around and if they were used in a request it could cause<br>
Weston to access invalid memory. Now when an output is unplugged the<br>
resources for it are marked as zombies. This is done by setting a<br>
dummy dispatcher and setting the user data to NULL. The dispatcher<br>
ignores all requests except for one which is marked as a destructor.<br>
When the destructor is called the zombie resource will be destroyed.<br>
The opcode for the destructor request is stashed into one of the<br>
pointers in the resource's linked-list node so that it can be compared<br>
against all of the opcodes later. The wl_output interface doesn't<br>
currently have any requests nor a destructor but the intention is that<br>
this mechanism could also be used later for more complicated<br>
interfaces.<br>
<br>
Any requests that take a wl_output as an argument have also been<br>
patched to take into account that the output can now be a zombie.<br>
These are handled on a case-by-case basis but in general if the<br>
argument is allowed to be NULL then zombie resources will be treated<br>
as such. Otherwise the request is generally completely ignored.<br>
<br>
The screenshooter interface is a special case because that has a<br>
callback so we can't really just ignore the request. Instead the<br>
buffer for the screenshot is cleared and the callback is immediately<br>
invoked.<br>
<br>
The code for zombifying a resource is based on a snippet by Jason<br>
Esktrand.<br>
<br>
<a href="https://bugs.freedesktop.org/show_bug.cgi?id=78415" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=78415</a><br>
---<br>
 desktop-shell/input-panel.c         |  6 +++++<br>
 desktop-shell/shell.c               | 38 ++++++++++++++++++++++-------<br>
 fullscreen-shell/fullscreen-shell.c | 12 ++++++++++<br>
 src/compositor.c                    | 36 ++++++++++++++++++++++++++++<br>
 src/compositor.h                    |  3 +++<br>
 src/screenshooter.c                 | 48 +++++++++++++++++++++++++++++++++----<br>
 6 files changed, 130 insertions(+), 13 deletions(-)<br>
<br>
diff --git a/desktop-shell/input-panel.c b/desktop-shell/input-panel.c<br>
index 7623f6c..df365fb 100644<br>
--- a/desktop-shell/input-panel.c<br>
+++ b/desktop-shell/input-panel.c<br>
@@ -252,6 +252,12 @@ input_panel_surface_set_toplevel(struct wl_client *client,<br>
        struct input_panel_surface *input_panel_surface =<br>
                wl_resource_get_user_data(resource);<br>
        struct desktop_shell *shell = input_panel_surface->shell;<br>
+       struct weston_output *output;<br>
+<br>
+       output = wl_resource_get_user_data(output_resource);<br>
+       /* Ignore the request if the output has become a zombie */<br>
+       if (output == NULL)<br>
+               return;<br>
<br>
        wl_list_insert(&shell->input_panel.surfaces,<br>
                       &input_panel_surface->link);<br>
diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c<br>
index dd0b2f9..97c5d11 100644<br>
--- a/desktop-shell/shell.c<br>
+++ b/desktop-shell/shell.c<br>
@@ -2431,10 +2431,12 @@ shell_surface_set_fullscreen(struct wl_client *client,<br>
        struct shell_surface *shsurf = wl_resource_get_user_data(resource);<br>
        struct weston_output *output;<br>
<br>
-       if (output_resource)<br>
+       if (output_resource) {<br>
                output = wl_resource_get_user_data(output_resource);<br>
-       else<br>
+               /* Output can be NULL here if the resource is a zombie */<br>
+       } else {<br>
                output = NULL;<br>
+       }<br>
<br>
        shell_surface_set_parent(shsurf, NULL);<br>
<br>
@@ -2566,10 +2568,12 @@ shell_surface_set_maximized(struct wl_client *client,<br>
        shsurf->type = SHELL_SURFACE_TOPLEVEL;<br>
        shell_surface_set_parent(shsurf, NULL);<br>
<br>
-       if (output_resource)<br>
+       if (output_resource) {<br>
                output = wl_resource_get_user_data(output_resource);<br>
-       else<br>
+               /* output can be NULL here if the resource is zombified */<br>
+       } else {<br>
                output = NULL;<br>
+       }<br>
<br>
        shell_surface_set_output(shsurf, output);<br>
<br>
@@ -3487,10 +3491,13 @@ xdg_surface_set_fullscreen(struct wl_client *client,<br>
        shsurf->state_requested = true;<br>
        shsurf->requested_state.fullscreen = true;<br>
<br>
-       if (output_resource)<br>
+       if (output_resource) {<br>
                output = wl_resource_get_user_data(output_resource);<br>
-       else<br>
+               /* output can still be NULL here if the resource has<br>
+                * become a zombie */<br>
+       } else {<br>
                output = NULL;<br>
+       }<br>
<br>
        shell_surface_set_output(shsurf, output);<br>
        shsurf->fullscreen_output = shsurf->output;<br>
@@ -3919,6 +3926,10 @@ desktop_shell_set_background(struct wl_client *client,<br>
                return;<br>
        }<br>
<br>
+       /* Skip the request if the output has become a zombie */<br>
+       if (wl_resource_get_user_data(output_resource) == NULL)<br>
+               return;<br>
+<br>
        wl_list_for_each_safe(view, next, &surface->views, surface_link)<br>
                weston_view_destroy(view);<br>
        view = weston_view_create(surface);<br>
@@ -3953,6 +3964,7 @@ desktop_shell_set_panel(struct wl_client *client,<br>
        struct desktop_shell *shell = wl_resource_get_user_data(resource);<br>
        struct weston_surface *surface =<br>
                wl_resource_get_user_data(surface_resource);<br>
+       struct weston_output *output;<br>
        struct weston_view *view, *next;<br>
<br>
        if (surface->configure) {<br>
@@ -3962,14 +3974,20 @@ desktop_shell_set_panel(struct wl_client *client,<br>
                return;<br>
        }<br>
<br>
+       output = wl_resource_get_user_data(output_resource);<br>
+<br>
+       /* Skip the request if the output has become a zombie */<br>
+       if (output == NULL)<br>
+               return;<br>
+<br>
        wl_list_for_each_safe(view, next, &surface->views, surface_link)<br>
                weston_view_destroy(view);<br>
        view = weston_view_create(surface);<br>
<br>
        surface->configure = panel_configure;<br>
        surface->configure_private = shell;<br>
-       surface->output = wl_resource_get_user_data(output_resource);<br>
-       view->output = surface->output;<br>
+       surface->output = output;<br>
+       view->output = output;<br>
        desktop_shell_send_configure(resource, 0,<br>
                                     surface_resource,<br>
                                     surface->output->width,<br>
@@ -5362,6 +5380,10 @@ screensaver_set_surface(struct wl_client *client,<br>
        struct weston_output *output = wl_resource_get_user_data(output_resource);<br>
        struct weston_view *view, *next;<br>
<br>
+       /* Skip the request if the output has become a zombie */<br>
+       if (output == NULL)<br>
+               return;<br>
+<br>
        /* Make sure we only have one view */<br>
        wl_list_for_each_safe(view, next, &surface->views, surface_link)<br>
                weston_view_destroy(view);<br>
diff --git a/fullscreen-shell/fullscreen-shell.c b/fullscreen-shell/fullscreen-shell.c<br>
index a8dad8e..c3bc517 100644<br>
--- a/fullscreen-shell/fullscreen-shell.c<br>
+++ b/fullscreen-shell/fullscreen-shell.c<br>
@@ -673,6 +673,13 @@ fullscreen_shell_present_surface(struct wl_client *client,<br>
<br>
        if (output_res) {<br>
                output = wl_resource_get_user_data(output_res);<br>
+               /* output can still be NULL here if the resource has<br>
+                * become a zombie */<br>
+       } else {<br>
+               output = NULL;<br>
+       }<br></blockquote><div><br></div><div>Yeah, these are the wrong semantics.  If they specify an output and it turns out to be a zombie, we should do nothing.  A null output in wl_fullscreen_shell.present_surface means "put it somewhere".  In the case of weston's implementation, it goes on all outputs.  We don't want a surface to accidentally end up everywhere when it was intended for a particular output.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+       if (output) {<br>
                fsout = fs_output_for_output(output);<br>
                fs_output_set_surface(fsout, surface, method, 0, 0);<br>
        } else {<br>
@@ -712,6 +719,11 @@ fullscreen_shell_present_surface_for_mode(struct wl_client *client,<br>
        struct fs_output *fsout;<br>
<br>
        output = wl_resource_get_user_data(output_res);<br>
+<br>
+       /* Skip the request if the output has become a zombie */<br>
+       if (output == NULL)<br>
+               return;<br>
+<br>
        fsout = fs_output_for_output(output);<br>
<br>
        if (surface_res == NULL) {<br>
diff --git a/src/compositor.c b/src/compositor.c<br>
index 574db2d..ea0c9b4 100644<br>
--- a/src/compositor.c<br>
+++ b/src/compositor.c<br>
@@ -3107,9 +3107,42 @@ weston_compositor_remove_output(struct weston_compositor *compositor,<br>
        }<br>
 }<br>
<br>
+static int<br>
+zombie_dispatcher(const void *data, void *resource, uint32_t opcode,<br>
+                 const struct wl_message *msg, union wl_argument *args)<br>
+{<br>
+       struct wl_list *link = wl_resource_get_link(resource);<br>
+       uint32_t destructor = (uintptr_t) link->next;<br>
+<br>
+       if (opcode == destructor)<br>
+               wl_resource_destroy(resource);<br>
+<br>
+       return 0;<br>
+}<br>
+<br>
+WL_EXPORT void<br>
+weston_resource_zombify(struct wl_resource *res, uint32_t destructor)<br>
+{<br>
+       struct wl_list *link;<br>
+<br>
+       /* Set a dummy dispatcher so that all requests will be<br>
+        * ignored. The NULL user_data is used to mark that this is a<br>
+        * zombie */<br>
+       wl_resource_set_dispatcher(res, zombie_dispatcher,<br>
+                                  NULL /* implementation */,<br>
+                                  NULL /* user data */,<br>
+                                  NULL /* destroy */);<br>
+       /* Stash the destructor opcode in one of the pointers of the<br>
+        * list node so we can compare it later in the dispatcher */<br>
+       link = wl_resource_get_link(res);<br>
+       link->next = (struct wl_list *) (uintptr_t) destructor;<br></blockquote><div><br></div><div>This is a bit ugly.  In my original snippit, I pulled a similar stunt and stashed the opcode in the implementation implementation pointer.  Is there some particular reason why you opted for the internal wl_list link instead?  I guess it doesn't really matter that much, but the implementation is for storring dispatcher-specific stuff (such as an opcode).  Putting it in the internal wl_list link prevents someone from keeping a list of zombie resources (no idea why they'd want to though).<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+<br>
 WL_EXPORT void<br>
 weston_output_destroy(struct weston_output *output)<br>
 {<br>
+       struct wl_resource *resource, *tmp;<br>
+<br>
        output->destroying = 1;<br>
<br>
        weston_compositor_remove_output(output->compositor, output);<br>
@@ -3124,6 +3157,9 @@ weston_output_destroy(struct weston_output *output)<br>
        output->compositor->output_id_pool &= ~(1 << output->id);<br>
<br>
        wl_global_destroy(output->global);<br>
+<br>
+       wl_resource_for_each_safe(resource, tmp, &output->resource_list)<br>
+               weston_resource_zombify(resource, ~0);<br>
 }<br>
<br>
 static void<br>
diff --git a/src/compositor.h b/src/compositor.h<br>
index 057f8be..6d9800a 100644<br>
--- a/src/compositor.h<br>
+++ b/src/compositor.h<br>
@@ -1416,6 +1416,9 @@ weston_transformed_region(int width, int height,<br>
 void *<br>
 weston_load_module(const char *name, const char *entrypoint);<br>
<br>
+void<br>
+weston_resource_zombify(struct wl_resource *res, uint32_t destructor);<br>
+<br>
 #ifdef  __cplusplus<br>
 }<br>
 #endif<br>
diff --git a/src/screenshooter.c b/src/screenshooter.c<br>
index 369e920..574fde1 100644<br>
--- a/src/screenshooter.c<br>
+++ b/src/screenshooter.c<br>
@@ -213,6 +213,33 @@ weston_screenshooter_shoot(struct weston_output *output,<br>
 }<br>
<br>
 static void<br>
+shoot_zombie_output(struct weston_buffer *buffer,<br>
+                   weston_screenshooter_done_func_t done,<br>
+                   void *data)<br>
+{<br>
+       struct wl_shm_buffer *shm_buffer;<br>
+       int32_t height, stride;<br>
+       void *pixels;<br>
+<br>
+       shm_buffer = wl_shm_buffer_get(buffer->resource);<br>
+<br>
+       if (shm_buffer == NULL) {<br>
+               done(data, WESTON_SCREENSHOOTER_BAD_BUFFER);<br>
+               return;<br>
+       }<br>
+<br>
+       height = wl_shm_buffer_get_height(shm_buffer);<br>
+       stride = wl_shm_buffer_get_stride(shm_buffer);<br>
+       pixels = wl_shm_buffer_get_data(shm_buffer);<br>
+<br>
+       wl_shm_buffer_begin_access(shm_buffer);<br>
+       memset(pixels, 0, height * stride);<br>
+       wl_shm_buffer_end_access(shm_buffer);<br>
+<br>
+       done(data, WESTON_SCREENSHOOTER_SUCCESS);<br>
+}<br>
+<br>
+static void<br>
 screenshooter_done(void *data, enum weston_screenshooter_outcome outcome)<br>
 {<br>
        struct wl_resource *resource = data;<br>
@@ -235,17 +262,28 @@ screenshooter_shoot(struct wl_client *client,<br>
                    struct wl_resource *output_resource,<br>
                    struct wl_resource *buffer_resource)<br>
 {<br>
-       struct weston_output *output =<br>
-               wl_resource_get_user_data(output_resource);<br>
-       struct weston_buffer *buffer =<br>
-               weston_buffer_from_resource(buffer_resource);<br>
+       struct weston_output *output;<br>
+       struct weston_buffer *buffer;<br>
+<br>
+       output = wl_resource_get_user_data(output_resource);<br>
+       buffer = weston_buffer_from_resource(buffer_resource);<br>
<br>
        if (buffer == NULL) {<br>
                wl_resource_post_no_memory(resource);<br>
                return;<br>
        }<br>
<br>
-       weston_screenshooter_shoot(output, buffer, screenshooter_done, resource);<br>
+       /* If the output has become a zombie then we obviously can't<br>
+        * take a screenshot. We don't want to report an error because<br>
+        * that would likely make the client abort and this is an<br>
+        * unavoidable occurence. Instead we can just clear the<br>
+        * buffer. This is probably reasonable because that is what<br>
+        * the output will actually if it is unplugged */<br>
+       if (output == NULL)<br>
+               shoot_zombie_output(buffer, screenshooter_done, resource);<br>
+       else<br>
+               weston_screenshooter_shoot(output, buffer,<br>
+                                          screenshooter_done, resource);<br>
 }<br>
<br>
 struct screenshooter_interface screenshooter_implementation = {<br>
<span class="HOEnZb"><font color="#888888">--<br>
1.9.0<br>
<br>
_______________________________________________<br>
wayland-devel mailing list<br>
<a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
</font></span></blockquote></div><br></div></div></div></div>