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

[Emil Velikov]

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.


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

>     /* make new bindings */
>     if (!_eglBindContext(ctx, dsurf, rsurf, &old_ctx, &old_dsurf, &old_rsurf)) {
>        /* _eglBindContext already sets the EGL error (in _eglCheckMakeCurrent) */
> @@ -1196,14 +1201,14 @@ dri2_make_current(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *dsurf,
>     }
>
>     /* flush before context switch */
> -   if (old_ctx && dri2_drv->glFlush)
> +   if (old_ctx && dri2_dpy && dri2_drv->glFlush)
Mildly related: We might want to bail out in dri2_load on
!dri2_drv->glFlush and drop the check here. If we cannot get glapi
and/or glFlush symbol things are seriously stuffed and pretending to
continue just won't work.

>  dri2_destroy_surface(_EGLDriver *drv, _EGLDisplay *dpy, _EGLSurface *surf)
>  {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
> +   if (!dri2_dpy)
> +      return EGL_TRUE;
Analogous to the ctx hunk, this shouldn't be needed.

Cheers,
Emil