[PATCH] Disable DRI2 if we're running on shadowfb

[PATCH] Disable DRI2 if we're running on shadowfb

[Julien Cristau]

Without this change, DRI2 gets enabled but doesn't work and glxinfo
crashes my X server.

Signed-off-by: Julien Cristau <jcristau@debian.org>
---
 src/intel_driver.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/intel_driver.c b/src/intel_driver.c
index c0ad69e..ad90174 100644
--- a/src/intel_driver.c
+++ b/src/intel_driver.c
@@ -830,12 +830,6 @@ I830ScreenInit(int scrnIndex, ScreenPtr screen, int argc, char **argv)
 
 	scrn->videoRam = device->regions[fb_bar].size / 1024;
 
-#ifdef DRI2
-	if (intel->directRenderingType == DRI_NONE
-	    && I830DRI2ScreenInit(screen))
-		intel->directRenderingType = DRI_DRI2;
-#endif
-
 	intel->force_fallback = FALSE;
 	intel->use_shadow = FALSE;
 
@@ -859,8 +853,15 @@ I830ScreenInit(int scrnIndex, ScreenPtr screen, int argc, char **argv)
 		xf86DrvMsg(scrn->scrnIndex, X_CONFIG,
 			   "Shadow buffer enabled,"
 			   " GPU acceleration disabled.\n");
+		intel->directRenderingType = DRI_DISABLED;
 	}
 
+#ifdef DRI2
+	if (intel->directRenderingType == DRI_NONE
+	    && I830DRI2ScreenInit(screen))
+		intel->directRenderingType = DRI_DRI2;
+#endif
+
 	/* SwapBuffers delays to avoid tearing */
 	intel->swapbuffers_wait = TRUE;
 
-- 
1.7.1

Re: [PATCH] Disable DRI2 if we are running on shadowfb

[Chris Wilson]

On Wed, 22 Sep 2010 13:47:27 +0200, Julien Cristau <jcristau@debian.org> wrote:
> Without this change, DRI2 gets enabled but doesn't work and glxinfo
> crashes my X server.

Like this?

==2989== Invalid write of size 4
==2989==    at 0x48CE6E5: DrawableGone (glxext.c:169)
==2989==    by 0x809F401: FreeResource (resource.c:601)
==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
==2989==    by 0x8066439: main (main.c:291)
==2989==  Address 0x55a9c1c is 76 bytes inside a block of size 88 free'd
==2989==    at 0x4023B6A: free (vg_replace_malloc.c:366)
==2989==    by 0x48D9DD8: __glXDRIcontextDestroy (glxdri2.c:250)
==2989==    by 0x48CE1A0: __glXFreeContext (glxext.c:222)
==2989==    by 0x48CE786: DrawableGone (glxext.c:165)
==2989==    by 0x809F401: FreeResource (resource.c:601)
==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
==2989==    by 0x8066439: main (main.c:291)

Kristian, I know how much you enjoy fixing these... You probably already
have a patch. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] glx: Avoid use-after-free after drawableGone

[Chris Wilson]

Becareful during list processing to keep valgrind quiet:

==2989== Invalid read of size 4
==2989==    at 0x48CE6B5: DrawableGone (glxext.c:168)
==2989==    by 0x809F401: FreeResource (resource.c:601)
==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
==2989==    by 0x8066439: main (main.c:291)
==2989==  Address 0x55a9c1c is 76 bytes inside a block of size 88 free'd
==2989==    at 0x4023B6A: free (vg_replace_malloc.c:366)
==2989==    by 0x48D9DD8: __glXDRIcontextDestroy (glxdri2.c:250)
==2989==    by 0x48CE1A0: __glXFreeContext (glxext.c:222)
==2989==    by 0x48CE786: DrawableGone (glxext.c:165)
==2989==    by 0x809F401: FreeResource (resource.c:601)
==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
==2989==    by 0x8066439: main (main.c:291)

Reported-by: Julien Cristau <jcristau@debian.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Kristian Høgsberg <krh@bitplanet.net>
---
 glx/glxext.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/glx/glxext.c b/glx/glxext.c
