glamor versus AIGLX GL context series

[Michel Dänzer]

On 19.04.2014 03:39, Eric Anholt wrote:
> Here it is, at long last.  I was stumped for several days with a weird
> fbo incomplete that was happening after my first round of fixes, and
> it took a day's detour of cooking up a Xephyr with DRI3 to get a fast
> debug environment and figure out what was going on (xephyr-glamor-egl
> is that branch.  I warn you: that branch is almost entirely made up of
> layering violations and bad assumptions, and should not be viewed by
> people with a weak stomach).
> 
> With this series, I've run piglit on my glamor-using desktop, and also
> played with some LIBGL_ALWAYS_INDIRECT=1 glxgears.  Those both used to
> take down my server in different ways, so I think this fixes what we
> need for 1.16.  However, swrast AIGLX (as used in xephyr-glamor-dri3)
> has a problem with recursive flushes during the loader's GetImage, and
> I don't have a solution to it.
> 
> This branch can be found on glamor-gl-context-2 of my tree.

This didn't reliably survive LIBGL_ALWAYS_INDIRECT=1 glxgears for me, let
alone piglit. valgrind found this:


==32448== Invalid read of size 8
==32448==    at 0x836EAB4: dri2GetBuffersWithFormat (glxdri2.c:746)
==32448==    by 0xB808C8C: dri2_allocate_textures (dri2.c:185)
==32448==    by 0xB806A94: dri_st_framebuffer_validate (dri_drawable.c:83)
==32448==    by 0xB99DB09: st_framebuffer_validate (st_manager.c:193)
==32448==    by 0xB99ECC0: st_api_make_current (st_manager.c:763)
==32448==    by 0xB805D5C: dri_make_current (dri_context.c:253)
==32448==    by 0xB7DCD54: driBindContext (dri_util.c:543)
==32448==    by 0x8361217: DoMakeCurrent (glxcmds.c:639)
==32448==    by 0x8365C0E: __glXDispatch (glxext.c:595)
==32448==    by 0x4378F5: Dispatch (dispatch.c:432)
==32448==    by 0x43BB0A: dix_main (main.c:296)
==32448==    by 0x6A74B44: (below main) (libc-start.c:287)
==32448==  Address 0xf6d5480 is 0 bytes inside a block of size 16 free'd
==32448==    at 0x4C2870C: free (vg_replace_malloc.c:468)
==32448==    by 0x563063: do_get_buffers (dri2.c:509)
==32448==    by 0x5632C3: DRI2GetBuffersWithFormat (dri2.c:673)
==32448==    by 0x836EA59: dri2GetBuffersWithFormat (glxdri2.c:724)
==32448==    by 0xB808C8C: dri2_allocate_textures (dri2.c:185)
==32448==    by 0xB806A94: dri_st_framebuffer_validate (dri_drawable.c:83)
==32448==    by 0xB99DB09: st_framebuffer_validate (st_manager.c:193)
==32448==    by 0xB99ECC0: st_api_make_current (st_manager.c:763)
==32448==    by 0xB805D5C: dri_make_current (dri_context.c:253)
==32448==    by 0xB7DCD54: driBindContext (dri_util.c:543)
==32448==    by 0x836EA76: dri2GetBuffersWithFormat (glxdri2.c:729)
==32448==    by 0xB808C8C: dri2_allocate_textures (dri2.c:185)
==32448== 


The patch below fixes this.


From: =?UTF-8?q?Michel=20D=C3=A4nzer?= <michel.daenzer at amd.com>
Date: Mon, 21 Apr 2014 17:40:47 +0900
Subject: [PATCH] glx: If DRI2GetBuffers(WithFormat) changes the GL context,
 call it again
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

By changing the context, it may also invalidate the DRI2 buffer information,
so we need to get that again.

Fixes crashes due to use-after-free with LIBGL_ALWAYS_INDIRECT=1 glxgears
and piglit.

Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
---
 glx/glxdri2.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/glx/glxdri2.c b/glx/glxdri2.c
index 7b368d2..c756bf5 100644
--- a/glx/glxdri2.c
+++ b/glx/glxdri2.c
@@ -676,6 +676,13 @@ dri2GetBuffers(__DRIdrawable * driDrawable,
     if (cx != lastGLContext) {
         lastGLContext = cx;
         cx->makeCurrent(cx);
+
+        /* If DRI2GetBuffers() changed the GL context, it may also have
+         * invalidated the DRI2 buffers, so let's get them again
+         */
+        buffers = DRI2GetBuffers(private->base.pDraw,
+                                 width, height, attachments, count, out_count);
+        assert(lastGLContext == cx);
     }

     if (*out_count > MAX_DRAWABLE_BUFFERS) {
@@ -727,6 +734,14 @@ dri2GetBuffersWithFormat(__DRIdrawable * driDrawable,
     if (cx != lastGLContext) {
         lastGLContext = cx;
         cx->makeCurrent(cx);
+
+        /* If DRI2GetBuffersWithFormat() changed the GL context, it may also have
+         * invalidated the DRI2 buffers, so let's get them again
+         */
+        buffers = DRI2GetBuffersWithFormat(private->base.pDraw,
+                                           width, height, attachments, count,
+                                           out_count);
+        assert(lastGLContext == cx);
     }

     if (*out_count > MAX_DRAWABLE_BUFFERS) {
-- 
1.9.2


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer