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