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

[Kristian Høgsberg]

2010/9/23 Jeremy Huddleston <jeremyhu at 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 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
>
>