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

[Juan A. Suarez Romero]

On Wed, 2018-08-08 at 17:21 +0100, Daniel Stone wrote:
> 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.

I think I didn't explain wrongly :)

If color buffer is locked, we set color_buffer.wl_buffer to NULL, and thus we
can't free wl_buffer later.

> 
> > 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?

Besides this wl_buffer leak, the tests crashes. Found out that fixing the leak
the crash goes away.

> 
> > --- 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>
> 

I understand this patch does not require any further work, right? Except
clarifying the commit messsage as explaining above.


> Cheers,
> Daniel
>