[PATCH] pixman: Check whether the buffer still exists when the surface is redrawn

[Pekka Paalanen]

Hi Lubomir,

On Mon, 18 Nov 2013 23:42:40 +0100
Lubomir Rintel <lkundrak at 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 at 7.create_pool(new id wl_shm_pool at 26, fd 40, 1545000)
> [1524847.215] wl_shm_pool at 26.create_buffer(new id wl_buffer at 27, 0, 750, 515, 3000, 0)
> [1524847.735] wl_buffer at 24.destroy()
> [1524847.953]  -> wl_display at 1.delete_id(24)
> [1524848.144] wl_shm_pool at 23.destroy()
> munmap(0xb5b2e000, 1563540)             = 0
> [1524849.021]  -> wl_display at 1.delete_id(23)
> [1524849.425] wl_surface at 14.attach(wl_buffer at 27, 0, 0)
> [1524849.730] wl_surface at 14.set_buffer_scale(1)
> [1524849.821] wl_surface at 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 at 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 <lkundrak at 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);