On Tue, 19 Nov 2013 12:57:10 +0100 Lubomir Rintel <lkund...@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@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 > > src/pixman-renderer.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c > index b719829..eed1fc2 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,24 @@ 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); > + > + weston_buffer_reference(&ps->buffer_ref, NULL); You should not need weston_buffer_reference() here, as it internally already uses a buffer destroy listener. > + > + if (ps->image) { > + pixman_image_unref(ps->image); > + ps->image = NULL; > + } > + > + wl_list_remove(&ps->buffer_destroy_listener.link); Destroy listeners do not need to remove themselves, the list will evaporate anyway. > +} > + > +static void > pixman_renderer_attach(struct weston_surface *es, struct weston_buffer > *buffer) > { > struct pixman_surface_state *ps = get_surface_state(es); > @@ -459,6 +478,7 @@ pixman_renderer_attach(struct weston_surface *es, struct > weston_buffer *buffer) > weston_buffer_reference(&ps->buffer_ref, buffer); > > if (ps->image) { > + wl_list_remove(&ps->buffer_destroy_listener.link); > 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 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. Thanks, pq _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel