[PATCH weston v3 13/17] compositor: fix wp_viewport use after free

[Pekka Paalanen]

On Wed, 4 May 2016 17:57:59 -0700
Bryce Harrington <bryce at osg.samsung.com> wrote:

> On Tue, Apr 26, 2016 at 03:51:05PM +0300, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > 
> > If a client destroyed the wl_surface before the wp_viewport, Weston core
> > would access freed memory, because the weston_surface pointer stored in
> > the wp_viewport wl_resource's user data was stale.
> > 
> > Fix this by setting the user data to NULL on wl_surface destruction. It
> > specifically wl_surface and not weston_surface destruction, because this  
> 
> Think we're missing a word or two somewhere in the beginning of that
> sentence.

Woops, yeah.

> > is about client-visible behaviour. Something internal might keep
> > weston_surface alive past the wl_surface.
> > 
> > Add checks to all wp_viewport request handlers.
> > 
> > At the same time, implement the new error conditions in wp_viewport:
> > calling any request except destroy must result in a protocol error.
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> > ---
> >  src/compositor.c | 25 +++++++++++++++++++++++--
> >  1 file changed, 23 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/compositor.c b/src/compositor.c
> > index d462409..1d605c7 100644
> > --- a/src/compositor.c
> > +++ b/src/compositor.c
> > @@ -1986,6 +1986,10 @@ destroy_surface(struct wl_resource *resource)
> >  	 * dangling pointer if the surface was refcounted and survives
> >  	 * the weston_surface_destroy() call. */
> >  	surface->resource = NULL;
> > +
> > +	if (surface->viewport_resource)
> > +		wl_resource_set_user_data(surface->viewport_resource, NULL);
> > +
> >  	weston_surface_destroy(surface);
> >  }
> >  
> > @@ -4351,6 +4355,9 @@ destroy_viewport(struct wl_resource *resource)
> >  	struct weston_surface *surface =
> >  		wl_resource_get_user_data(resource);
> >  
> > +	if (!surface)
> > +		return;
> > +
> >  	surface->viewport_resource = NULL;
> >  	surface->pending.buffer_viewport.buffer.src_width =
> >  		wl_fixed_from_int(-1);
> > @@ -4376,7 +4383,14 @@ viewport_set_source(struct wl_client *client,
> >  	struct weston_surface *surface =
> >  		wl_resource_get_user_data(resource);
> >  
> > -	assert(surface->viewport_resource != NULL);
> > +	if (!surface) {
> > +		wl_resource_post_error(resource,
> > +			WP_VIEWPORT_ERROR_NO_SURFACE,
> > +			"wl_surface for this viewport is gone");  
> 
> "gone" seems a bit informal.  Perhaps it should say "is no longer valid"?

I tried to keep it short for the line length. I'll go with "no longer
exists".

> 
> > +		return;
> > +	}
> > +
> > +	assert(surface->viewport_resource == resource);
> >  
> >  	if (src_width == wl_fixed_from_int(-1) &&
> >  	    src_height == wl_fixed_from_int(-1)) {
> > @@ -4412,7 +4426,14 @@ viewport_set_destination(struct wl_client *client,
> >  	struct weston_surface *surface =
> >  		wl_resource_get_user_data(resource);
> >  
> > -	assert(surface->viewport_resource != NULL);
> > +	if (!surface) {
> > +		wl_resource_post_error(resource,
> > +			WP_VIEWPORT_ERROR_NO_SURFACE,
> > +			"wl_surface for this viewport is gone");  
> 
> ditto here
> 
> > +		return;
> > +	}
> > +
> > +	assert(surface->viewport_resource == resource);
> >  
> >  	if (dst_width == -1 && dst_height == -1) {
> >  		/* unset destination size */  
> 
> With those bits fixed,
> 
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>

Done, will be re-sent when it's time.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160506/c1b2b165/attachment-0001.sig>