Abstract cursor refcountingtouch-grab-race-condition-56578-v3

Too many callers relied on the refcnt being handled correctly. Use a simple
wrapper to handle that case.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

10 files changed, 73 insertions, 56 deletions

diff --git a/Xext/saver.c b/Xext/saver.c
index 8de043f8e..fe81bc4d7 100644
--- a/Xext/saver.c
+++ b/Xext/saver.c
@@ -531,15 +531,16 @@ CreateSaverWindow(ScreenPtr pScreen)
         mask |= CWBorderPixmap;
     }
     if (pAttr->pCursor) {
+        CursorPtr cursor;
         if (!pWin->optional)
             if (!MakeWindowOptional(pWin)) {
                 FreeResource(pWin->drawable.id, RT_NONE);
                 return FALSE;
             }
-        pAttr->pCursor->refcnt++;
+        cursor = RefCursor(pAttr->pCursor);
         if (pWin->optional->cursor)
             FreeCursor(pWin->optional->cursor, (Cursor) 0);
-        pWin->optional->cursor = pAttr->pCursor;
+        pWin->optional->cursor = cursor;
         pWin->cursorIsNone = FALSE;
         CheckWindowOptionalNeed(pWin);
         mask |= CWCursor;
@@ -1065,8 +1066,7 @@ ScreenSaverSetAttributes(ClientPtr client)
                     client->errorValue = cursorID;
                     goto PatchUp;
                 }
-                pCursor->refcnt++;
-                pAttr->pCursor = pCursor;
+                pAttr->pCursor = RefCursor(pCursor);
                 pAttr->mask &= ~CWCursor;
             }
             break;
diff --git a/dix/cursor.c b/dix/cursor.c
index 1ee127ac5..0820b18ad 100644
--- a/dix/cursor.c
+++ b/dix/cursor.c
@@ -114,9 +114,13 @@ FreeCursor(pointer value, XID cid)
     ScreenPtr pscr;
     DeviceIntPtr pDev = NULL;   /* unused anyway */
 
-    if (--pCurs->refcnt != 0)
+
+    UnrefCursor(pCurs);
+    if (CursorRefCount(pCurs) != 0)
         return Success;
 
+    BUG_WARN(CursorRefCount(pCurs) < 0);
+
     for (nscr = 0; nscr < screenInfo.numScreens; nscr++) {
         pscr = screenInfo.screens[nscr];
         (void) (*pscr->UnrealizeCursor) (pDev, pscr, pCurs);
@@ -127,6 +131,33 @@ FreeCursor(pointer value, XID cid)
     return Success;
 }
 
+CursorPtr
+RefCursor(CursorPtr cursor)
+{
+    ErrorF("%s ::::: cursor is %p", __func__, cursor);
+    if (cursor) {
+        xorg_backtrace();
+        cursor->refcnt++;
+    }
+    ErrorF("\n");
+    return cursor;
+}
+
+CursorPtr
+UnrefCursor(CursorPtr cursor)
+{
+    if (cursor)
+        cursor->refcnt--;
+    return cursor;
+}
+
+int
+CursorRefCount(const CursorPtr cursor)
+{
+    return cursor ? cursor->refcnt : 0;
+}
+
+
 /*
  * We check for empty cursors so that we won't have to display them
  */
diff --git a/dix/events.c b/dix/events.c
index 8124ca93d..e5db348c6 100644
--- a/dix/events.c
+++ b/dix/events.c
@@ -931,8 +931,7 @@ ChangeToCursor(DeviceIntPtr pDev, CursorPtr cursor)
 
         (*pScreen->DisplayCursor) (pDev, pScreen, cursor);
         FreeCursor(pSprite->current, (Cursor) 0);
-        pSprite->current = cursor;
-        pSprite->current->refcnt++;
+        pSprite->current = RefCursor(cursor);
     }
 }
 
@@ -3210,11 +3209,10 @@ InitializeSprite(DeviceIntPtr pDev, WindowPtr pWin)
         pSprite->pEnqueueScreen = screenInfo.screens[0];
         pSprite->pDequeueScreen = pSprite->pEnqueueScreen;
     }
-    if (pCursor)
-        pCursor->refcnt++;
+    pCursor = RefCursor(pCursor);
     if (pSprite->current)
         FreeCursor(pSprite->current, None);
