DRI2: Reference count buffers across SwapBuffers

The SwapBuffers request requires that we trigger the swap at some point
in the future.  Sane drivers implement this by passing this request to
something that will trigger a callback with the buffer pointers
at the appropriate time.

The client can cause those buffers to be freed in X before the trigger
occurs, most easily by quitting.  This leads to a server crash in
the driver when trying to do the swap.

See http://bugs.freedesktop.org/show_bug.cgi?id=29065 for Radeon and
https://bugs.freedesktop.org/show_bug.cgi?id=28080 for Intel.

Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
Signed-off-by: Pauli Nieminen <ext-pauli.nieminen@nokia.com>

2 files changed, 66 insertions, 15 deletions

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 30e36e8ca..d0aecb4bd 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -108,12 +108,35 @@ typedef struct _DRI2Screen {
     ConfigNotifyProcPtr		 ConfigNotify;
 } DRI2ScreenRec;
 
+typedef struct _DRI2SwapCompleteDataRec {
+    DRI2BufferPtr	src;
+    DRI2BufferPtr	dest;
+    void *		data;
+} DRI2SwapCompleteDataRec, *DRI2SwapCompleteDataPtr;
+
 static DRI2ScreenPtr
 DRI2GetScreen(ScreenPtr pScreen)
 {
     return dixLookupPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey);
 }
 
