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 <lkund...@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?
Thanks,
Lubo
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;
--
1.8.4.2
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