-    pSprite->current = pCursor;
+    pSprite->current = RefCursor(pCursor);
 
     if (pScreen) {
         (*pScreen->RealizeCursor) (pDev, pScreen, pSprite->current);
@@ -3293,9 +3291,7 @@ UpdateSpriteForScreen(DeviceIntPtr pDev, ScreenPtr pScreen)
     pSprite->hotLimits.x2 = pScreen->width;
     pSprite->hotLimits.y2 = pScreen->height;
     pSprite->win = win;
-    pCursor = wCursor(win);
-    if (pCursor)
-        pCursor->refcnt++;
+    pCursor = RefCursor(wCursor(win));
     if (pSprite->current)
         FreeCursor(pSprite->current, 0);
     pSprite->current = pCursor;
@@ -4945,9 +4941,7 @@ ProcChangeActivePointerGrab(ClientPtr client)
         (CompareTimeStamps(time, device->deviceGrab.grabTime) == EARLIER))
         return Success;
     oldCursor = grab->cursor;
-    grab->cursor = newCursor;
-    if (newCursor)
-        newCursor->refcnt++;
+    grab->cursor = RefCursor(newCursor);
     PostNewCursor(device);
     if (oldCursor)
         FreeCursor(oldCursor, (Cursor) 0);
@@ -5092,9 +5086,7 @@ GrabDevice(ClientPtr client, DeviceIntPtr dev,
         else
             xi2mask_merge(tempGrab->xi2mask, mask->xi2mask);
         tempGrab->device = dev;
-        tempGrab->cursor = cursor;
-        if (cursor)
-            tempGrab->cursor->refcnt++;
+        tempGrab->cursor = RefCursor(cursor);
         tempGrab->confineTo = confineTo;
         tempGrab->grabtype = grabtype;
         (*grabInfo->ActivateGrab) (dev, tempGrab, time, FALSE);
diff --git a/dix/grabs.c b/dix/grabs.c
index b254ddcf1..a03897af4 100644
--- a/dix/grabs.c
+++ b/dix/grabs.c
@@ -241,13 +241,11 @@ CreateGrab(int client, DeviceIntPtr device, DeviceIntPtr modDevice,
     grab->detail.exact = keybut;
     grab->detail.pMask = NULL;
     grab->confineTo = confineTo;
-    grab->cursor = cursor;
+    grab->cursor = RefCursor(cursor);
     grab->next = NULL;
 
     if (grabtype == XI2)
         xi2mask_merge(grab->xi2mask, mask->xi2mask);
-    if (cursor)
-        cursor->refcnt++;
     return grab;
 
 }
@@ -274,9 +272,6 @@ CopyGrab(GrabPtr dst, const GrabPtr src)
     Mask *details_mask = NULL;
     XI2Mask *xi2mask;
 
-    if (src->cursor)
-        src->cursor->refcnt++;
-
     if (src->modifiersDetail.pMask) {
         int len = MasksPerDetailMask * sizeof(Mask);
 
@@ -314,6 +309,7 @@ CopyGrab(GrabPtr dst, const GrabPtr src)
     dst->modifiersDetail.pMask = mdetails_mask;
     dst->detail.pMask = details_mask;
     dst->xi2mask = xi2mask;
+    dst->cursor = RefCursor(src->cursor);
 
     xi2mask_merge(dst->xi2mask, src->xi2mask);
 
diff --git a/dix/window.c b/dix/window.c
index a9297f3c8..8950f9766 100644
--- a/dix/window.c
+++ b/dix/window.c
@@ -547,8 +547,7 @@ InitRootWindow(WindowPtr pWin)
     (*pScreen->PositionWindow) (pWin, 0, 0);
 
     pWin->cursorIsNone = FALSE;
-    pWin->optional->cursor = rootCursor;
-    rootCursor->refcnt++;
+    pWin->optional->cursor = RefCursor(rootCursor);
 
     if (party_like_its_1989) {
         MakeRootTile(pWin);
@@ -1416,8 +1415,7 @@ ChangeWindowAttributes(WindowPtr pWin, Mask vmask, XID *vlist, ClientPtr client)
                     else if (pWin->parent && pCursor == wCursor(pWin->parent))
                         checkOptional = TRUE;
                     pOldCursor = pWin->optional->cursor;
-                    pWin->optional->cursor = pCursor;
-                    pCursor->refcnt++;
+                    pWin->optional->cursor = RefCursor(pCursor);
                     pWin->cursorIsNone = FALSE;
                     /*
                      * check on any children now matching the new cursor
@@ -3323,8 +3321,7 @@ MakeWindowOptional(WindowPtr pWin)
     parentOptional = FindWindowWithOptional(pWin)->optional;
     optional->visual = parentOptional->visual;
     if (!pWin->cursorIsNone) {
-        optional->cursor = parentOptional->cursor;
-        optional->cursor->refcnt++;
+        optional->cursor = RefCursor(parentOptional->cursor);
     }
     else {
         optional->cursor = None;
@@ -3412,8 +3409,7 @@ ChangeWindowDeviceCursor(WindowPtr pWin, DeviceIntPtr pDev, CursorPtr pCursor)
     if (pCursor && WindowParentHasDeviceCursor(pWin, pDev, pCursor))
         pNode->cursor = None;
     else {
-        pNode->cursor = pCursor;
-        pCursor->refcnt++;
+        pNode->cursor = RefCursor(pCursor);
     }
 
     pNode = pPrev = NULL;
@@ -3421,8 +3417,7 @@ ChangeWindowDeviceCursor(WindowPtr pWin, DeviceIntPtr pDev, CursorPtr pCursor)
     for (pChild = pWin->firstChild; pChild; pChild = pChild->nextSib) {
         if (WindowSeekDeviceCursor(pChild, pDev, &pNode, &pPrev)) {
             if (pNode->cursor == None) {        /* inherited from parent */
-                pNode->cursor = pOldCursor;
-                pOldCursor->refcnt++;
+                pNode->cursor = RefCursor(pOldCursor);
             }
             else if (pNode->cursor == pCursor) {
                 pNode->cursor = None;
diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
index 634ee3fe0..2b0db3492 100644
--- a/hw/xfree86/modes/xf86Cursors.c
+++ b/hw/xfree86/modes/xf86Cursors.c
@@ -481,7 +481,7 @@ xf86_use_hw_cursor(ScreenPtr screen, CursorPtr cursor)
     xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
     xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
 
-    ++cursor->refcnt;
+    cursor = RefCursor(cursor);
     if (xf86_config->cursor)
         FreeCursor(xf86_config->cursor, None);
     xf86_config->cursor = cursor;
@@ -500,7 +500,7 @@ xf86_use_hw_cursor_argb(ScreenPtr screen, CursorPtr cursor)
     xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
     xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
 
-    ++cursor->refcnt;
+    cursor = RefCursor(cursor);
     if (xf86_config->cursor)
         FreeCursor(xf86_config->cursor, None);
     xf86_config->cursor = cursor;
diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c
index d32aac1cf..860704e1c 100644
--- a/hw/xfree86/ramdac/xf86Cursor.c
+++ b/hw/xfree86/ramdac/xf86Cursor.c
@@ -271,7 +271,7 @@ xf86CursorRealizeCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs)
         (xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
                                                xf86CursorScreenKey);
 
-    if (pCurs->refcnt <= 1)
+    if (CursorRefCount(pCurs) <= 1)
         dixSetScreenPrivate(&pCurs->devPrivates, CursorScreenKey, pScreen,
                             NULL);
 
@@ -285,7 +285,7 @@ xf86CursorUnrealizeCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs)
         (xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
                                                xf86CursorScreenKey);
 
-    if (pCurs->refcnt <= 1) {
+    if (CursorRefCount(pCurs) <= 1) {
         free(dixLookupScreenPrivate
              (&pCurs->devPrivates, CursorScreenKey, pScreen));
         dixSetScreenPrivate(&pCurs->devPrivates, CursorScreenKey, pScreen,
@@ -322,37 +322,37 @@ xf86CursorSetCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs,
     /* only update for VCP, otherwise we get cursor jumps when removing a
        sprite. The second cursor is never HW rendered anyway. */
     if (GetMaster(pDev, MASTER_POINTER) == inputInfo.pointer) {
-        pCurs->refcnt++;
+        CursorPtr cursor = RefCursor(pCurs);
         if (ScreenPriv->CurrentCursor)
             FreeCursor(ScreenPriv->CurrentCursor, None);
-        ScreenPriv->CurrentCursor = pCurs;
+        ScreenPriv->CurrentCursor = cursor;
         ScreenPriv->x = x;
         ScreenPriv->y = y;
         ScreenPriv->CursorToRestore = NULL;
-        ScreenPriv->HotX = pCurs->bits->xhot;
-        ScreenPriv->HotY = pCurs->bits->yhot;
+        ScreenPriv->HotX = cursor->bits->xhot;
+        ScreenPriv->HotY = cursor->bits->yhot;
 
         if (!infoPtr->pScrn->vtSema)
-            ScreenPriv->SavedCursor = pCurs;
+            ScreenPriv->SavedCursor = cursor;
 
         if (infoPtr->pScrn->vtSema && xorg_list_is_empty(&pScreen->pixmap_dirty_list) &&
             (ScreenPriv->ForceHWCursorCount ||
              ((
 #ifdef ARGB_CURSOR
-               pCurs->bits->argb &&
+               cursor->bits->argb &&
                infoPtr->UseHWCursorARGB &&
-               (*infoPtr->UseHWCursorARGB)(pScreen, pCurs)) ||
-              (pCurs->bits->argb == 0 &&
+               (*infoPtr->UseHWCursorARGB)(pScreen, cursor)) ||
+              (cursor->bits->argb == 0 &&
 #endif
-               (pCurs->bits->height <= infoPtr->MaxHeight) &&
-               (pCurs->bits->width <= infoPtr->MaxWidth) &&
-               (!infoPtr->UseHWCursor || (*infoPtr->UseHWCursor) (pScreen, pCurs)))))) {
+               (cursor->bits->height <= infoPtr->MaxHeight) &&
+               (cursor->bits->width <= infoPtr->MaxWidth) &&
+               (!infoPtr->UseHWCursor || (*infoPtr->UseHWCursor) (pScreen, cursor)))))) {
             
             if (ScreenPriv->SWCursor)   /* remove the SW cursor */
                 (*ScreenPriv->spriteFuncs->SetCursor) (pDev, pScreen,
                                                        NullCursor, x, y);
 
-            xf86SetCursor(pScreen, pCurs, x, y);
+            xf86SetCursor(pScreen, cursor, x, y);
             ScreenPriv->SWCursor = FALSE;
             ScreenPriv->isUp = TRUE;
 
diff --git a/include/cursor.h b/include/cursor.h
index 082325123..89a650fc5 100644
--- a/include/cursor.h
+++ b/include/cursor.h
@@ -71,6 +71,10 @@ extern _X_EXPORT CursorPtr rootCursor;
 extern _X_EXPORT int FreeCursor(pointer /*pCurs */ ,
                                 XID /*cid */ );
 
+extern _X_EXPORT CursorPtr RefCursor(CursorPtr /* cursor */);
+extern _X_EXPORT CursorPtr UnrefCursor(CursorPtr /* cursor */);
+extern _X_EXPORT int CursorRefCount(const CursorPtr /* cursor */);
+
 extern _X_EXPORT int AllocARGBCursor(unsigned char * /*psrcbits */ ,
                                      unsigned char * /*pmaskbits */ ,
                                      CARD32 * /*argb */ ,
diff --git a/render/animcur.c b/render/animcur.c
index 9cbba83fa..038c5b9be 100644
--- a/render/animcur.c
+++ b/render/animcur.c
@@ -383,8 +383,7 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int ncursor,
     ac->elts = (AnimCurElt *) (ac + 1);
 
     for (i = 0; i < ncursor; i++) {
-        cursors[i]->refcnt++;
-        ac->elts[i].pCursor = cursors[i];
+        ac->elts[i].pCursor = RefCursor(cursors[i]);
         ac->elts[i].delay = deltas[i];
     }
 
diff --git a/xfixes/cursor.c b/xfixes/cursor.c
index 90a026b28..753d5f7bc 100644
--- a/xfixes/cursor.c
+++ b/xfixes/cursor.c
@@ -613,12 +613,12 @@ ReplaceCursorLookup(pointer value, XID id, pointer closure)
     }
     if (pCursor && pCursor != rcl->pNew) {
         if ((*rcl->testCursor) (pCursor, rcl->closure)) {
-            rcl->pNew->refcnt++;
+            CursorPtr curs = RefCursor(rcl->pNew);
             /* either redirect reference or update resource database */
             if (pCursorRef)
-                *pCursorRef = rcl->pNew;
+                *pCursorRef = curs;
             else
-                ChangeResourceValue(id, RT_CURSOR, rcl->pNew);
+                ChangeResourceValue(id, RT_CURSOR, curs);
             FreeCursor(pCursor, cursor);
         }
     }