On Tue, Apr 26, 2016 at 03:51:05PM +0300, Pekka Paalanen wrote: > From: Pekka Paalanen <pekka.paala...@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.paala...@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 <br...@osg.samsung.com> > -- > 2.7.3 > > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel