[PATCH] glx: Refcnt the GLXDrawable to avoid use after free with multiple FreeResource

[Michel Dänzer]

On Don, 2010-12-16 at 11:54 -0500, Kristian Høgsberg wrote: 
> From: Chris Wilson <chris at chris-wilson.co.uk>
> 
> Although there may be more than one resource handles pointing to the
> Drawable, we only want to destroy it once and only reference the
> resource which may have just been deleted on the first instance.
> 
> v2: Apply fixes and combine with another bug fix from Michel Dänzer,
>     https://bugs.freedesktop.org/show_bug.cgi?id=28181
> v3: Just use the refcnt and don't try to free other resources in the
>     DrawableGone callback.
> 
> Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
> 
> Can we just do this instead?  Didn't test the patch, but this should do
> the same and actually simplifies the code.
> 
> Kristian
> 
> ---
>  glx/glxcmds.c     |   13 +++++++++----
>  glx/glxdrawable.h |    2 ++
>  glx/glxext.c      |   13 +++----------
>  3 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/glx/glxcmds.c b/glx/glxcmds.c
> index 8d13c15..1e8044b 100644
> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -1131,14 +1133,17 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen,
>  
>      /* Add the glx drawable under the XID of the underlying X drawable
>       * too.  That way we'll get a callback in DrawableGone and can
>       * clean up properly when the drawable is destroyed. */
> -    if (drawableId != glxDrawableId &&
> -	!AddResource(pDraw->id, __glXDrawableRes, pGlxDraw)) {
> -	pGlxDraw->destroy (pGlxDraw);
> -	return BadAlloc;
> +    if (drawableId != glxDrawableId) {
> +	if (!AddResource(pDraw->id, __glXDrawableRes, pGlxDraw)) {
> +	    pGlxDraw->destroy (pGlxDraw);
> +	    return BadAlloc;
> +	}
> +	pGlxDraw->refcnt++;

The fixed version of this part from Chris' v2 patch is needed.


> diff --git a/glx/glxext.c b/glx/glxext.c
> index f5ebe4f..499567a 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -126,16 +126,9 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>  {
>      __GLXcontext *c, *next;
>  
> -    /* If this drawable was created using glx 1.3 drawable
> -     * constructors, we added it as a glx drawable resource under both
> -     * its glx drawable ID and it X drawable ID.  Remove the other
> -     * resource now so we don't a callback for freed memory. */
> -    if (glxPriv->drawId != glxPriv->pDraw->id) {
> -	if (xid == glxPriv->drawId)
> -	    FreeResourceByType(glxPriv->pDraw->id, __glXDrawableRes, TRUE);
> -	else
> -	    FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
> -    }
> +    glxPriv->refcnt--;
> +    if (glxPriv->refcnt > 0)
> +	return True;

If the IDs are different, doesn't this mean the GLX drawable will never
actually be destroyed before the X drawable?


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer