[Mesa-dev] [PATCH] glx: Add checks for context being direct before calling GetGLXDRIDrawable

[Adam Jackson]

On Wed, 2018-08-01 at 17:15 +0300, Danylo Piliaiev wrote:
> If indirect context is explicitly created by application and not
> with LIBGL_ALWAYS_INDIRECT=1 then dri may be initialized in
> __glXInitialize which allows Mesa to take invalid code paths
> due to GetGLXDRIDrawable returning non-null value.

This doesn't make any sense. Initializing DRI doesn't add any GLX
drawables to the hash, unless something deeply insane is going on.

> In most such places there are already checks for context being
> direct before calling GetGLXDRIDrawable. This patch adds checks
> where they were missed.

These checks are pretty much all wrong. Whether a drawable has direct
context state is orthogonal to whether the current context is direct,
you could have two contexts with different directness.

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42699
> 
> Signed-off-by: Danylo Piliaiev <danylo.piliaiev at globallogic.com>
> ---
>  src/glx/glx_pbuffer.c | 12 ++++++++++--
>  src/glx/glxcmds.c     |  3 +++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
> index fd3327f120..6ee2ed58d2 100644
> --- a/src/glx/glx_pbuffer.c
> +++ b/src/glx/glx_pbuffer.c
> @@ -133,6 +133,10 @@ ChangeDrawableAttribute(Display * dpy, GLXDrawable drawable,
>     SyncHandle();
>  
>  #ifdef GLX_DIRECT_RENDERING
> +   struct glx_context *gc = __glXGetCurrentContext();
> +   if (!gc->isDirect)
> +      return;
> +

This is saying "if the current context is direct, then refuse to tell
the server that we want to change this drawable", which can't be right.

> @@ -316,7 +320,11 @@ __glXGetDrawableAttribute(Display * dpy, GLXDrawable drawable,
>        return 0;
>  
>  #if defined(GLX_DIRECT_RENDERING) && !defined(GLX_USE_APPLEGL)
> -   pdraw = GetGLXDRIDrawable(dpy, drawable);
> +   struct glx_context *gc = __glXGetCurrentContext();
> +
> +   if (gc->isDirect) {
> +      pdraw = GetGLXDRIDrawable(dpy, drawable);
> +   }

Again, whether the drawable has DRI state attached to it is not a
property of the current context.

I don't see how the existing code would be wrong to begin with. Even if
__glXInitialize does set up DRI state on the screen, it's not going to
add any drawables to the hash table, so this should still hit the pdraw
== NULL case below it safely.

>     if (attribute == GLX_BACK_BUFFER_AGE_EXT) {
>        struct glx_context *gc = __glXGetCurrentContext();
> diff --git a/src/glx/glxcmds.c b/src/glx/glxcmds.c
> index 4db0228eab..3eb86b02a9 100644
> --- a/src/glx/glxcmds.c
> +++ b/src/glx/glxcmds.c
> @@ -799,6 +799,8 @@ glXDestroyGLXPixmap(Display * dpy, GLXPixmap glxpixmap)
>     DestroyGLXDrawable(dpy, glxpixmap);
>  
>  #if defined(GLX_DIRECT_RENDERING) && !defined(GLX_USE_APPLEGL)
> +   struct glx_context *gc = __glXGetCurrentContext();
> +   if (gc->isDirect)
>     {
>        struct glx_display *const priv = __glXInitialize(dpy);
>        __GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy, glxpixmap);

I believe this would introduce a leak case. If your current context is
indirect, and you glXDestroyGLXPixmap() on a pixmap with DRI state,
you'll never free that DRI state.

> @@ -831,6 +833,7 @@ glXSwapBuffers(Display * dpy, GLXDrawable drawable)
>     gc = __glXGetCurrentContext();
>  
>  #if defined(GLX_DIRECT_RENDERING) && !defined(GLX_USE_APPLEGL)
> +   if (gc->isDirect)
>     {
>        __GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy, drawable);
>  

The real problem here, I think, is that we need to store what kind of
DRI a given DRI drawable is, and look up its SwapBuffer method based on
that. Instead we just have one method for the whole screen, so if the
screen happens to support DRI3 and DRISW...

- ajax