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

[Bryce Harrington]

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.

> 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"?

> +		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>

> -- 
> 2.7.3
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel