Re: [PATCH weston] xdg-shell: Add NULL checks for resources that may have gone away.

[Pekka Paalanen]

On Sat,  3 Dec 2016 21:21:44 -0800
Dima Ryazanov  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 
> ---
>  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 =
> - 

[PATCH weston] xdg-shell: Add NULL checks for resources that may have gone away.

[Dima Ryazanov]

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

(I've only seen it happen once, while stepping through Weston's code in gdb, but
seems like the right thing to fix.)

Signed-off-by: Dima Ryazanov 
---
 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 =