[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 at 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 at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel