On Fri, Jul 15, 2016 at 9:03 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote: > 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.
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... 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. (tbh I'm not sure if we are handling surfaces correctly either, but, well, that's another problem...) >> /* 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