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

[Danylo Piliaiev]

On 01.08.18 19:42, Adam Jackson wrote:
> 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.
Ok, I see, since my patch seems to be inherently wrong I'll better 
explain what I have found and ask for advice:

The initial issue is the crash in glXSwapBuffers with indirect context, 
there GetGLXDRIDrawable returns
a non-null drawable then driScreen->swapBuffers is called then deep down 
draw->vtable->get_dri_context(draw)
is called which a pointer to glx_dri3_get_dri_context and does such cast:

struct dri3_context *pcp = (struct dri3_context *) gc;

But the context isn't dri3_context.

So why GetGLXDRIDrawable return something that is not null?

When we call glXCreateWindow:
#0 CreateDRIDrawable () at glx_pbuffer.c:209
#1 CreateDrawable () at glx_pbuffer.c:492
#2 glXCreateWindow () at glx_pbuffer.c:967

And since driScreen was created before - driScreen->createDrawable is 
called and succeeds. Now we have dri drawable in hashmap.
So later GetGLXDRIDrawable would return valid drawable.

The creation of drawable happens before we know that context will be 
indirect. Should it be deleted when indirect context is created?
Or something else? I don't know.
>> 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...
Maybe it's another issue. In this case pdraw is dri3 drawable so the 
correct SwapBuffer method
is called.
> - ajax