[Mesa-dev] [PATCH] egl/wayland: do not leak wl_buffer when it is locked

[Daniel Stone]

Hi Juan,

On Thu, 2 Aug 2018 at 10:02, Juan A. Suarez Romero <jasuarez at igalia.com> wrote:
> If color buffer is locked, do not set its wayland buffer to NULL;
> otherwise it can not be freed later.

It can: see the 'if (i == ARRAY_SIZE(...))' branch inside wl_buffer_release.

> This also fixes dEQP-EGL.functional.swap_buffers_with_damage.* tests.

How does it fix them? Is it just leaks, or is there a functional difference?

> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -412,8 +412,11 @@ dri2_wl_release_buffers(struct dri2_egl_surface *dri2_surf)
>
>     for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
>        if (dri2_surf->color_buffers[i].wl_buffer &&
> -          !dri2_surf->color_buffers[i].locked)
> +          !dri2_surf->color_buffers[i].locked) {
>           wl_buffer_destroy(dri2_surf->color_buffers[i].wl_buffer);
> +         dri2_surf->color_buffers[i].wl_buffer = NULL;
> +      }
> +
>        if (dri2_surf->color_buffers[i].dri_image)
>           dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
>        if (dri2_surf->color_buffers[i].linear_copy)
> @@ -422,11 +425,9 @@ dri2_wl_release_buffers(struct dri2_egl_surface *dri2_surf)
>           munmap(dri2_surf->color_buffers[i].data,
>                  dri2_surf->color_buffers[i].data_size);
>
> -      dri2_surf->color_buffers[i].wl_buffer = NULL;
>        dri2_surf->color_buffers[i].dri_image = NULL;
>        dri2_surf->color_buffers[i].linear_copy = NULL;
>        dri2_surf->color_buffers[i].data = NULL;
> -      dri2_surf->color_buffers[i].locked = false;

So in this case, we still occupy a slot with (wl_buffer != NULL &&
locked == true) when we resize a surface when an old buffer is still
locked.

I _think_ the main functional difference is that the buffer is now
destroyed inside dri2_wl_destroy_surface() when the surface is
destroyed, even if it is locked. Destroying a surface with a locked
buffer is bad: calling wl_buffer_destroy() may provoke a fatal
protocol error, as destroying whilst still in use is illegal. On the
other hand, destroying the event queue without destroying the object
may cause a use-after-free later when the event does arrive, if the
connection is still alive (we should make that an assert really).

On the balance of things, I think destroying the buffer immediately
(running the risk of a protocol error), is less bad. It would be even
better if we could rely on clients to have destroyed their
surface-wrapper object (causing all buffers to be released) by the
time they destroy their EGLSurface, but that's of course never going
to happen. So, assuming there's no unexpected consequences I haven't
seen, this is:
Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel