[PATCH] shell: Check more thoroughly for undefined surface resource

[Bryce Harrington]

On Tue, Sep 15, 2015 at 08:31:56AM +0800, Jonas Ådahl wrote:
> On Mon, Sep 14, 2015 at 01:15:47PM -0700, Bryce Harrington wrote:
> > On Mon, Sep 14, 2015 at 12:31:10PM -0700, Bryce Harrington wrote:
> > > The surface can have an undefined resource in certain situations (such
> > > as with xwayland).  So, since NULL is a valid state for this parameter,
> > > and since the wl_resource_*, etc. calls require their parameters to be
> > > non-NULL, make a practice of always checking the surface resource before
> > > making wayland calls.
> > > 
> > > Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> > 
> > Some of these checks I wasn't sure if maybe they should instead be
> > asserts().  E.g. the grab stuff may be predicated on there being a
> > defined resource, so asserts may be more appropriate there, but I wasn't
> > sure.  I can respin the patch with those if needed.
> 
> IMHO we should not have any unnecessary NULL checks. If there is a NULL
> check I would expect that it is a known possible and allowed situation.

As I mentioned, NULL appears to be a known possible and allowed value
for shsurf->resource here.
 
> But the OOM checks makes sense of course.
> 
> 
> Jonas
> 
> > 
> > Bryce
> > 
> > > ---
> > >  desktop-shell/shell.c | 37 ++++++++++++++++++++++++++++++-------
> > >  1 file changed, 30 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > > index ecc42c5..0b91ce6 100644
> > > --- a/desktop-shell/shell.c
> > > +++ b/desktop-shell/shell.c
> > > @@ -2148,7 +2148,7 @@ xdg_ping_timeout_handler(void *data)
> > >  			continue;
> > >  
> > >  		shsurf = get_shell_surface(pointer->focus->surface);
> > > -		if (shsurf &&
> > > +		if (shsurf && grab->shsurf->resource &&
> > >  		    wl_resource_get_client(shsurf->resource) == sc->client)
> > >  			set_busy_cursor(shsurf, pointer);
> > >  	}
> > > @@ -2183,7 +2183,8 @@ handle_xdg_ping(struct shell_surface *shsurf, uint32_t serial)
> > >  	if (shell_surface_is_xdg_surface(shsurf) ||
> > >  	    shell_surface_is_xdg_popup(shsurf))
> > >  		xdg_shell_send_ping(sc->resource, serial);
> > > -	else if (shell_surface_is_wl_shell_surface(shsurf))
> > > +	else if (shell_surface_is_wl_shell_surface(shsurf)
> > > +		 && grab->shsurf->resource)
> > >  		wl_shell_surface_send_ping(shsurf->resource, serial);
> > >  }
> > >  
> > > @@ -3372,7 +3373,9 @@ static const struct weston_touch_grab_interface touch_popup_grab_interface = {
> > >  static void
> > >  shell_surface_send_popup_done(struct shell_surface *shsurf)
> > >  {
> > > -	if (shell_surface_is_wl_shell_surface(shsurf))
> > > +	if (grab->shsurf->resource == NULL)
> > > +		return;
> > > +	else if (shell_surface_is_wl_shell_surface(shsurf))
> > >  		wl_shell_surface_send_popup_done(shsurf->resource);
> > >  	else if (shell_surface_is_xdg_popup(shsurf))
> > >  		xdg_popup_send_popup_done(shsurf->resource);
> > > @@ -3455,6 +3458,12 @@ add_popup_grab(struct shell_surface *shsurf,
> > >  	struct weston_pointer *pointer = weston_seat_get_pointer(seat);
> > >  	struct weston_touch *touch = weston_seat_get_touch(seat);
> > >  
> > > +	if (shsurf->resource == NULL) {
> > > +		wl_resource_post_error(shsurf->owner_resource,
> > > +				       WL_DISPLAY_ERROR_INVALID_OBJECT,
> > > +				       "undefined resource when grabbing");
> > > +		return -1;
> > > +	}
> > >  	parent = get_shell_surface(shsurf->parent);
> > >  	top_surface = get_top_popup(shseat);
> > >  	if (shell_surface_is_xdg_popup(shsurf) &&
> > > @@ -3619,7 +3628,8 @@ shell_destroy_shell_surface(struct wl_resource *resource)
> > >  
> > >  	if (!wl_list_empty(&shsurf->popup.grab_link))
> > >  		remove_popup_grab(shsurf);
> > > -	wl_list_remove(wl_resource_get_link(shsurf->resource));
> > > +	if (shsurf->resource)
> > > +		wl_list_remove(wl_resource_get_link(shsurf->resource));
> > >  	shsurf->resource = NULL;
> > >  }
> > >  
> > > @@ -3803,6 +3813,10 @@ shell_get_shell_surface(struct wl_client *client,
> > >  	shsurf->resource =
> > >  		wl_resource_create(client,
> > >  				   &wl_shell_surface_interface, 1, id);
> > > +	if (!shsurf->resource) {
> > > +		wl_resource_post_no_memory(surface_resource);
> > > +	        return;
> > > +	}
> > >  	wl_resource_set_implementation(shsurf->resource,
> > >  				       &shell_surface_implementation,
> > >  				       shsurf, shell_destroy_shell_surface);
> > > @@ -4150,6 +4164,10 @@ xdg_get_xdg_surface(struct wl_client *client,
> > >  	shsurf->resource =
> > >  		wl_resource_create(client,
> > >  				   &xdg_surface_interface, 1, id);
> > > +	if (!shsurf->resource) {
> > > +		wl_resource_post_no_memory(surface_resource);
> > > +		return;
> > > +	}
> > >  	wl_resource_set_implementation(shsurf->resource,
> > >  				       &xdg_surface_implementation,
> > >  				       shsurf, shell_destroy_shell_surface);
> > > @@ -4279,6 +4297,10 @@ xdg_get_xdg_popup(struct wl_client *client,
> > >  	shsurf->resource =
> > >  		wl_resource_create(client,
> > >  				   &xdg_popup_interface, 1, id);
> > > +	if (!shsurf->resource) {
> > > +		wl_resource_post_no_memory(surface_resource);
> > > +		return;
> > > +	}
> > >  	wl_resource_set_implementation(shsurf->resource,
> > >  				       &xdg_popup_implementation,
> > >  				       shsurf, shell_destroy_shell_surface);
> > > @@ -4299,9 +4321,10 @@ xdg_pong(struct wl_client *client,
> > >  static bool
> > >  shell_surface_is_xdg_popup(struct shell_surface *shsurf)
> > >  {
> > > -	return wl_resource_instance_of(shsurf->resource,
> > > -				       &xdg_popup_interface,
> > > -				       &xdg_popup_implementation);
> > > +	return (shsurf->resource &&
> > > +		wl_resource_instance_of(shsurf->resource,
> > > +					&xdg_popup_interface,
> > > +					&xdg_popup_implementation));
> > >  }
> > >  
> > >  static const struct xdg_shell_interface xdg_implementation = {
> > > -- 
> > > 1.9.1
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel