[PATCH weston] xdg-shell: Add NULL checks for resources that may have gone away.
Pekka Paalanen
ppaalanen at gmail.com
Mon Dec 5 11:12:25 UTC 2016
On Sat, 3 Dec 2016 21:21:44 -0800
Dima Ryazanov <dima at gmail.com> wrote:
> This should fix some (fairly rare) crashes where the client goes away at exactly
> the wrong moment, e.g.:
> - Weston sends a message to the client
> - Message causes the client to crash for whatever reason
> - Weston keeps processing the remaining requests from the now dead client
Hi,
I'm curious. Even if the client crashes, this should happen:
1. the server processes queued requests while it is not aware of the
client going away
2. libwayland-server notices the client has gone away, and calls
destructors on all objects
During step 1, the client being already dead does not affect anything
because the disconnection has not been processed, so any user data
still remains valid.
Once the destruction begins, no requests should dispatch anymore. If
some requests are still being dispatched, that would be a bug in
libwayland-server.
I'd really like to know how it is possible to have these functions
called with invalid user data. I fear that this patch might just paper
over some real bugs.
Resources do not just go away, especially those that represent objects
created by the client. I can understand that wl_output user data can be
NULL if the weston_output has been destroyed, but not with e.g.
wl_surface since the surface is created by the client, so it cannot go
away without the client destroying it or disconnecting.
Could there perhaps be some object destruction sequence that e.g.
libweston-desktop does not handle? Is that what is going on here?
Does the protocol define a destruction order, should we be sending
protocol errors somewhere?
> (I've only seen it happen once, while stepping through Weston's code in gdb, but
> seems like the right thing to fix.)
Stepping in gdb only makes Weston extremely slow to execute, so it can
expose races or trigger "unresponsive client" etc. timeouts, but it
does not affect steps 1 and 2 mentioned above. So if there is a race,
we need to fix the race instead.
Hence, I'm not yet convinced this patch is the right thing to do.
On the contrary, without any other explanation I would expect the checks
you added to be asserts instead of silently ignoring the unexpected.
Thanks,
pq
> Signed-off-by: Dima Ryazanov <dima at gmail.com>
> ---
> libweston-desktop/xdg-shell-v6.c | 141 ++++++++++++++++++++++++++++-----------
> 1 file changed, 101 insertions(+), 40 deletions(-)
>
> diff --git a/libweston-desktop/xdg-shell-v6.c b/libweston-desktop/xdg-shell-v6.c
> index 7d0bd8e..30f6d82 100644
> --- a/libweston-desktop/xdg-shell-v6.c
> +++ b/libweston-desktop/xdg-shell-v6.c
> @@ -302,10 +302,14 @@ weston_desktop_xdg_toplevel_protocol_set_parent(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> struct weston_desktop_surface *parent = NULL;
>
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> +
> if (parent_resource != NULL)
> parent = wl_resource_get_user_data(parent_resource);
>
> @@ -346,8 +350,12 @@ weston_desktop_xdg_toplevel_protocol_show_window_menu(struct wl_client *wl_clien
> wl_resource_get_user_data(resource);
> struct weston_seat *seat =
> wl_resource_get_user_data(seat_resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> +
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
>
> if (!toplevel->base.configured) {
> wl_resource_post_error(toplevel->resource,
> @@ -370,8 +378,12 @@ weston_desktop_xdg_toplevel_protocol_move(struct wl_client *wl_client,
> wl_resource_get_user_data(resource);
> struct weston_seat *seat =
> wl_resource_get_user_data(seat_resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> +
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
>
> if (!toplevel->base.configured) {
> wl_resource_post_error(toplevel->resource,
> @@ -394,11 +406,15 @@ weston_desktop_xdg_toplevel_protocol_resize(struct wl_client *wl_client,
> wl_resource_get_user_data(resource);
> struct weston_seat *seat =
> wl_resource_get_user_data(seat_resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> enum weston_desktop_surface_edge surf_edges =
> (enum weston_desktop_surface_edge) edges;
>
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> +
> if (!toplevel->base.configured) {
> wl_resource_post_error(toplevel->resource,
> ZXDG_SURFACE_V6_ERROR_NOT_CONSTRUCTED,
> @@ -423,9 +439,12 @@ weston_desktop_xdg_toplevel_protocol_set_min_size(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
>
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> toplevel->next_min_size.width = width;
> toplevel->next_min_size.height = height;
> }
> @@ -437,9 +456,12 @@ weston_desktop_xdg_toplevel_protocol_set_max_size(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> +
> + if (!dsurface)
> + return;
>
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> toplevel->next_max_size.width = width;
> toplevel->next_max_size.height = height;
> }
> @@ -450,9 +472,12 @@ weston_desktop_xdg_toplevel_protocol_set_maximized(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
>
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> weston_desktop_xdg_toplevel_ensure_added(toplevel);
> weston_desktop_api_maximized_requested(toplevel->base.desktop, dsurface, true);
> }
> @@ -463,9 +488,12 @@ weston_desktop_xdg_toplevel_protocol_unset_maximized(struct wl_client *wl_client
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> +
> + if (!dsurface)
> + return;
>
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> weston_desktop_xdg_toplevel_ensure_added(toplevel);
> weston_desktop_api_maximized_requested(toplevel->base.desktop, dsurface, false);
> }
> @@ -477,10 +505,14 @@ weston_desktop_xdg_toplevel_protocol_set_fullscreen(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> struct weston_output *output = NULL;
>
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> +
> if (output_resource != NULL)
> output = wl_resource_get_user_data(output_resource);
>
> @@ -495,9 +527,12 @@ weston_desktop_xdg_toplevel_protocol_unset_fullscreen(struct wl_client *wl_clien
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
> +
> + if (!dsurface)
> + return;
>
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> weston_desktop_xdg_toplevel_ensure_added(toplevel);
> weston_desktop_api_fullscreen_requested(toplevel->base.desktop, dsurface,
> false, NULL);
> @@ -509,9 +544,12 @@ weston_desktop_xdg_toplevel_protocol_set_minimized(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_toplevel *toplevel;
>
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> weston_desktop_xdg_toplevel_ensure_added(toplevel);
> weston_desktop_api_minimized_requested(toplevel->base.desktop, dsurface);
> }
> @@ -737,12 +775,19 @@ weston_desktop_xdg_popup_protocol_grab(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_popup *popup =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_popup *popup;
> struct weston_seat *wseat = wl_resource_get_user_data(seat_resource);
> - struct weston_desktop_seat *seat = weston_desktop_seat_from_seat(wseat);
> + struct weston_desktop_seat *seat;
> struct weston_desktop_surface *topmost;
> - bool parent_is_toplevel =
> + bool parent_is_toplevel;
> +
> + if (!dsurface || !wseat)
> + return;
> +
> + seat = weston_desktop_seat_from_seat(wseat);
> + popup = weston_desktop_surface_get_implementation_data(dsurface);
> +
> + parent_is_toplevel =
> popup->parent->role == WESTON_DESKTOP_XDG_SURFACE_ROLE_TOPLEVEL;
>
> if (popup->committed) {
> @@ -891,10 +936,14 @@ weston_desktop_xdg_surface_protocol_get_toplevel(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_surface *wsurface =
> - weston_desktop_surface_get_surface(dsurface);
> - struct weston_desktop_xdg_toplevel *toplevel =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_surface *wsurface;
> + struct weston_desktop_xdg_toplevel *toplevel;
> +
> + if (!dsurface)
> + return;
> +
> + toplevel = weston_desktop_surface_get_implementation_data(dsurface);
> + wsurface = weston_desktop_surface_get_surface(dsurface);
>
> if (weston_surface_set_role(wsurface, weston_desktop_xdg_toplevel_role,
> resource, ZXDG_SHELL_V6_ERROR_ROLE) < 0)
> @@ -920,17 +969,21 @@ weston_desktop_xdg_surface_protocol_get_popup(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_surface *wsurface =
> - weston_desktop_surface_get_surface(dsurface);
> - struct weston_desktop_xdg_popup *popup =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_surface *wsurface;
> + struct weston_desktop_xdg_popup *popup;
> struct weston_desktop_surface *parent_surface =
> wl_resource_get_user_data(parent_resource);
> - struct weston_desktop_xdg_surface *parent =
> - weston_desktop_surface_get_implementation_data(parent_surface);
> + struct weston_desktop_xdg_surface *parent;
> struct weston_desktop_xdg_positioner *positioner =
> wl_resource_get_user_data(positioner_resource);
>
> + if (!dsurface || !parent_surface || !positioner)
> + return;
> +
> + wsurface = weston_desktop_surface_get_surface(dsurface);
> + popup = weston_desktop_surface_get_implementation_data(dsurface);
> + parent = weston_desktop_surface_get_implementation_data(parent_surface);
> +
> /* Checking whether the size and anchor rect both have a positive size
> * is enough to verify both have been correctly set */
> if (positioner->size.width == 0 || positioner->anchor_rect.width == 0) {
> @@ -994,8 +1047,12 @@ weston_desktop_xdg_surface_protocol_set_window_geometry(struct wl_client *wl_cli
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_surface *surface =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_surface *surface;
> +
> + if (!dsurface)
> + return;
> +
> + surface = weston_desktop_surface_get_implementation_data(dsurface);
>
> if (!weston_desktop_xdg_surface_check_role(surface))
> return;
> @@ -1014,8 +1071,12 @@ weston_desktop_xdg_surface_protocol_ack_configure(struct wl_client *wl_client,
> {
> struct weston_desktop_surface *dsurface =
> wl_resource_get_user_data(resource);
> - struct weston_desktop_xdg_surface *surface =
> - weston_desktop_surface_get_implementation_data(dsurface);
> + struct weston_desktop_xdg_surface *surface;
> +
> + if (!dsurface)
> + return;
> +
> + surface = weston_desktop_surface_get_implementation_data(dsurface);
>
> if (!weston_desktop_xdg_surface_check_role(surface))
> return;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161205/e895f578/attachment-0001.sig>
More information about the wayland-devel
mailing list