Hi Lubomir, On Mon, 18 Nov 2013 23:42:40 +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. Should there be a destroy listener for the wl_buffer, that would destroy also the pixman image when the buffer goes away? > A more proper fix (without skipping rendering of the surface) would need a > change to Wayland API, so that the buffers are reference-counted in the same > way as pools are, so that they would not release their pulls if they are still > needed. Reference counting would not solve the whole issue. It would only solve the case where a client destroys a buffer and discards the storage, but not the case where the client immediately reuses the storage of a destroyed buffer. Keeping the buffer forcibly alive would possibly cause the compositor to read arbitrary data the client is writing for other purposes. Therefore once the client destroys a wl_buffer, the compositor cannot access the buffer's data anymore. > $ WAYLAND_DEBUG=1 strace -emunmap weston --backend=fbdev-backend.so > ... > [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> > So, does this mean that wl_buffer@24 was committed to the surface previously, and is still the current buffer on it? IOW, the client is destroying a wl_buffer that has not yet been released? If so, then yes, just skipping rendering that surface is ok, because the protocol specifies that if a client destroys a buffer that has not been released, the surface contents become undefined immediately (see wl_surface.attach). This case also includes a race, where the compositor could be reading the buffer contents and has not yet processed the destroy request, while the client has sent the destroy request and is already reusing and writing into the storage that was previously used for the buffer, leading to graphical corruption. In this case there is also another bug in gnome-terminal or GTK: a client should not destroy a wl_buffer before it is released. Instead, it needs to allocate a new one as needed. Also note, that while destroying a wl_buffer immediately after committing another buffer may seem to work in practice, it is not safe due to the race if the client is reusing the buffer storage. > Signed-off-by: Lubomir Rintel <lkund...@v3.sk> > --- > A Perl-based reproducer available here, in case it is more convenient to run > than gnome-terminal: http://v3.sk/~lkundrak/pixman-crash.pl Right, that example attempts demonstrate the undefined behaviour of destroying a buffer that is in use by the compositor, i.e. not released. I think it might be relying on a race, in that while the client is drawing the random pattern, the compositor processes the wl_buffer.destroy request and enters repaint... but I'm not sure, there would need to be a flush of the client send queue I guess... Thanks, pq > > src/pixman-renderer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/pixman-renderer.c b/src/pixman-renderer.c > index b719829..6759a00 100644 > --- a/src/pixman-renderer.c > +++ b/src/pixman-renderer.c > @@ -348,7 +348,7 @@ draw_view(struct weston_view *ev, struct weston_output > *output, > pixman_region32_t surface_blend; > > /* No buffer attached */ > - if (!ps->image) > + if (!ps->buffer_ref.buffer) > return; > > pixman_region32_init(&repaint); _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel