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.frant...@collabora.com>
Reviewed-by: Pekka Paalanen <pekka.paala...@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+...@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
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
