[Mesa-stable] [PATCH] egl/dri2: dri2_make_current: Make sure display is initialized before using it

[Emil Velikov]

On 18 July 2016 at 04:19, Nicolas Boichat <drinkcat at chromium.org> wrote:
> On Fri, Jul 15, 2016 at 9:03 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 15 July 2016 at 09:28, Nicolas Boichat <drinkcat at chromium.org> wrote:
>>> android.opengl.cts.WrapperTest#testGetIntegerv1 CTS test calls
>>> eglTerminate, followed by eglReleaseThread. In that case, the
>>> display will not be initialized when eglReleaseThread calls
>>> MakeCurrent with NULL parameters, to unbind the context, which
>>> causes a a segfault in drv->API.MakeCurrent (dri2_make_current),
>>> either in glFlush or in a latter call.
>>>
>>> A similar case is observed in this bug:
>>> https://bugs.freedesktop.org/show_bug.cgi?id=69622, where the test
>>> call eglTerminate, then eglMakeCurrent(dpy, NULL, NULL, NULL).
>>>
>>> This patch:
>>>  1. Validates that the display is initialized, if ctx/dsurf/rsurf
>>>     are not all NULL.
>>>  2. Does not call glFlush/unBindContext is there is no display.
>>>  3. However, it still goes through the normal path as
>>>     drv->API.DestroyContext decrements the reference count on the
>>>     context, and frees the structure.
>>>
>>> Cc: "11.2 12.0" <mesa-stable at lists.freedesktop.org>
>>> Signed-off-by: Nicolas Boichat <drinkcat at chromium.org>
>>> ---
>>>
>>> I did not actually test the case reported on the bug, only the CTS
>>> test, would be nice if someone can try it out (Rhys? Chad?).
>>>
>>> Applies after "egl/dri2: dri2_make_current: Set EGL error if bindContext
>>> fails", but wanted to keep the 2 patches separate as they are different
>>> problems and discussions.
>>>
>>>  src/egl/drivers/dri2/egl_dri2.c | 13 ++++++++++---
>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
>>> index 2e97d85..3269e52 100644
>>> --- a/src/egl/drivers/dri2/egl_dri2.c
>>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>>> @@ -1166,7 +1166,8 @@ dri2_destroy_context(_EGLDriver *drv, _EGLDisplay *disp, _EGLContext *ctx)
>>>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
>>>
>>>     if (_eglPutContext(ctx)) {
>>> -      dri2_dpy->core->destroyContext(dri2_ctx->dri_context);
>>> +      if (dri2_dpy)
>> Not sure if this is the right place/we need this.
>>
>> When calling eglDestroyContext (after eglTerminate or not) this cannot
>> happen since the _EGL_CHECK_CONTEXT macro will dive into the _eglCheck
>> functions and will bail out since the since the dpy is no initialized.
>>
>> The only case we can reach this is from dri2_make_current. See below for more.
>
> Yes, that's correct.
>
>>> @@ -1189,6 +1190,10 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf,
>>>     __DRIdrawable *ddraw, *rdraw;
>>>     __DRIcontext *cctx;
>>>
>>> +   /* Check that the display is initialized */
>>> +   if ((ctx != NULL || dsurf != NULL || rsurf != NULL) && !dri2_dpy)
>>> +      return _eglError(EGL_NOT_INITIALIZED, "eglMakeCurrent error");
>>> +
>> Was doing to say "the first few lines in eglMakeCurrent) should
>> already handle these" only to realise that dri2_dpy doesn't wrap
>> around/subclass the respective _EGLfoo type (_EGLDisplay), like the
>> dri2_egl_{surface,context...} :-\
>>
>> At the same time, dri2_make_current cannot (should not really) do
>> anything in the !dri2_dpy case, since we cannot:
>>  - call unbind the old ctx bind the new one, or even restore/rebind
>> the original ctx
>>  - free any of the ctx/surf resources. The latter should already be
>> gone, as upon eglTerminate walk over the ctx/surf lists and tear them
>> all down (see _eglReleaseDisplayResources).
>>
>> Thus something like the following should be fine imho.
>>
>> if (!dri2_dpy)
>>    /* copy/reword some my earlier comment in here */
>>    return 0;
>
> Yes, that was my first approach, but we'd still leak the current dri2_ctx.
>
> If I trace the reference counting right:
> 1. eglCreateContext => API.CreateContext =>
>  - _eglInitResource => RefCount = 1
>  - _eglLinkContext => RefCount = 2
> 2. eglMakeCurrent => API.MakeCurrent => _eglBindContext =>
> eglGetContext => RefCount = 3
> 3. eglTerminate => API.Terminate => _eglReleaseDisplayResources =>
>   - _eglUnlinkContext => _eglUnlinkResource => Refcount = 2
>   - API.DestroyContext => RefCount = 1 (so context structure is not freed)
> 4. eglReleaseThread (or eglMakeCurrent(dpy, NULL, NULL, NULL)) =>
> API.MakeCurrent(drv, disp, NULL, NULL, NULL) => API.DestroyContext =>
> Refcount = 0 and structure is freed (with my current patch).
>
> Well, the structure is freed, but driDestroyContext is never called
> (so pcp->driScreenPriv->driver->DestroyContext is not called), which,
> looks a bit concerning...
>
This is exactly what made me question the proposed approach.

> So one way around this is to get eglTerminate to destroy the current> context(s). But then the spec says
> (https://www.khronos.org/registry/egl/sdk/docs/man/html/eglTerminate.xhtml):
> If contexts or surfaces associated with display is current to any
> thread, they are not released until they are no longer current as a
> result of eglMakeCurrent.
>
The way I see/read it:
 - 'leaking' ctx/surf resources if the user has not called
[eglMakeCurrent(... !NULL, !NULL) + eglTerminate +] eglReleaseThread
or eglMakeCurrent(...NULL,NULL) [+ eglTerminate] is expected.
 - we need to defer dpy vtbls (and active dri dpy connection?) in
eglReleaseThread or ctx/surf will leak

IMHO the better approach is to defer the dpy teardown (in the case of
missing eglMakeCurrent(...NULL,NULL)) to eglReleaseThread. Since that
one is more evasive (but shouldn't be too crazy) we could get this
patch, and revert to 'if (!dri2_dpy) return;' once everything else is
handled.

Not sure when we'll get time for that one, so we want an inline
XXX/TODO or two that roughly explains the idea.

Related: from a very brief look we seems to be leaking the
_EGLsync/_EGLimage resources.

Regards,
Emil