On Fri, 26 Jan 2018 18:47:58 +0200 Alexandros Frantzis <[email protected]> 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. > > Signed-off-by: Alexandros Frantzis <[email protected]> > --- > libweston/input.c | 45 +++++++++++++++++++++++++++++++++++---------- > 1 file changed, 35 insertions(+), 10 deletions(-) > > diff --git a/libweston/input.c b/libweston/input.c > index 48bcc55c..e4daa56b 100644 > --- a/libweston/input.c > +++ b/libweston/input.c > @@ -2412,6 +2412,13 @@ seat_get_pointer(struct wl_client *client, struct > wl_resource *resource, > uint32_t id) > { > struct weston_seat *seat = wl_resource_get_user_data(resource); > + struct weston_pointer *pointer; > + struct wl_resource *cr; > + struct weston_pointer_client *pointer_client; > + > + if (!seat) > + return; > + > /* We use the pointer_state directly, which means we'll > * give a wl_pointer if the seat has ever had one - even though > * the spec explicitly states that this request only takes effect > @@ -2420,10 +2427,7 @@ 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 wl_resource *cr; > - struct weston_pointer_client *pointer_client; > - > + pointer = seat->pointer_state; > if (!pointer) > return; > > @@ -2499,6 +2503,12 @@ seat_get_keyboard(struct wl_client *client, struct > wl_resource *resource, > uint32_t id) > { > struct weston_seat *seat = wl_resource_get_user_data(resource); > + struct weston_keyboard *keyboard; > + struct wl_resource *cr; > + > + if (!seat) > + return; > + > /* We use the keyboard_state directly, which means we'll > * give a wl_keyboard if the seat has ever had one - even though > * the spec explicitly states that this request only takes effect > @@ -2507,9 +2517,7 @@ 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 wl_resource *cr; > - > + keyboard = seat->keyboard_state; > if (!keyboard) > return; > > @@ -2579,6 +2587,12 @@ seat_get_touch(struct wl_client *client, struct > wl_resource *resource, > uint32_t id) > { > struct weston_seat *seat = wl_resource_get_user_data(resource); > + struct weston_touch *touch; > + struct wl_resource *cr; > + > + if (!seat) > + return; > + > /* We use the touch_state directly, which means we'll > * give a wl_touch if the seat has ever had one - even though > * the spec explicitly states that this request only takes effect > @@ -2587,9 +2601,7 @@ 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 wl_resource *cr; > - > + touch = seat->touch_state; > if (!touch) > return; Hi, all the seat_get_*() changes have the same problem that they skip calling wl_resource_create() which will lead to protocol state mismatch. These functions need to create inert wl_resources, but thankfully the earlier patches already take care of handling them further. > @@ -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); > + } Other requests which take wl_seat as argument are: - wl_data_device_manager.get_data_device - wl_shell_surface.move - wl_shell_surface.resize - wl_shell_surface.set_popup But there are even more in wayland-protocols, you can find them with $ git grep 'interface="wl_seat"' These are unlikely to cope with an inert wl_seat. Patching weston_desktop_seat_from_seat() will probably take care of a lot. > + > + 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) Setting NULL with the drag_resource_list needs more changes in data-device.c. Looks like the user data of wl_data_device resource is the weston_seat, so all uses of wl_data_device needs to be audited. That's all the functions in data_device_interface. Luckily none of the wl_data_device requests create new protocol objects, and wl_data_device is never used as a request argument. Thanks, pq
pgprcXZVeMSG_.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
