On Wed, 2013-12-04 at 11:54 -0800, Kristian Høgsberg wrote:
> On Wed, Dec 04, 2013 at 08:31:37AM +0100, Lubomir Rintel 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] [email protected]_pool(new id wl_shm_pool@23, fd 40, 1563540)
> > [1524827.315] [email protected]_buffer(new id wl_buffer@24, 0, 759,
> > 515, 3036, 0)
> > ...
> > [1524829.488] [email protected](wl_buffer@24, 0, 0)
> > [1524829.766] [email protected]_buffer_scale(1)
> > [1524829.904] [email protected](0, 0, 759, 515)
> > [1524830.248] [email protected](new id wl_callback@25)
> > [1524830.450] [email protected]()
> > ...
> > [1524846.706] [email protected]_pool(new id wl_shm_pool@26, fd 40, 1545000)
> > [1524847.215] [email protected]_buffer(new id wl_buffer@27, 0, 750,
> > 515, 3000, 0)
> > [1524847.735] [email protected]()
> > [1524847.953] -> [email protected]_id(24)
> > [1524848.144] [email protected]()
> > munmap(0xb5b2e000, 1563540) = 0
> > [1524849.021] -> [email protected]_id(23)
> > [1524849.425] [email protected](wl_buffer@27, 0, 0)
> > [1524849.730] [email protected]_buffer_scale(1)
> > [1524849.821] [email protected](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 <[email protected]>
> > ---
> > Thank you for your responses, Kristian & Pekka.
> > Hopefully it's correct now.
>
> Hi Lubomir,
>
> Sorry, but I think we have to do one more round...
>
> > 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
> > Changes to v4:
> > - Unhook the listener early when replacing surface's buffer
> >
> > src/pixman-renderer.c | 28 +++++++++++++++++++++++++++-
> > 1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c
> > index e854e2a..83a69de 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);
> > @@ -458,6 +475,9 @@ pixman_renderer_attach(struct weston_surface *es,
> > struct weston_buffer *buffer)
> >
> > weston_buffer_reference(&ps->buffer_ref, buffer);
> >
> > + if (ps->buffer_destroy_listener.notify)
> > + wl_list_remove(&ps->buffer_destroy_listener.link);
>
> If you want to use ps->buffer_destroy_listener.notify == NULL to
> indicate that there is no listener, you have to set that here. As it
> is, pixman_renderer_surface_state_destroy() or another attach with
> NULL will remove the listener again, causing list corruption.
>
> Better would be to instead use ps->image == NULL for this. That is,
> ps->buffer_destroy_listener is installed if and only if ps->image is
> non-NULL.
That is not actually true. It is possible for weston_surface to have
ps->image set, but no wl_buffer attached and thus no listener hooked --
weston uses that internally when doing fades.
Therefore I'll follow up with an updated patch implementing the first
option.
> Kristian
>
> > if (ps->image) {
> > pixman_image_unref(ps->image);
> > ps->image = NULL;
> > @@ -499,6 +519,11 @@ 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));
> > +
> > + ps->buffer_destroy_listener.notify =
> > + buffer_state_handle_buffer_destroy;
> > + wl_signal_add(&buffer->destroy_signal,
> > + &ps->buffer_destroy_listener);
> > }
> >
> > static void
> > @@ -506,7 +531,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;
> >
> > --
> > 1.8.4.2
> >
_______________________________________________
wayland-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
