<div dir="ltr">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 ? Is there a case where a renderer wouldn't want this to be done ?</div> <div class="gmail_extra"> <div class="gmail_quote">On Thu, Nov 21, 2013 at 8:35 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, 19 Nov 2013 15:44:44 +0100 Lubomir Rintel <<a href="mailto:lkundrak@v3.sk">lkundrak@v3.sk</a>> 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@7.create_pool(new id wl_shm_pool@23, fd 40, 1563540) > [1524827.315] wl_shm_pool@23.create_buffer(new id wl_buffer@24, 0, 759, 515, 3036, 0) > ... > [1524829.488] wl_surface@14.attach(wl_buffer@24, 0, 0) > [1524829.766] wl_surface@14.set_buffer_scale(1) > [1524829.904] wl_surface@14.damage(0, 0, 759, 515) > [1524830.248] wl_surface@14.frame(new id wl_callback@25) > [1524830.450] wl_surface@14.commit() > ... > [1524846.706] wl_shm@7.create_pool(new id wl_shm_pool@26, fd 40, 1545000) > [1524847.215] wl_shm_pool@26.create_buffer(new id wl_buffer@27, 0, 750, 515, 3000, 0) > [1524847.735] wl_buffer@24.destroy() > [1524847.953] -> wl_display@1.delete_id(24) > [1524848.144] wl_shm_pool@23.destroy() > munmap(0xb5b2e000, 1563540) = 0 > [1524849.021] -> wl_display@1.delete_id(23) > [1524849.425] wl_surface@14.attach(wl_buffer@27, 0, 0) > [1524849.730] wl_surface@14.set_buffer_scale(1) > [1524849.821] wl_surface@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 <<a href="mailto:lkundrak@v3.sk">lkundrak@v3.sk</a>> > --- > 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? </div></div>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 <div class="HOEnZb"><div class="h5"> > 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; > </div></div><div class="HOEnZb"><div class="h5">_______________________________________________ wayland-devel mailing list <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> </div></div></blockquote></div> <div> </div>-- Arnaud Vrac </div>