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

[Juan A. Suarez Romero]

On Wed, 2018-08-08 at 18:39 +0200, Juan A. Suarez Romero wrote:
> 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 :)

I mean, I explained it wrongly :D


> 
> 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
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev