On 08/12/10 07:56 +0100, ext Christopher James Halse Rogers wrote:
> 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
> <[email protected]>
> ---
> hw/xfree86/dri2/dri2.c | 98 ++++++++++++++++++++++++++++++++++++++---------
> hw/xfree86/dri2/dri2.h | 1 +
> 2 files changed, 80 insertions(+), 19 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index e4693d9..6da2e17 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -107,12 +107,41 @@ 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(DrawablePtr pDraw, DRI2BufferPtr buffer)
> +{
> + DRI2ScreenPtr ds;
> + if (buffer->refcnt == 0) {
> + xf86DrvMsg(pDraw->pScreen->myNum, X_ERROR,
> + "[DRI2] Attempt to unreference already freed buffer ignored\n");
> + return;
> + }
> +
> + buffer->refcnt--;
> + if (buffer->refcnt == 0) {
> + ds = DRI2GetScreen(pDraw->pScreen);
> + (*ds->DestroyBuffer)(pDraw, buffer);
> + }
> +}
> +
> static DRI2DrawablePtr
> DRI2GetDrawable(DrawablePtr pDraw)
> {
> @@ -261,7 +290,6 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw,
> XID id,
> static int DRI2DrawableGone(pointer p, XID id)
> {
> DRI2DrawablePtr pPriv = p;
> - DRI2ScreenPtr ds = pPriv->dri2_screen;
> DRI2DrawableRefPtr ref, next;
> WindowPtr pWin;
> PixmapPtr pPixmap;
> @@ -300,7 +328,7 @@ static int DRI2DrawableGone(pointer p, XID id)
>
> if (pPriv->buffers != NULL) {
> for (i = 0; i < pPriv->bufferCount; i++)
> - (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]);
> + buffer_unref(pDraw, pPriv->buffers[i]);
>
> free(pPriv->buffers);
> }
> @@ -341,6 +369,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;
>
> @@ -355,13 +384,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)(pDraw, pPriv->buffers[i]);
> + buffer_unref(pDraw, pPriv->buffers[i]);
> }
> }
>
> @@ -498,7 +526,7 @@ err_out:
>
> for (i = 0; i < count; i++) {
> if (buffers[i] != NULL)
> - (*ds->DestroyBuffer)(pDraw, buffers[i]);
> + buffer_unref(pDraw, buffers[i]);
> }
>
> free(buffers);
> @@ -708,21 +736,31 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int
> frame,
> }
> }
>
> +static void
> +free_swap_complete_data (DrawablePtr pDraw, DRI2SwapCompleteDataPtr
> pSwapData)
> +{
> + buffer_unref(pDraw, pSwapData->src);
> + buffer_unref(pDraw, pSwapData->dest);
> + free(pSwapData);
> +}
> +
> void
> DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame,
> unsigned int tv_sec, unsigned int tv_usec, int type,
> DRI2SwapEventPtr swap_complete, void *swap_data)
> {
> - ScreenPtr pScreen = pDraw->pScreen;
> - DRI2DrawablePtr pPriv;
> - CARD64 ust = 0;
> - BoxRec box;
> - RegionRec region;
> + ScreenPtr pScreen = pDraw->pScreen;
We are still accessing freed pointer pDraw. That means xserver has to be able
to keep Drawable and DRI2Drawable until SwapComplete has been called for all
pending flips.
I know that drivers can store Drawable->id instead of pointer but then they
still can't call this function to free the swap data.
> + DRI2DrawablePtr pPriv;
> + CARD64 ust = 0;
> + BoxRec box;
> + RegionRec region;
> + DRI2SwapCompleteDataPtr pSwapData = swap_data;
>
> pPriv = DRI2GetDrawable(pDraw);
Here too accessing freed memory.
> if (pPriv == NULL) {
> xf86DrvMsg(pScreen->myNum, X_ERROR,
> "[DRI2] %s: bad drawable\n", __func__);
> + free_swap_complete_data(pDraw, pSwapData);
> return;
> }
>
> @@ -739,12 +777,15 @@ DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw,
> 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, pDraw, frame, tv_sec, tv_usec);
> +
> + free_swap_complete_data(pDraw, pSwapData);
> }
>
> Bool
> @@ -771,12 +812,13 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
> CARD64 target_msc,
> CARD64 divisor, CARD64 remainder, CARD64 *swap_target,
> DRI2SwapEventPtr func, void *data)
> {
> - ScreenPtr pScreen = pDraw->pScreen;
> - DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen);
> - DRI2DrawablePtr pPriv;
> - DRI2BufferPtr pDestBuffer = NULL, pSrcBuffer = NULL;
> - int ret, i;
> - CARD64 ust, current_msc;
> + ScreenPtr pScreen = pDraw->pScreen;
> + DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen);
> + DRI2DrawablePtr pPriv;
> + DRI2BufferPtr pDestBuffer = NULL, pSrcBuffer = NULL;
> + DRI2SwapCompleteDataPtr pSwapData;
> + int ret, i;
> + CARD64 ust, current_msc;
>
> pPriv = DRI2GetDrawable(pDraw);
> if (pPriv == NULL) {
> @@ -796,6 +838,23 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
> CARD64 target_msc,
> "[DRI2] %s: drawable has no back or front?\n", __func__);
> 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) {
> @@ -812,7 +871,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
> CARD64 target_msc,
>
> (*ds->CopyRegion)(pDraw, ®ion, pDestBuffer, pSrcBuffer);
> DRI2SwapComplete(client, pDraw, target_msc, 0, 0, DRI2_BLIT_COMPLETE,
> - func, data);
> + func, pSwapData);
> return Success;
> }
>
> @@ -851,11 +910,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw,
> CARD64 target_msc,
>
> pPriv->swapsPending++;
> 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 */
> xf86DrvMsg(pScreen->myNum, X_ERROR,
> "[DRI2] %s: driver failed to schedule swap\n", __func__);
> + free_swap_complete_data(pDraw, pSwapData);
> return BadDrawable;
> }
>
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index fe0bf6c..12343fd 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -42,6 +42,7 @@ typedef struct {
> unsigned int pitch;
> unsigned int cpp;
> unsigned int flags;
> + unsigned int refcnt;
New additions to end of the structure to minimize impact of ABI/API break,
please.
> unsigned int format;
> void *driverPrivate;
> } DRI2BufferRec, *DRI2BufferPtr;
> --
> 1.7.2.3
>
> _______________________________________________
> [email protected]: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
