[PATCH weston] xdg-shell: Add NULL checks for resources that may have gone away.
Dima Ryazanov
dima at gmail.com
Tue Dec 6 10:26:44 UTC 2016
Hi Pekka,
Thanks for the explanation. You're probably right; this shouldn't be
possible.
I haven't been able to reproduce the bug again. The one time I got it was
when I was working on my show_window_menu patches - so it's quite possible
that something there was causing it. (In fact, the window menu involves two
different clients - the owner of the window and the desktop shell - so
maybe that was the problem.)
Anyways, let's ignore this patch unless I figure out what actually happened
there.
On Mon, Dec 5, 2016 at 3:12 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> 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 --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161206/6b40b079/attachment-0001.html>
More information about the wayland-devel
mailing list