[PATCH weston v3] pixman: Destroy pixman images when underlying buffer is destroyed
Lubomir Rintel
lkundrak at v3.sk
Mon Nov 25 14:33:44 PST 2013
Hi Arnaud,
On Fri, 2013-11-22 at 15:48 +0100, Arnaud Vrac wrote:
> Wouldn't it be better to do this in compositor.c and call
> renderer->attach(surface, NULL) when the buffer attached to a surface
> is destroyed ?
I don't think that's possible. A buffer does not keep track of the
surface it's attached to -- it's not possible to determine the surface
in weston_buffer_destroy_handler().
> Is there a case where a renderer wouldn't want this to be done ?
>
>
> On Thu, Nov 21, 2013 at 8:35 AM, Pekka Paalanen <ppaalanen at gmail.com>
> wrote:
> On Tue, 19 Nov 2013 15:44:44 +0100
> Lubomir Rintel <lkundrak at v3.sk> wrote:
>
> > While the pixman image might be attached, the underlying
> buffer might be
> > already gone under certain circumstances. This is easily
> reproduced by
> > attempting to resize gnome-terminal on a fbdev backend.
> >
> > $ WAYLAND_DEBUG=1 strace -emunmap weston
> --backend=fbdev-backend.so
> > ...
> > [1524826.942] wl_shm at 7.create_pool(new id wl_shm_pool at 23, fd
> 40, 1563540)
> > [1524827.315] wl_shm_pool at 23.create_buffer(new id
> wl_buffer at 24, 0, 759, 515, 3036, 0)
> > ...
> > [1524829.488] wl_surface at 14.attach(wl_buffer at 24, 0, 0)
> > [1524829.766] wl_surface at 14.set_buffer_scale(1)
> > [1524829.904] wl_surface at 14.damage(0, 0, 759, 515)
> > [1524830.248] wl_surface at 14.frame(new id wl_callback at 25)
> > [1524830.450] wl_surface at 14.commit()
> > ...
> > [1524846.706] wl_shm at 7.create_pool(new id wl_shm_pool at 26, fd
> 40, 1545000)
> > [1524847.215] wl_shm_pool at 26.create_buffer(new id
> wl_buffer at 27, 0, 750, 515, 3000, 0)
> > [1524847.735] wl_buffer at 24.destroy()
> > [1524847.953] -> wl_display at 1.delete_id(24)
> > [1524848.144] wl_shm_pool at 23.destroy()
> > munmap(0xb5b2e000, 1563540) = 0
> > [1524849.021] -> wl_display at 1.delete_id(23)
> > [1524849.425] wl_surface at 14.attach(wl_buffer at 27, 0, 0)
> > [1524849.730] wl_surface at 14.set_buffer_scale(1)
> > [1524849.821] wl_surface at 14.damage(0, 0, 750, 515)
> > <No commit yet, so drawing is attempted from older buffer
> that used to be
> > attached to the surface, which happens to come from a
> destroyed pool,
> > resulting it an invalid read from address 0xb5b2e000>
> >
> > Signed-off-by: Lubomir Rintel <lkundrak at v3.sk>
> > ---
> > Changes to v2:
> > - Added listener instead of checking the buffer mapping,
> > as suggested by Pekka Paalanen
> > - Added more context to the commit message
> > Changes to v3:
> > - Removed unnecessary buffer unreferences and signal
> handler unhook
> > of itself after it was called, suggested by Pekka
> Paalanen
> > - Unhook the signal if we destroy the surface or reattach
> the buffer
> > to avoid circular attachment or call after the surface
> state is gone
> >
> > On Tue, 2013-11-19 at 14:23 +0200, Pekka Paalanen wrote:
> > > Otherwise looks good.
> > >
> > > We have the trade-off between less code (the first version
> of the
> > > patch), and not having a lingering ps->image referring to
> unusable
> > > memory (the second version of the patch). IMHO the latter
> is cleaner.
> >
> > I've found another issue; if a surface is removed, the
> destroy handler
> > will use-after-free the ps. Therefore I'm unhooking it once
> before ps
> > vanishes if it exists and only if it exists -- as the image
> data might
> > as well be a color fill without actual buffer attached.
> >
> > I found no better way of discovering whether it's
> appropriate to unhook
> > (buffer attached) or not (nothing attached or color-filled)
> that looking
> > at the notify callback pointer. It works well enough for me
> byt maybe
> > there's a cleaner solution?
>
>
> Right, looks like it should work. Another way would be to
> ensure, that
> buffer_destroy_listener.link is always removable. You can do
> that by
> initializing it with and always after remove calling
> wl_list_init() on
> it. That way wl_list_remove()'ing it is never illegal.
>
>
> Thanks,
> pq
>
> > src/pixman-renderer.c | 27 ++++++++++++++++++++++++++-
> > 1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
> > index b719829..c2c6ca9 100644
> > --- a/src/pixman-renderer.c
> > +++ b/src/pixman-renderer.c
> > @@ -42,6 +42,7 @@ struct pixman_surface_state {
> > pixman_image_t *image;
> > struct weston_buffer_reference buffer_ref;
> >
> > + struct wl_listener buffer_destroy_listener;
> > struct wl_listener surface_destroy_listener;
> > struct wl_listener renderer_destroy_listener;
> > };
> > @@ -450,6 +451,22 @@ pixman_renderer_flush_damage(struct
> weston_surface *surface)
> > }
> >
> > static void
> > +buffer_state_handle_buffer_destroy(struct wl_listener
> *listener, void *data)
> > +{
> > + struct pixman_surface_state *ps;
> > +
> > + ps = container_of(listener, struct
> pixman_surface_state,
> > + buffer_destroy_listener);
> > +
> > + if (ps->image) {
> > + pixman_image_unref(ps->image);
> > + ps->image = NULL;
> > + }
> > +
> > + ps->buffer_destroy_listener.notify = NULL;
> > +}
> > +
> > +static void
> > pixman_renderer_attach(struct weston_surface *es, struct
> weston_buffer *buffer)
> > {
> > struct pixman_surface_state *ps =
> get_surface_state(es);
> > @@ -499,6 +516,13 @@ pixman_renderer_attach(struct
> weston_surface *es, struct weston_buffer *buffer)
> > buffer->width, buffer->height,
> > wl_shm_buffer_get_data(shm_buffer),
> > wl_shm_buffer_get_stride(shm_buffer));
> > +
> > + if (ps->buffer_destroy_listener.notify)
> > +
> wl_list_remove(&ps->buffer_destroy_listener.link);
> > + ps->buffer_destroy_listener.notify =
> > + buffer_state_handle_buffer_destroy;
> > + wl_signal_add(&buffer->destroy_signal,
> > + &ps->buffer_destroy_listener);
> > }
> >
> > static void
> > @@ -506,7 +530,8 @@
> pixman_renderer_surface_state_destroy(struct
> pixman_surface_state *ps)
> > {
> > wl_list_remove(&ps->surface_destroy_listener.link);
> > wl_list_remove(&ps->renderer_destroy_listener.link);
> > -
> > + if (ps->buffer_destroy_listener.notify)
> > +
> wl_list_remove(&ps->buffer_destroy_listener.link);
> >
> > ps->surface->renderer_state = NULL;
> >
>
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
>
>
>
> --
> Arnaud Vrac
More information about the wayland-devel
mailing list