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

[Kristian Høgsberg]

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
@@ -530,6 +530,7 @@ __glXGetDrawable(__GLXcontext *glxc, GLXDrawable drawId, ClientPtr client,
 	*error = BadAlloc;
 	return NULL;
     }
+    pGlxDraw->refcnt++;
 
     return pGlxDraw;
 }
@@ -1102,6 +1103,7 @@ __glXDrawableInit(__GLXdrawable *drawable,
     drawable->drawId = drawId;
     drawable->config = config;
     drawable->eventMask = 0;
+    drawable->refcnt = 0;
 
     return GL_TRUE;
 }
@@ -1131,14 +1133,17 @@ DoCreateGLXDrawable(ClientPtr client, __GLXscreen *pGlxScreen,
 	pGlxDraw->destroy (pGlxDraw);
 	return BadAlloc;
     }
+    pGlxDraw->refcnt++;
 
     /* 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++;
     }
 
     return Success;
diff --git a/glx/glxdrawable.h b/glx/glxdrawable.h
index 2a365c5..e3d4bb5 100644
--- a/glx/glxdrawable.h
+++ b/glx/glxdrawable.h
@@ -51,6 +51,8 @@ struct __GLXdrawable {
     void      (*waitX)(__GLXdrawable *);
     void      (*waitGL)(__GLXdrawable *);
 
+    int refcnt; /* number of resources handles referencing this */
+
     DrawablePtr pDraw;
     XID drawId;
 
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;
 
     for (c = glxAllContexts; c; c = next) {
 	next = c->next;
-- 
1.7.3.2