+static void
+buffer_ref(DRI2BufferPtr buffer)
+{
+    buffer->refcnt++;
+}
+
+static void
+buffer_unref(DRI2DrawablePtr pPriv, DRI2BufferPtr buffer)
+{
+    DRI2ScreenPtr ds = pPriv->dri2_screen;
+
+    buffer->refcnt--;
+    if (buffer->refcnt == 0) {
+	(*ds->DestroyBuffer)(pPriv, buffer);
+    }
+}
+
 DRI2DrawablePtr
 DRI2GetDrawable(DrawablePtr pDraw)
 {
@@ -325,7 +348,6 @@ DRI2DestroyDrawable(ClientPtr client, DRI2DrawablePtr pPriv, XID id)
 static int DRI2DrawableGone(pointer p, XID id)
 {
     DRI2DrawablePtr pPriv = p;
-    DRI2ScreenPtr   ds = pPriv->dri2_screen;
     DRI2DrawableRefPtr ref, next;
     DrawablePtr pDraw = pPriv->drawable;
     int i;
@@ -364,7 +386,7 @@ static int DRI2DrawableGone(pointer p, XID id)
 
     if (pPriv->buffers != NULL) {
 	for (i = 0; i < pPriv->bufferCount; i++)
-	    (*ds->DestroyBuffer)(pPriv, pPriv->buffers[i]);
+	    buffer_unref(pPriv, pPriv->buffers[i]);
 
 	free(pPriv->buffers);
     }
@@ -405,6 +427,7 @@ allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr ds,
 	|| !dimensions_match
 	|| (pPriv->buffers[old_buf]->format != format)) {
 	*buffer = (*ds->CreateBuffer)(pDraw, attachment, format);
+	buffer_ref(*buffer);
 	pPriv->serialNumber = DRI2DrawableSerial(pDraw);
 	return TRUE;
 
@@ -419,14 +442,12 @@ static void
 update_dri2_drawable_buffers(DRI2DrawablePtr pPriv, DrawablePtr pDraw,
 			     DRI2BufferPtr *buffers, int *out_count, int *width, int *height)
 {
-    DRI2ScreenPtr   ds = DRI2GetScreen(pDraw->pScreen);
     int i;
 
     if (pPriv->buffers != NULL) {
 	for (i = 0; i < pPriv->bufferCount; i++) {
-	    if (pPriv->buffers[i] != NULL) {
-		(*ds->DestroyBuffer)(pPriv, pPriv->buffers[i]);
-	    }
+	    if (pPriv->buffers[i] != NULL)
+		buffer_unref(pPriv, pPriv->buffers[i]);
 	}
 
 	free(pPriv->buffers);
@@ -562,7 +583,7 @@ err_out:
 
     for (i = 0; i < count; i++) {
 	    if (buffers[i] != NULL)
-		    (*ds->DestroyBuffer)(pPriv, buffers[i]);
+		    buffer_unref(pPriv, buffers[i]);
     }
 
     free(buffers);
@@ -773,12 +794,21 @@ DRI2WakeClient(ClientPtr client, DRI2DrawablePtr pPriv, int frame,
     }
 }
 
+static void
+free_swap_complete_data (DRI2DrawablePtr pPriv, DRI2SwapCompleteDataPtr pSwapData)
+{
+    buffer_unref(pPriv, pSwapData->src);
+    buffer_unref(pPriv, pSwapData->dest);
+    free(pSwapData);
+}
+
 void
 DRI2SwapComplete(int client_index, DRI2DrawablePtr pPriv, int frame,
 		   unsigned int tv_sec, unsigned int tv_usec, int type,
 		   DRI2SwapEventPtr swap_complete, void *swap_data)
 {
     ClientPtr       client = clients[client_index];
+    DRI2SwapCompleteDataPtr pSwapData = swap_data;
     DrawablePtr     pDraw = pPriv->drawable;
     CARD64          ust = 0;
 
@@ -786,11 +816,6 @@ DRI2SwapComplete(int client_index, DRI2DrawablePtr pPriv, int frame,
     pPriv->swap_count++;
     pPriv->refcnt--;
 
-    if (pPriv->refcnt == 0) {
-	DRI2DrawableGone(pPriv, 0);
-	return;
-    }
-
     if (pDraw) {
 	BoxRec          box;
 	RegionRec       region;
@@ -806,12 +831,18 @@ DRI2SwapComplete(int client_index, DRI2DrawablePtr pPriv, int frame,
 
     ust = ((CARD64)tv_sec * 1000000) + tv_usec;
     if (swap_complete)
-	swap_complete(client, swap_data, type, ust, frame, pPriv->swap_count);
+	swap_complete(client, pSwapData->data, type, ust, frame,
+		      pPriv->swap_count);
 
     pPriv->last_swap_msc = frame;
     pPriv->last_swap_ust = ust;
 
     DRI2WakeClient(client, pPriv, frame, tv_sec, tv_usec);
+
+    free_swap_complete_data(pPriv, pSwapData);
+
+    if (pPriv->refcnt == 0)
+	DRI2DrawableGone(pPriv, 0);
 }
 
 Bool
@@ -840,6 +871,7 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
     ScreenPtr       pScreen = pPriv->dri2_screen->screen;
     DRI2ScreenPtr   ds = pPriv->dri2_screen;
     DRI2BufferPtr   pDestBuffer = NULL, pSrcBuffer = NULL;
+    DRI2SwapCompleteDataPtr pSwapData;
     int             ret, i;
     CARD64          ust, current_msc;
 
@@ -861,6 +893,23 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
 	return BadDrawable;
     }
 
+    pSwapData = malloc(sizeof *pSwapData);
+    if (pSwapData == NULL)
+	return BadAlloc;
+
+    /*
+     * Take a reference to the buffers we're exchanging.
+     * This ensures that these buffers will remain valid until the swap
+     * is completed.
+     *
+     * DRI2SwapComplete is required to unref these buffers.
+     */
+    buffer_ref(pSrcBuffer);
+    buffer_ref(pDestBuffer);
+    pSwapData->src = pSrcBuffer;
+    pSwapData->dest = pDestBuffer;
+    pSwapData->data = data;
+
     /* Old DDX or no swap interval, just blit */
     if (!ds->ScheduleSwap || !pPriv->swap_interval) {
 	BoxRec box;
@@ -877,7 +926,7 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
 
 	(*ds->CopyRegion)(pDraw, &region, pDestBuffer, pSrcBuffer);
 	DRI2SwapComplete(client->index, pPriv, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
-			 func, data);
+			 func, pSwapData);
 	return Success;
     }
 
@@ -917,12 +966,13 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
     pPriv->swapsPending++;
     pPriv->refcnt++;
     ret = (*ds->ScheduleSwap)(client, pDraw, pDestBuffer, pSrcBuffer,
-			      swap_target, divisor, remainder, func, data);
+			      swap_target, divisor, remainder, func, pSwapData);
     if (!ret) {
 	pPriv->swapsPending--; /* didn't schedule */
 	pPriv->refcnt--;
         xf86DrvMsg(pScreen->myNum, X_ERROR,
 		   "[DRI2] %s: driver failed to schedule swap\n", __func__);
+	free_swap_complete_data(pPriv, pSwapData);
 	return BadDrawable;
     }
 
diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
index 071fe4c2b..6502fef0c 100644
--- a/hw/xfree86/dri2/dri2.h
+++ b/hw/xfree86/dri2/dri2.h
@@ -44,6 +44,7 @@ typedef struct {
     unsigned int flags;
     unsigned int format;
     void *driverPrivate;
+    unsigned int refcnt;
 } DRI2BufferRec, *DRI2BufferPtr;
 
 struct _DRI2Drawable;