[PATCH weston v3 3/7] libweston: Make weston_seat release safe
Quentin Glidic
sardemff7+wayland at sardemff7.net
Thu Feb 15 08:17:01 UTC 2018
On 2/14/18 2:05 PM, Alexandros Frantzis wrote:
> Ensure the server can safely handle client requests for wl_seat resource
> that have become inert due to weston_seat object release and subsequent
> destruction.
>
> The clean-up involves, among other things, unsetting the destroyed
> weston_seat object from the user data of wl_seat resources, and handling
> this NULL user data case where required.
>
> The list of sites extracting and using weston_seat object from wl_seat
> resources which were audited for this patch are:
>
> Legend:
> N/A = Not Applicable (not implemented by weston)
> FIXED = Fixed in the commit
> OK = Already works correctly
>
> == keyboard_shortcuts_inhibit_unstable_v1 ==
> [N/A] zwp_keyboard_shortcuts_inhibit_manager_v1.inhibit_shortcuts
> == tablet_input_unstable_v{1,2} ==
> [N/A] zwp_tablet_manager_v{1,2}.get_tablet_seat
> == text_input_unstable_v1 ==
> [FIXED] zwp_text_input_v1.activate
> [FIXED] zwp_text_input_v1.deactivate
> == wl_data_device ==
> [FIXED] wl_data_device_manager.get_data_device
> [OK] wl_data_device.start_drag
> [FIXED] wl_data_device.set_selection
> [OK] wl_data_device.release
> == wl_shell ==
> [FIXED] wl_shell_surface.move
> [FIXED] wl_shell_surface.resize
> [FIXED] wl_shell_surface.set_popup
> == xdg_shell and xdg_shell_unstable_v6 ==
> [FIXED] xdg_toplevel.show_window_menu
> [FIXED] xdg_toplevel.move
> [FIXED] xdg_toplevel.resize
> [FIXED] xdg_popup.grab
> == xdg_shell_unstable_v5 ==
> [FIXED] xdg_shell.get_xdg_popup
> [FIXED] xdg_surface.show_window_menu
> [FIXED] xdg_surface.move
> [FIXED] xdg_surface.resize
>
> Signed-off-by: Alexandros Frantzis <alexandros.frantzis at collabora.com>
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> ---
> Changes in v3:
> - Drop xdg_shell v5 changes since support for v5 has been removed.
> - Handle inert seats more transparently in libweston-desktop, by ensuring that
> the weston_desktop_seat_from_seat,
> weston_desktop_seat_popup_grab_get_topmost_surface, and
> weston desktop_seat_popup_grab_start functions gracefully handle NULL
> seat pointer arguments internally.
>
> Changes in v2:
> - Properly create inert resources in seat_get_pointer/touch/keyboard.
> - Ensure all sites which have a wl_seat input resource can deal
> with inert resources.
>
> compositor/text-backend.c | 8 ++++--
> libweston-desktop/seat.c | 13 ++++++---
> libweston-desktop/wl-shell.c | 9 +++++-
> libweston-desktop/xdg-shell-v6.c | 24 ++++++++++++++++
> libweston/data-device.c | 15 ++++++----
> libweston/input.c | 61 ++++++++++++++++++++++++++++------------
> 6 files changed, 100 insertions(+), 30 deletions(-)
>
> diff --git a/compositor/text-backend.c b/compositor/text-backend.c
> index e6ee249c..4d8c085b 100644
> --- a/compositor/text-backend.c
> +++ b/compositor/text-backend.c
> @@ -193,10 +193,14 @@ text_input_activate(struct wl_client *client,
> {
> struct text_input *text_input = wl_resource_get_user_data(resource);
> struct weston_seat *weston_seat = wl_resource_get_user_data(seat);
> - struct input_method *input_method = weston_seat->input_method;
> + struct input_method *input_method;
> struct weston_compositor *ec = text_input->ec;
> struct text_input *current;
>
> + if (!weston_seat)
> + return;
> +
> + input_method = weston_seat->input_method;
> if (input_method->input == text_input)
> return;
>
> @@ -237,7 +241,7 @@ text_input_deactivate(struct wl_client *client,
> {
> struct weston_seat *weston_seat = wl_resource_get_user_data(seat);
>
> - if (weston_seat->input_method->input)
> + if (weston_seat && weston_seat->input_method->input)
> deactivate_input_method(weston_seat->input_method);
> }
>
> diff --git a/libweston-desktop/seat.c b/libweston-desktop/seat.c
> index 382b9e41..4e51ee0f 100644
> --- a/libweston-desktop/seat.c
> +++ b/libweston-desktop/seat.c
> @@ -242,6 +242,9 @@ weston_desktop_seat_from_seat(struct weston_seat *wseat)
> struct wl_listener *listener;
> struct weston_desktop_seat *seat;
>
> + if (wseat == NULL)
> + return NULL;
> +
> listener = wl_signal_get(&wseat->destroy_signal,
> weston_desktop_seat_destroy);
> if (listener != NULL)
> @@ -270,7 +273,7 @@ weston_desktop_seat_from_seat(struct weston_seat *wseat)
> struct weston_desktop_surface *
> weston_desktop_seat_popup_grab_get_topmost_surface(struct weston_desktop_seat *seat)
> {
> - if (wl_list_empty(&seat->popup_grab.surfaces))
> + if (seat == NULL || wl_list_empty(&seat->popup_grab.surfaces))
> return NULL;
>
> struct wl_list *grab_link = seat->popup_grab.surfaces.next;
> @@ -284,9 +287,11 @@ weston_desktop_seat_popup_grab_start(struct weston_desktop_seat *seat,
> {
> assert(seat->popup_grab.client == NULL || seat->popup_grab.client == client);
>
Here we should have "seat == NULL || " added, or the assert would
segfault, and this patch makes it legal to call grab_start() with NULL.
> - struct weston_keyboard *keyboard = weston_seat_get_keyboard(seat->seat);
> - struct weston_pointer *pointer = weston_seat_get_pointer(seat->seat);
> - struct weston_touch *touch = weston_seat_get_touch(seat->seat);
> + struct weston_seat *wseat = seat != NULL ? seat->seat : NULL;
> + /* weston_seat_get_* functions can properly handle a NULL wseat */
> + struct weston_keyboard *keyboard = weston_seat_get_keyboard(wseat);
> + struct weston_pointer *pointer = weston_seat_get_pointer(wseat);
> + struct weston_touch *touch = weston_seat_get_touch(wseat);
>
> if ((keyboard == NULL || keyboard->grab_serial != serial) &&
> (pointer == NULL || pointer->grab_serial != serial) &&
> diff --git a/libweston-desktop/wl-shell.c b/libweston-desktop/wl-shell.c
> index 66553f45..8467dfb8 100644
> --- a/libweston-desktop/wl-shell.c
> +++ b/libweston-desktop/wl-shell.c
> @@ -220,6 +220,9 @@ weston_desktop_wl_shell_surface_protocol_move(struct wl_client *wl_client,
> struct weston_desktop_wl_shell_surface *surface =
> weston_desktop_surface_get_implementation_data(dsurface);
>
> + if (seat == NULL)
> + return;
> +
> weston_desktop_api_move(surface->desktop, dsurface, seat, serial);
> }
>
> @@ -238,6 +241,9 @@ weston_desktop_wl_shell_surface_protocol_resize(struct wl_client *wl_client,
> enum weston_desktop_surface_edge surf_edges =
> (enum weston_desktop_surface_edge) edges;
>
> + if (seat == NULL)
> + return;
> +
> weston_desktop_api_resize(surface->desktop, dsurface, seat, serial, surf_edges);
> }
>
> @@ -328,7 +334,8 @@ weston_desktop_wl_shell_surface_protocol_set_popup(struct wl_client *wl_client,
> struct weston_desktop_wl_shell_surface *surface =
> weston_desktop_surface_get_implementation_data(dsurface);
>
> - if (seat == NULL) {
> + /* Check that if we have a valid wseat we also got a valid desktop seat */
> + if (wseat != NULL && seat == NULL) {
> wl_client_post_no_memory(wl_client);
> return;
> }
> diff --git a/libweston-desktop/xdg-shell-v6.c b/libweston-desktop/xdg-shell-v6.c
> index 4db3748b..618cce5e 100644
> --- a/libweston-desktop/xdg-shell-v6.c
> +++ b/libweston-desktop/xdg-shell-v6.c
> @@ -371,6 +371,9 @@ weston_desktop_xdg_toplevel_protocol_show_window_menu(struct wl_client *wl_clien
> struct weston_desktop_xdg_toplevel *toplevel =
> weston_desktop_surface_get_implementation_data(dsurface);
>
> + if (seat == NULL)
> + return;
> +
> if (!toplevel->base.configured) {
> wl_resource_post_error(toplevel->resource,
> ZXDG_SURFACE_V6_ERROR_NOT_CONSTRUCTED,
> @@ -395,6 +398,9 @@ weston_desktop_xdg_toplevel_protocol_move(struct wl_client *wl_client,
> struct weston_desktop_xdg_toplevel *toplevel =
> weston_desktop_surface_get_implementation_data(dsurface);
>
> + if (seat == NULL)
> + return;
> +
Here…
> if (!toplevel->base.configured) {
> wl_resource_post_error(toplevel->resource,
> ZXDG_SURFACE_V6_ERROR_NOT_CONSTRUCTED,
> @@ -421,6 +427,9 @@ weston_desktop_xdg_toplevel_protocol_resize(struct wl_client *wl_client,
> enum weston_desktop_surface_edge surf_edges =
> (enum weston_desktop_surface_edge) edges;
>
> + if (seat == NULL)
> + return;
> +
… and here, we should reorder the checks after the if (!configured). It
is a race, so it could happen at a bad time, and hide a race in the
client code sending the request too soon (though since it could not
receive an input event, it should never send the request that early).
Let’s prefer real errors over racy ignores.
> if (!toplevel->base.configured) {
> wl_resource_post_error(toplevel->resource,
> ZXDG_SURFACE_V6_ERROR_NOT_CONSTRUCTED,
> @@ -762,6 +771,12 @@ weston_desktop_xdg_popup_protocol_grab(struct wl_client *wl_client,
> bool parent_is_toplevel =
> popup->parent->role == WESTON_DESKTOP_XDG_SURFACE_ROLE_TOPLEVEL;
>
> + /* Check that if we have a valid wseat we also got a valid desktop seat */
> + if (wseat != NULL && seat == NULL) {
> + wl_client_post_no_memory(wl_client);
> + return;
> + }
> +
Good catch. We could put it everywhere but that may be overkill.
> if (popup->committed) {
> wl_resource_post_error(popup->resource,
> ZXDG_POPUP_V6_ERROR_INVALID_GRAB,
> @@ -769,6 +784,15 @@ weston_desktop_xdg_popup_protocol_grab(struct wl_client *wl_client,
> return;
> }
>
> + /* If seat is NULL then get_topmost_surface will return NULL. In
> + * combination with setting parent_is_toplevel to TRUE here we will
> + * avoid posting an error, and we will instead gracefully fail the
> + * grab and dismiss the surface.
> + * FIXME: this is a hack because currently we cannot check the topmost
> + * parent with a destroyed weston_seat */
> + if (seat == NULL)
> + parent_is_toplevel = true;
> +
Good comment. Now I’m not sure if it belongs here or in
weston_desktop_seat_popup_grab_get_topmost_surface() (with a separate if).
Probably no big deal anyway.
With these 3 (not counting the comment) things fixed, the
libweston-desktop stuff is:
Reviewed-by: Quentin Glidic <sardemff7+git at sardemff7.net>
Thanks,
> topmost = weston_desktop_seat_popup_grab_get_topmost_surface(seat);
> if ((topmost == NULL && !parent_is_toplevel) ||
> (topmost != NULL && topmost != popup->parent->desktop_surface)) {
> diff --git a/libweston/data-device.c b/libweston/data-device.c
> index b4bb4b37..e3dbee3e 100644
> --- a/libweston/data-device.c
> +++ b/libweston/data-device.c
> @@ -1167,9 +1167,10 @@ data_device_set_selection(struct wl_client *client,
> struct wl_resource *resource,
> struct wl_resource *source_resource, uint32_t serial)
> {
> + struct weston_seat *seat = wl_resource_get_user_data(resource);
> struct weston_data_source *source;
>
> - if (!source_resource)
> + if (!seat || !source_resource)
> return;
>
> source = wl_resource_get_user_data(source_resource);
> @@ -1182,8 +1183,7 @@ data_device_set_selection(struct wl_client *client,
> }
>
> /* FIXME: Store serial and check against incoming serial here. */
> - weston_seat_set_selection(wl_resource_get_user_data(resource),
> - source, serial);
> + weston_seat_set_selection(seat, source, serial);
> }
> static void
> data_device_release(struct wl_client *client, struct wl_resource *resource)
> @@ -1296,8 +1296,13 @@ get_data_device(struct wl_client *client,
> return;
> }
>
> - wl_list_insert(&seat->drag_resource_list,
> - wl_resource_get_link(resource));
> + if (seat) {
> + wl_list_insert(&seat->drag_resource_list,
> + wl_resource_get_link(resource));
> + } else {
> + wl_list_init(wl_resource_get_link(resource));
> + }
> +
> wl_resource_set_implementation(resource, &data_device_interface,
> seat, unbind_data_device);
> }
> diff --git a/libweston/input.c b/libweston/input.c
> index 647268af..da002548 100644
> --- a/libweston/input.c
> +++ b/libweston/input.c
> @@ -2420,13 +2420,10 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
> * This prevents a race between the compositor sending new
> * capabilities and the client trying to use the old ones.
> */
> - struct weston_pointer *pointer = seat->pointer_state;
> + struct weston_pointer *pointer = seat ? seat->pointer_state : NULL;
> struct wl_resource *cr;
> struct weston_pointer_client *pointer_client;
>
> - if (!pointer)
> - return;
> -
> cr = wl_resource_create(client, &wl_pointer_interface,
> wl_resource_get_version(resource), id);
> if (cr == NULL) {
> @@ -2434,6 +2431,15 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
> return;
> }
>
> + wl_list_init(wl_resource_get_link(cr));
> + wl_resource_set_implementation(cr, &pointer_interface, pointer,
> + unbind_pointer_client_resource);
> +
> + /* If we don't have a pointer_state, the resource is inert, so there
> + * is nothing more to set up */
> + if (!pointer)
> + return;
> +
> pointer_client = weston_pointer_ensure_pointer_client(pointer, client);
> if (!pointer_client) {
> wl_client_post_no_memory(client);
> @@ -2442,8 +2448,6 @@ seat_get_pointer(struct wl_client *client, struct wl_resource *resource,
>
> wl_list_insert(&pointer_client->pointer_resources,
> wl_resource_get_link(cr));
> - wl_resource_set_implementation(cr, &pointer_interface, pointer,
> - unbind_pointer_client_resource);
>
> if (pointer->focus && pointer->focus->surface->resource &&
> wl_resource_get_client(pointer->focus->surface->resource) == client) {
> @@ -2507,12 +2511,9 @@ seat_get_keyboard(struct wl_client *client, struct wl_resource *resource,
> * This prevents a race between the compositor sending new
> * capabilities and the client trying to use the old ones.
> */
> - struct weston_keyboard *keyboard = seat->keyboard_state;
> + struct weston_keyboard *keyboard = seat ? seat->keyboard_state : NULL;
> struct wl_resource *cr;
>
> - if (!keyboard)
> - return;
> -
> cr = wl_resource_create(client, &wl_keyboard_interface,
> wl_resource_get_version(resource), id);
> if (cr == NULL) {
> @@ -2520,12 +2521,19 @@ seat_get_keyboard(struct wl_client *client, struct wl_resource *resource,
> return;
> }
>
> + wl_list_init(wl_resource_get_link(cr));
> + wl_resource_set_implementation(cr, &keyboard_interface,
> + keyboard, unbind_resource);
> +
> + /* If we don't have a keyboard_state, the resource is inert, so there
> + * is nothing more to set up */
> + if (!keyboard)
> + return;
> +
> /* May be moved to focused list later by either
> * weston_keyboard_set_focus or directly if this client is already
> * focused */
> wl_list_insert(&keyboard->resource_list, wl_resource_get_link(cr));
> - wl_resource_set_implementation(cr, &keyboard_interface,
> - keyboard, unbind_resource);
>
> if (wl_resource_get_version(cr) >= WL_KEYBOARD_REPEAT_INFO_SINCE_VERSION) {
> wl_keyboard_send_repeat_info(cr,
> @@ -2587,12 +2595,9 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
> * This prevents a race between the compositor sending new
> * capabilities and the client trying to use the old ones.
> */
> - struct weston_touch *touch = seat->touch_state;
> + struct weston_touch *touch = seat ? seat->touch_state : NULL;
> struct wl_resource *cr;
>
> - if (!touch)
> - return;
> -
> cr = wl_resource_create(client, &wl_touch_interface,
> wl_resource_get_version(resource), id);
> if (cr == NULL) {
> @@ -2600,6 +2605,15 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
> return;
> }
>
> + wl_list_init(wl_resource_get_link(cr));
> + wl_resource_set_implementation(cr, &touch_interface,
> + touch, unbind_resource);
> +
> + /* If we don't have a touch_state, the resource is inert, so there
> + * is nothing more to set up */
> + if (!touch)
> + return;
> +
> if (touch->focus &&
> wl_resource_get_client(touch->focus->surface->resource) == client) {
> wl_list_insert(&touch->focus_resource_list,
> @@ -2608,8 +2622,6 @@ seat_get_touch(struct wl_client *client, struct wl_resource *resource,
> wl_list_insert(&touch->resource_list,
> wl_resource_get_link(cr));
> }
> - wl_resource_set_implementation(cr, &touch_interface,
> - touch, unbind_resource);
> }
>
> static void
> @@ -3087,6 +3099,19 @@ weston_seat_init(struct weston_seat *seat, struct weston_compositor *ec,
> WL_EXPORT void
> weston_seat_release(struct weston_seat *seat)
> {
> + struct wl_resource *resource;
> +
> + wl_resource_for_each(resource, &seat->base_resource_list) {
> + wl_resource_set_user_data(resource, NULL);
> + }
> +
> + wl_resource_for_each(resource, &seat->drag_resource_list) {
> + wl_resource_set_user_data(resource, NULL);
> + }
> +
> + wl_list_remove(&seat->base_resource_list);
> + wl_list_remove(&seat->drag_resource_list);
> +
> wl_list_remove(&seat->link);
>
> if (seat->saved_kbd_focus)
>
--
Quentin “Sardem FF7” Glidic
More information about the wayland-devel
mailing list