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.pilia...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
