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

[Jonas Ådahl]

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.

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