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 ?
On Thu, Nov 21, 2013 at 8:35 AM, Pekka Paalanen <[email protected]> wrote: > On Tue, 19 Nov 2013 15:44:44 +0100 > Lubomir Rintel <[email protected]> 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]> > > --- > > 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 > [email protected] > http://lists.freedesktop.org/mailman/listinfo/wayland-devel > -- Arnaud Vrac
_______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