index e203156..69ed24e 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -124,7 +124,7 @@ static int glxBlockClients;
 */
 static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 {
-    __GLXcontext *c;
+    __GLXcontext *c, *tmp;
 
     /* If this drawable was created using glx 1.3 drawable
      * constructors, we added it as a glx drawable resource under both
@@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 	    FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
     }
 
-    for (c = glxAllContexts; c; c = c->next) {
+    for (c = glxAllContexts; c; c = tmp) {
+	tmp = c->next;
 	if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
 	    int i;
 
@@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 		    }
 		}
 	    }
-
-	    if (!c->idExists) {
-		__glXFreeContext(c);
-	    }
 	}
 	if (c->drawPriv == glxPriv)
 	    c->drawPriv = NULL;
 	if (c->readPriv == glxPriv)
 	    c->readPriv = NULL;
+	if (!c->idExists)
+	    __glXFreeContext(c);
     }
 
     glxPriv->destroy(glxPriv);
-- 
1.7.1

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] Disable DRI2 if we're running on shadowfb

[Owain Ainsworth]

On Wed, Sep 22, 2010 at 01:47:27PM +0200, Julien Cristau wrote:
> Without this change, DRI2 gets enabled but doesn't work and glxinfo
> crashes my X server.

I think we need a bigger hammer than this.

The problem is that when we disable acceleration (EIO returned from
execbuffer), the same thing happens. I think a fix for this should also
deal with that case. Probably something conditional on the
force_fallback member at dri2 time... (we can disallow dri2
connections, right?)

No patch from me yet, but I am thinking about it.

-0-
-- 
Hartley's Second Law:
	Never sleep with anyone crazier than yourself.

Re: [PATCH] Disable DRI2 if we are running on shadowfb

[Chris Wilson]

On Wed, 22 Sep 2010 14:35:50 +0100, Owain Ainsworth <zerooa@googlemail.com> wrote:
> On Wed, Sep 22, 2010 at 01:47:27PM +0200, Julien Cristau wrote:
> > Without this change, DRI2 gets enabled but doesn't work and glxinfo
> > crashes my X server.
> 
> I think we need a bigger hammer than this.
> 
> The problem is that when we disable acceleration (EIO returned from
> execbuffer), the same thing happens. I think a fix for this should also
> deal with that case. Probably something conditional on the
> force_fallback member at dri2 time... (we can disallow dri2
> connections, right?)

This is what we get at the moment by returning buffers with name == 0 to
the client. Just we still appear to be exercising what were rarely tested
error paths...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] glx: Avoid use-after-free after drawableGone

[jamey-sH+B+fTmh7PR7s880joybQ]

I don't know the code, but this looks pretty obvious.

Reviewed-by: Jamey Sharp <jamey@minilop.net>

On Wed, Sep 22, 2010 at 5:32 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Becareful during list processing to keep valgrind quiet:
>
> ==2989== Invalid read of size 4
> ==2989==    at 0x48CE6B5: DrawableGone (glxext.c:168)
> ==2989==    by 0x809F401: FreeResource (resource.c:601)
> ==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
> ==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
> ==2989==    by 0x8066439: main (main.c:291)
> ==2989==  Address 0x55a9c1c is 76 bytes inside a block of size 88 free'd
> ==2989==    at 0x4023B6A: free (vg_replace_malloc.c:366)
> ==2989==    by 0x48D9DD8: __glXDRIcontextDestroy (glxdri2.c:250)
> ==2989==    by 0x48CE1A0: __glXFreeContext (glxext.c:222)
> ==2989==    by 0x48CE786: DrawableGone (glxext.c:165)
> ==2989==    by 0x809F401: FreeResource (resource.c:601)
> ==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
> ==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
> ==2989==    by 0x8066439: main (main.c:291)
>
> Reported-by: Julien Cristau <jcristau@debian.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Kristian Høgsberg <krh@bitplanet.net>
> ---
>  glx/glxext.c |   11 +++++------
>  1 files changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/glx/glxext.c b/glx/glxext.c
> index e203156..69ed24e 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -124,7 +124,7 @@ static int glxBlockClients;
>  */
>  static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>  {
> -    __GLXcontext *c;
> +    __GLXcontext *c, *tmp;
>
>     /* If this drawable was created using glx 1.3 drawable
>      * constructors, we added it as a glx drawable resource under both
> @@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>            FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
>     }
>
> -    for (c = glxAllContexts; c; c = c->next) {
> +    for (c = glxAllContexts; c; c = tmp) {
> +       tmp = c->next;
>        if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
>            int i;
>
> @@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>                    }
>                }
>            }
> -
> -           if (!c->idExists) {
> -               __glXFreeContext(c);
> -           }
>        }
>        if (c->drawPriv == glxPriv)
>            c->drawPriv = NULL;
>        if (c->readPriv == glxPriv)
>            c->readPriv = NULL;
> +       if (!c->idExists)
> +           __glXFreeContext(c);
>     }
>
>     glxPriv->destroy(glxPriv);
> --
> 1.7.1
>
> _______________________________________________
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] glx: Avoid use-after-free after drawableGone

[Adam Jackson]

[-- Attachment #1.1: Type: text/plain, Size: 1317 bytes --]

On Wed, 2010-09-22 at 13:32 +0100, Chris Wilson wrote:
> Becareful during list processing to keep valgrind quiet:
> 
> ==2989== Invalid read of size 4
> ==2989==    at 0x48CE6B5: DrawableGone (glxext.c:168)
> ==2989==    by 0x809F401: FreeResource (resource.c:601)
> ==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
> ==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
> ==2989==    by 0x8066439: main (main.c:291)
> ==2989==  Address 0x55a9c1c is 76 bytes inside a block of size 88 free'd
> ==2989==    at 0x4023B6A: free (vg_replace_malloc.c:366)
> ==2989==    by 0x48D9DD8: __glXDRIcontextDestroy (glxdri2.c:250)
> ==2989==    by 0x48CE1A0: __glXFreeContext (glxext.c:222)
> ==2989==    by 0x48CE786: DrawableGone (glxext.c:165)
> ==2989==    by 0x809F401: FreeResource (resource.c:601)
> ==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
> ==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
> ==2989==    by 0x8066439: main (main.c:291)
> 
> Reported-by: Julien Cristau <jcristau-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
> Cc: Kristian Høgsberg <krh-1OA22m9ORUweIZ0/mPfg9Q@public.gmane.org>

Reviewed-by: Adam Jackson <ajax-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

- ajax

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 219 bytes --]

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] glx: Fix use after free in DrawableGone

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
---

> ==2989== Invalid write of size 4
> ==2989==    at 0x48CE6E5: DrawableGone (glxext.c:169)
> ==2989==    by 0x809F401: FreeResource (resource.c:601)
> ==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
> ==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
> ==2989==    by 0x8066439: main (main.c:291)
> ==2989==  Address 0x55a9c1c is 76 bytes inside a block of size 88 free'd
> ==2989==    at 0x4023B6A: free (vg_replace_malloc.c:366)
> ==2989==    by 0x48D9DD8: __glXDRIcontextDestroy (glxdri2.c:250)
> ==2989==    by 0x48CE1A0: __glXFreeContext (glxext.c:222)
> ==2989==    by 0x48CE786: DrawableGone (glxext.c:165)
> ==2989==    by 0x809F401: FreeResource (resource.c:601)
> ==2989==    by 0x80845CE: ProcDestroyWindow (dispatch.c:733)
> ==2989==    by 0x8087D76: Dispatch (dispatch.c:432)
> ==2989==    by 0x8066439: main (main.c:291)
>
> Kristian, I know how much you enjoy fixing these... You probably already
> have a patch. ;-)

I think we're looking at this use after free problem.  We may free the
context in the big if-statement above and the go and touch its drawPriv
and readPriv fields afterwards.

Kristian

 glx/glxext.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/glx/glxext.c b/glx/glxext.c
index e203156..0b04c37 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -160,15 +160,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 		    }
 		}
 	    }
-
-	    if (!c->idExists) {
-		__glXFreeContext(c);
-	    }
 	}
 	if (c->drawPriv == glxPriv)
 	    c->drawPriv = NULL;
 	if (c->readPriv == glxPriv)
 	    c->readPriv = NULL;
+	if (!c->idExists && !c->isCurrent)
+	    __glXFreeContext(c);
     }
 
     glxPriv->destroy(glxPriv);
-- 
1.7.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] glx: Fix use after free in DrawableGone

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
---

Chris Wilson points out that we were still accessing c->next after free.
Here's an updated version that fixes that.

Kristian

 glx/glxext.c |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/glx/glxext.c b/glx/glxext.c
index e203156..f5ebe4f 100644
--- a/glx/glxext.c
+++ b/glx/glxext.c
@@ -124,7 +124,7 @@ static int glxBlockClients;
 */
 static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 {
-    __GLXcontext *c;
+    __GLXcontext *c, *next;
 
     /* If this drawable was created using glx 1.3 drawable
      * constructors, we added it as a glx drawable resource under both
@@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 	    FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
     }
 
-    for (c = glxAllContexts; c; c = c->next) {
+    for (c = glxAllContexts; c; c = next) {
+	next = c->next;
 	if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
 	    int i;
 
@@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
 		    }
 		}
 	    }
-
-	    if (!c->idExists) {
-		__glXFreeContext(c);
-	    }
 	}
 	if (c->drawPriv == glxPriv)
 	    c->drawPriv = NULL;
 	if (c->readPriv == glxPriv)
 	    c->readPriv = NULL;
+	if (!c->idExists && !c->isCurrent)
+	    __glXFreeContext(c);
     }
 
     glxPriv->destroy(glxPriv);
-- 
1.7.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] glx: Fix use after free in DrawableGone

[Chris Wilson]

[-- Attachment #1: Type: text/plain, Size: 450 bytes --]

On Thu, 23 Sep 2010 09:04:11 -0400, Kristian Høgsberg <krh-1OA22m9ORUweIZ0/mPfg9Q@public.gmane.org> wrote:
> Signed-off-by: Kristian Høgsberg <krh-1OA22m9ORUweIZ0/mPfg9Q@public.gmane.org>

Now that is starting to look familiar ;-)

Reported-by: Julien Cristau <jcristau-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
Tested-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>

-- 
Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: Type: text/plain, Size: 219 bytes --]

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] glx: Fix use after free in DrawableGone

[Jeremy Huddleston]

That seems off to me.  This is doing more than changing the c->next dereference.  You're now freeing it where you weren't before.

Previously, you freed it inside:
if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv))
    if(!c->idExists)

Now, you free it inside:
if (!c->idExists && !c->isCurrent)

So you're missing the check for (c->drawPriv == glxPriv || c->readPriv == glxPriv) ... is that intentional and we had a leak before, or are we now over-releasing?


On Sep 23, 2010, at 06:04, Kristian Høgsberg wrote:

> Signed-off-by: Kristian Høgsberg <krh-1OA22m9ORUweIZ0/mPfg9Q@public.gmane.org>
> ---
> 
> Chris Wilson points out that we were still accessing c->next after free.
> Here's an updated version that fixes that.
> 
> Kristian
> 
> glx/glxext.c |   11 +++++------
> 1 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/glx/glxext.c b/glx/glxext.c
> index e203156..f5ebe4f 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -124,7 +124,7 @@ static int glxBlockClients;
> */
> static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
> {
> -    __GLXcontext *c;
> +    __GLXcontext *c, *next;
> 
>     /* If this drawable was created using glx 1.3 drawable
>      * constructors, we added it as a glx drawable resource under both
> @@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
> 	    FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
>     }
> 
> -    for (c = glxAllContexts; c; c = c->next) {
> +    for (c = glxAllContexts; c; c = next) {
> +	next = c->next;
> 	if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
> 	    int i;
> 
> @@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
> 		    }
> 		}
> 	    }
> -
> -	    if (!c->idExists) {
> -		__glXFreeContext(c);
> -	    }
> 	}
> 	if (c->drawPriv == glxPriv)
> 	    c->drawPriv = NULL;
> 	if (c->readPriv == glxPriv)
> 	    c->readPriv = NULL;
> +	if (!c->idExists && !c->isCurrent)
> +	    __glXFreeContext(c);
>     }
> 
>     glxPriv->destroy(glxPriv);
> -- 
> 1.7.3
> 
> _______________________________________________
> xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] glx: Fix use after free in DrawableGone

[Kristian Høgsberg]

2010/9/23 Jeremy Huddleston <jeremyhu@apple.com>:
> That seems off to me.  This is doing more than changing the c->next dereference.  You're now freeing it where you weren't before.
>
> Previously, you freed it inside:
> if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv))
>    if(!c->idExists)
>
> Now, you free it inside:
> if (!c->idExists && !c->isCurrent)
>
> So you're missing the check for (c->drawPriv == glxPriv || c->readPriv == glxPriv) ... is that intentional and we had a leak before, or are we now over-releasing?

All the contexts on the context list have either idExists or isCurrent
or both true, otherwise we would have already destroyed them.  If a
context that was current is destroyed, we set idExists to NULL (it no
longer has an XID) but keep it around while it's still current.  If
the context is not current, we just destroy it right away and take it
out of the list.

The loop above, iterates through the list of all contexts to see if
there is a context that has been destroyed but is still current with
the drawable that we're getting the DrawableGone() callback for.  We
treat that as an unbind, which triggers the context destruction.  As
Jon mentions, that may not be the right thing to do, but in the scope
of this patch, we're just delaying the destroy until we're done
touching the context.

Kristian

> On Sep 23, 2010, at 06:04, Kristian Høgsberg wrote:
>
>> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
>> ---
>>
>> Chris Wilson points out that we were still accessing c->next after free.
>> Here's an updated version that fixes that.
>>
>> Kristian
>>
>> glx/glxext.c |   11 +++++------
>> 1 files changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/glx/glxext.c b/glx/glxext.c
>> index e203156..f5ebe4f 100644
>> --- a/glx/glxext.c
>> +++ b/glx/glxext.c
>> @@ -124,7 +124,7 @@ static int glxBlockClients;
>> */
>> static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>> {
>> -    __GLXcontext *c;
>> +    __GLXcontext *c, *next;
>>
>>     /* If this drawable was created using glx 1.3 drawable
>>      * constructors, we added it as a glx drawable resource under both
>> @@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>           FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
>>     }
>>
>> -    for (c = glxAllContexts; c; c = c->next) {
>> +    for (c = glxAllContexts; c; c = next) {
>> +     next = c->next;
>>       if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
>>           int i;
>>
>> @@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>                   }
>>               }
>>           }
>> -
>> -         if (!c->idExists) {
>> -             __glXFreeContext(c);
>> -         }
>>       }
>>       if (c->drawPriv == glxPriv)
>>           c->drawPriv = NULL;
>>       if (c->readPriv == glxPriv)
>>           c->readPriv = NULL;
>> +     if (!c->idExists && !c->isCurrent)
>> +         __glXFreeContext(c);
>>     }
>>
>>     glxPriv->destroy(glxPriv);
>> --
>> 1.7.3
>>
>> _______________________________________________
>> xorg-devel@lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>
>
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] glx: Fix use after free in DrawableGone

[Kristian Høgsberg]

2010/9/23 Kristian Høgsberg <krh@bitplanet.net>:
> 2010/9/23 Jeremy Huddleston <jeremyhu@apple.com>:
>> That seems off to me.  This is doing more than changing the c->next dereference.  You're now freeing it where you weren't before.
>>
>> Previously, you freed it inside:
>> if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv))
>>    if(!c->idExists)
>>
>> Now, you free it inside:
>> if (!c->idExists && !c->isCurrent)
>>
>> So you're missing the check for (c->drawPriv == glxPriv || c->readPriv == glxPriv) ... is that intentional and we had a leak before, or are we now over-releasing?
>
> All the contexts on the context list have either idExists or isCurrent
> or both true, otherwise we would have already destroyed them.  If a
> context that was current is destroyed, we set idExists to NULL (it no
> longer has an XID) but keep it around while it's still current.  If
> the context is not current, we just destroy it right away and take it
> out of the list.
>
> The loop above, iterates through the list of all contexts to see if
> there is a context that has been destroyed but is still current with
> the drawable that we're getting the DrawableGone() callback for.  We
> treat that as an unbind, which triggers the context destruction.  As
> Jon mentions, that may not be the right thing to do, but in the scope
> of this patch, we're just delaying the destroy until we're done
> touching the context.

Jeremy, does the above explanation satisfy your concerns?  Keith, do
you want to pick this up for master?

>> On Sep 23, 2010, at 06:04, Kristian Høgsberg wrote:
>>
>>> Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
>>> ---
>>>
>>> Chris Wilson points out that we were still accessing c->next after free.
>>> Here's an updated version that fixes that.
>>>
>>> Kristian
>>>
>>> glx/glxext.c |   11 +++++------
>>> 1 files changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/glx/glxext.c b/glx/glxext.c
>>> index e203156..f5ebe4f 100644
>>> --- a/glx/glxext.c
>>> +++ b/glx/glxext.c
>>> @@ -124,7 +124,7 @@ static int glxBlockClients;
>>> */
>>> static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>> {
>>> -    __GLXcontext *c;
>>> +    __GLXcontext *c, *next;
>>>
>>>     /* If this drawable was created using glx 1.3 drawable
>>>      * constructors, we added it as a glx drawable resource under both
>>> @@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>>           FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
>>>     }
>>>
>>> -    for (c = glxAllContexts; c; c = c->next) {
>>> +    for (c = glxAllContexts; c; c = next) {
>>> +     next = c->next;
>>>       if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
>>>           int i;
>>>
>>> @@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>>                   }
>>>               }
>>>           }
>>> -
>>> -         if (!c->idExists) {
>>> -             __glXFreeContext(c);
>>> -         }
>>>       }
>>>       if (c->drawPriv == glxPriv)
>>>           c->drawPriv = NULL;
>>>       if (c->readPriv == glxPriv)
>>>           c->readPriv = NULL;
>>> +     if (!c->idExists && !c->isCurrent)
>>> +         __glXFreeContext(c);
>>>     }
>>>
>>>     glxPriv->destroy(glxPriv);
>>> --
>>> 1.7.3
>>>
>>> _______________________________________________
>>> xorg-devel@lists.x.org: X.Org development
>>> Archives: http://lists.x.org/archives/xorg-devel
>>> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>>
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] glx: Fix use after free in DrawableGone

[Jeremy Huddleston]

On Sep 27, 2010, at 05:42, Kristian Høgsberg wrote:
...
> Jeremy, does the above explanation satisfy your concerns?  Keith, do
> you want to pick this up for master?

Yes, thanks.


> 
>>> On Sep 23, 2010, at 06:04, Kristian Høgsberg wrote:
>>> 
>>>> Signed-off-by: Kristian Høgsberg <krh-1OA22m9ORUweIZ0/mPfg9Q@public.gmane.org>
>>>> ---
>>>> 
>>>> Chris Wilson points out that we were still accessing c->next after free.
>>>> Here's an updated version that fixes that.
>>>> 
>>>> Kristian
>>>> 
>>>> glx/glxext.c |   11 +++++------
>>>> 1 files changed, 5 insertions(+), 6 deletions(-)
>>>> 
>>>> diff --git a/glx/glxext.c b/glx/glxext.c
>>>> index e203156..f5ebe4f 100644
>>>> --- a/glx/glxext.c
>>>> +++ b/glx/glxext.c
>>>> @@ -124,7 +124,7 @@ static int glxBlockClients;
>>>> */
>>>> static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>>> {
>>>> -    __GLXcontext *c;
>>>> +    __GLXcontext *c, *next;
>>>> 
>>>>     /* If this drawable was created using glx 1.3 drawable
>>>>      * constructors, we added it as a glx drawable resource under both
>>>> @@ -137,7 +137,8 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>>>           FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
>>>>     }
>>>> 
>>>> -    for (c = glxAllContexts; c; c = c->next) {
>>>> +    for (c = glxAllContexts; c; c = next) {
>>>> +     next = c->next;
>>>>       if (c->isCurrent && (c->drawPriv == glxPriv || c->readPriv == glxPriv)) {
>>>>           int i;
>>>> 
>>>> @@ -160,15 +161,13 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>>>>                   }
>>>>               }
>>>>           }
>>>> -
>>>> -         if (!c->idExists) {
>>>> -             __glXFreeContext(c);
>>>> -         }
>>>>       }
>>>>       if (c->drawPriv == glxPriv)
>>>>           c->drawPriv = NULL;
>>>>       if (c->readPriv == glxPriv)
>>>>           c->readPriv = NULL;
>>>> +     if (!c->idExists && !c->isCurrent)
>>>> +         __glXFreeContext(c);
>>>>     }
>>>> 
>>>>     glxPriv->destroy(glxPriv);
>>>> --
>>>> 1.7.3
>>>> 
>>>> _______________________________________________
>>>> xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
>>>> Archives: http://lists.x.org/archives/xorg-devel
>>>> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>>> 
>>> 
>> 

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH v2] glx: Fix use after free in DrawableGone

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]

On Thu, 23 Sep 2010 09:04:11 -0400, Kristian Høgsberg <krh@bitplanet.net> wrote:
> Signed-off-by: Kristian Høgsberg <krh-1OA22m9ORUweIZ0/mPfg9Q@public.gmane.org>
> ---
> 
> Chris Wilson points out that we were still accessing c->next after free.
> Here's an updated version that fixes that.

I've merged this patch (and attempted to collect all of the *-by:
lines). I liked 'next' better than 'tmp' as a name, so Kristian ends up
with the credit.

-- 
keith.packard-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 219 bytes --]

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel