On 15 July 2016 at 09:28, Nicolas Boichat <drink...@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-sta...@lists.freedesktop.org> > Signed-off-by: Nicolas Boichat <drink...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev