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

[Michel Dänzer]

On Fre, 2010-12-10 at 12:43 +0000, Chris Wilson wrote: 
> 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.
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Kristian Høgsberg <krh at bitplanet.net>
> Cc: Michel Dänzer <daenzer at vmware.com>
> ---
>  glx/glxcmds.c     |    6 ++++++
>  glx/glxdrawable.h |    3 +++
>  glx/glxext.c      |   15 ++++++++++-----
>  3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/glx/glxcmds.c b/glx/glxcmds.c
> index de9c3f0..fa153d5 100644
> --- a/glx/glxcmds.c
> +++ b/glx/glxcmds.c
> @@ -1139,6 +1143,8 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen,
>  	pGlxDraw->destroy (pGlxDraw);
>  	return BadAlloc;
>      }
> +    pGlxDraw->refcnt++;
> +    pGlxDraw->otherId = pDraw->id;

I think this should use drawableId instead of pDraw->id, and refcnt
should only be incremented if it's different from glxDrawableId.
Combined with the patch I attached to
https://bugs.freedesktop.org/show_bug.cgi?id=28181 this block could look
something like

    if (drawableId != glxDrawableId) {
	/* 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 (!AddResource(drawableId, __glXDrawableRes, pGlxDraw)) {
	    pGlxDraw->destroy (pGlxDraw);
	    return BadAlloc;
	}

	pGlxDraw->refcnt++;
	pGlxDraw->otherId = pDraw->id;
    }

Then this patch will hopefully address that bug and maybe others with
references to DrawableGone. Fingers crossed. :)


> diff --git a/glx/glxext.c b/glx/glxext.c
> index 4bd5d6b..c473aa9 100644
> --- a/glx/glxext.c
> +++ b/glx/glxext.c
> @@ -128,13 +128,18 @@ static Bool DrawableGone(__GLXdrawable *glxPriv, XID xid)
>       * 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);
> +    if (glxPriv->otherId) {
> +	    XID other = glxPriv->otherId;
> +	    glxPriv->otherId = 0;
> +	    if (xid == other)
> +		    FreeResourceByType(glxPriv->drawId, __glXDrawableRes, TRUE);
> +	    else
> +		    FreeResourceByType(other, __glXDrawableRes, TRUE);
>      }
>  
> +    if (--glxPriv->refcnt)
> +	    return TRUE;

The return at the end of the function uses True instead of TRUE.


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