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

[Kristian Høgsberg]

2010/9/23 Kristian Høgsberg <krh at bitplanet.net>:
> 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.

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 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
>>
>>
>