hw/xwin: Make winOverrideIcon() thread-safe for icon data access

winOverrideIcon() is called from the internal WM client thread.

Accessing server-internal data structures to get icon data or window hints is
not safe, as there is no lock to ensure we do not collide with these data
structures being updated in the server thread.

Rewrite so the internal client thread uses X client calls to obtain this data
safely

We used to also set the icon inside the server when the window was initially
created.  For simplicity, we simply send a message to the internal WM to update
the icon when the window is created (rather than writing different icon update
code which can work in the server thread for that one case...)

extwm mode used to do the icon update in the server. I'm not sure that actually
made much sense.  Let's assume the external WM client can do it instead...

v2
Make sure that WM_WM_ICON_EVENT does nothing for override-redirect windows

v3
Reinstate check that native window actually has expected properties for an X
window before trying to update it's icon; some auxiliary windows owned by the
XWin process don't, which would cause a crash

v4
Various fixes to pixmap icon conversion:
- remove left-over malloc in winScaleXimageToWindowsIcon causing a memory leak
- don't recalculate DDBitmap stride in winScaleXimageToWindowsIcon, when we already have worked it out
- properly check that XGetWindowProperty(NET_WM_ICON) returned some data
- don't try to retrieve WM_HINTS icon_mask if it isn't set
- restore accidentally dropped calculation of effBpp, stride, maskStride of output DDBitmap
- make sure imageMask is zero-initalized before we use it to mask the DDBitmap

v5
Remove a left-over unused variable

v6
Avoid XDestroyImage(NULL) crash if XGetImage failed for icon_pixmap

Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
Reviewed-by: Colin Harrison <colin.harrison@virgin.net>

1 files changed, 193 insertions, 199 deletions

diff --git a/hw/xwin/winmultiwindowicons.c b/hw/xwin/winmultiwindowicons.c
index df59f6066..b8357e72d 100644
--- a/hw/xwin/winmultiwindowicons.c
+++ b/hw/xwin/winmultiwindowicons.c
@@ -31,75 +31,65 @@
 #ifdef HAVE_XWIN_CONFIG_H
 #include <xwin-config.h>
 #endif
-#include "win.h"
-#include "dixevents.h"
-#include "winmultiwindowclass.h"
-#include "winprefs.h"
 
-#include "propertyst.h"
-#include "windowstr.h"
+#ifndef WINVER
+#define WINVER 0x0500
+#endif
+
+#include <X11/Xwindows.h>
+#include <X11/Xlib.h>
+#include <X11/Xutil.h>
 
+#include "winresource.h"
+#include "winprefs.h"
+#include "winmsg.h"
+#include "winmultiwindowicons.h"
+#include "winglobals.h"
 /*
- * Prototypes for local functions
+ * global variables
  */
-
-static void
-
-winScaleXBitmapToWindows(int iconSize, int effBPP,
-                         PixmapPtr pixmap, unsigned char *image);
+extern HINSTANCE g_hInstance;
 
 /*
- * Scale an X icon bitmap into a Windoze icon bitmap
+ * Scale an X icon ZPixmap into a Windoze icon bitmap
  */
 
 static void
-winScaleXBitmapToWindows(int iconSize,
-                         int effBPP, PixmapPtr pixmap, unsigned char *image)
+winScaleXImageToWindowsIcon(int iconSize,
+                            int effBPP,
+                            int stride, XImage * pixmap, unsigned char *image)
 {
     int row, column, effXBPP, effXDepth;
     unsigned char *outPtr;
-    char *iconData = 0;
-    int stride, xStride;
+    unsigned char *iconData = 0;
+    int xStride;
     float factX, factY;
     int posX, posY;
     unsigned char *ptr;
     unsigned int zero;
     unsigned int color;
 
-    effXBPP = BitsPerPixel(pixmap->drawable.depth);
-    effXDepth = pixmap->drawable.depth;
-
-    if (pixmap->drawable.bitsPerPixel == 15)
+    effXBPP = pixmap->bits_per_pixel;
+    if (pixmap->bits_per_pixel == 15)
         effXBPP = 16;
 
-    if (pixmap->drawable.depth == 15)
+    effXDepth = pixmap->depth;
+    if (pixmap->depth == 15)
         effXDepth = 16;
 
-    /* Need 16-bit aligned rows for DDBitmaps */
-    stride = ((iconSize * effBPP + 15) & (~15)) / 8;
-    xStride = PixmapBytePad(pixmap->drawable.width, pixmap->drawable.depth);
+    xStride = pixmap->bytes_per_line;
     if (stride == 0 || xStride == 0) {
         ErrorF("winScaleXBitmapToWindows - stride or xStride is zero.  "
                "Bailing.\n");
         return;
     }
 
-    /* Allocate memory for icon data */
-    iconData = malloc(xStride * pixmap->drawable.height);
-    if (!iconData) {
-        ErrorF("winScaleXBitmapToWindows - malloc failed for iconData.  "
-               "Bailing.\n");
-        return;
-    }
-
     /* Get icon data */
-    miGetImage((DrawablePtr) &(pixmap->drawable), 0, 0,
-               pixmap->drawable.width, pixmap->drawable.height,
-               ZPixmap, 0xffffffff, iconData);
+    iconData = (unsigned char *) pixmap->data;
 
     /* Keep aspect ratio */
-    factX = ((float) pixmap->drawable.width) / ((float) iconSize);
-    factY = ((float) pixmap->drawable.height) / ((float) iconSize);
+    factX = ((float) pixmap->width) / ((float) iconSize);
+    factY = ((float) pixmap->height) / ((float) iconSize);
     if (factX > factY)
         factY = factX;
     else
@@ -119,8 +109,7 @@ winScaleXBitmapToWindows(int iconSize,
                 ptr += posX / 8;
 
                 /* Out of X icon bounds, leave space blank */
-                if (posX >= pixmap->drawable.width
-                    || posY >= pixmap->drawable.height)
+                if (posX >= pixmap->width || posY >= pixmap->height)
                     ptr = (unsigned char *) &zero;
 
                 if ((*ptr) & (1 << (posX & 7)))
@@ -162,8 +151,7 @@ winScaleXBitmapToWindows(int iconSize,
                 ptr += posX * (effXBPP / 8);
 
                 /* Out of X icon bounds, leave space blank */
-                if (posX >= pixmap->drawable.width
-                    || posY >= pixmap->drawable.height)
+                if (posX >= pixmap->width || posY >= pixmap->height)
                     ptr = (unsigned char *) &zero;
                 color = (((*ptr) << 16)
                          + ((*(ptr + 1)) << 8)
@@ -203,8 +191,7 @@ winScaleXBitmapToWindows(int iconSize,
                 ptr += posX * (effXBPP / 8);
 
                 /* Out of X icon bounds, leave space blank */
-                if (posX >= pixmap->drawable.width
-                    || posY >= pixmap->drawable.height)
+                if (posX >= pixmap->width || posY >= pixmap->height)
                     ptr = (unsigned char *) &zero;
                 color = ((*ptr) << 8) + (*(ptr + 1));
                 switch (effBPP) {
@@ -238,7 +225,6 @@ winScaleXBitmapToWindows(int iconSize,
             }                   /* end if effxbpp==16) */
         }                       /* end for column */
     }                           /* end for row */
-    free(iconData);
 }
 
 static HICON
@@ -250,7 +236,7 @@ NetWMToWinIconAlpha(uint32_t * icon)
     HICON result;
     HDC hdc = GetDC(NULL);
     uint32_t *DIB_pixels;
-    ICONINFO ii = { TRUE };
+    ICONINFO ii;
     BITMAPV4HEADER bmh = { sizeof(bmh) };
 
     /* Define an ARGB pixel format used for Color+Alpha icons */
@@ -264,6 +250,9 @@ NetWMToWinIconAlpha(uint32_t * icon)
     bmh.bV4GreenMask = 0x0000FF00;
     bmh.bV4BlueMask = 0x000000FF;
 
+    ii.fIcon = TRUE;
+    ii.xHotspot = 0;            /* ignored */
+    ii.yHotspot = 0;            /* ignored */
     ii.hbmColor = CreateDIBSection(hdc, (BITMAPINFO *) & bmh,
                                    DIB_RGB_COLORS, (void **) &DIB_pixels, NULL,
                                    0);
@@ -291,12 +280,15 @@ NetWMToWinIconThreshold(uint32_t * icon)
     uint32_t *pixels = &icon[2];
     int row, col;
     HICON result;
-    ICONINFO ii = { TRUE };
+    ICONINFO ii;
 
     HDC hdc = GetDC(NULL);
     HDC xorDC = CreateCompatibleDC(hdc);
     HDC andDC = CreateCompatibleDC(hdc);
 
+    ii.fIcon = TRUE;
+    ii.xHotspot = 0;            /* ignored */
+    ii.yHotspot = 0;            /* ignored */
     ii.hbmColor = CreateCompatibleBitmap(hdc, width, height);
     ii.hbmMask = CreateCompatibleBitmap(hdc, width, height);
     ReleaseDC(NULL, hdc);
@@ -365,202 +357,220 @@ NetWMToWinIcon(int bpp, uint32_t * icon)
         return NetWMToWinIconThreshold(icon);
 }
 
-static pointer
-GetWindowProp(WindowPtr pWin, Atom name, long int *size_return)
-{
-    struct _Window *pwin;
-    struct _Property *prop;
-
-    if (!pWin || !name) {
-        ErrorF("GetWindowProp - pWin or name was NULL\n");
-        return 0;
-    }
-    pwin = (struct _Window *) pWin;
-    if (!pwin->optional)
-        return NULL;
-    for (prop = (struct _Property *) pwin->optional->userProps;
-         prop; prop = prop->next) {
-        if (prop->propertyName == name) {
-            *size_return = prop->size;
-            return prop->data;
-        }
-    }
-    return NULL;
-}
-
 /*
  * Attempt to create a custom icon from the WM_HINTS bitmaps
  */
 
-HICON
-winXIconToHICON(WindowPtr pWin, int iconSize)
+static
+ HICON
+winXIconToHICON(Display * pDisplay, Window id, int iconSize)
 {
-    unsigned char *mask, *image, *imageMask;
+    unsigned char *mask, *image = NULL, *imageMask;
     unsigned char *dst, *src;
-    PixmapPtr iconPtr;
-    PixmapPtr maskPtr;
-    int planes, bpp, effBPP, stride, maskStride, i;
+    int planes, bpp, i;
     int biggest_size = 0;
     HDC hDC;
     ICONINFO ii;
-    WinXWMHints hints;
+    XWMHints *hints;
     HICON hIcon = NULL;
     uint32_t *biggest_icon = NULL;
 
-    /* Try to get _NET_WM_ICON icons first */
     static Atom _XA_NET_WM_ICON;
     static int generation;
     uint32_t *icon, *icon_data = NULL;
-    long int size = 0;
+    unsigned long int size;
+    unsigned long int type;
+    int format;
+    unsigned long int left;
 
     hDC = GetDC(GetDesktopWindow());
     planes = GetDeviceCaps(hDC, PLANES);
     bpp = GetDeviceCaps(hDC, BITSPIXEL);
     ReleaseDC(GetDesktopWindow(), hDC);
 
+    /* Always prefer _NET_WM_ICON icons */
     if (generation != serverGeneration) {
         generation = serverGeneration;
-        _XA_NET_WM_ICON = MakeAtom("_NET_WM_ICON", 12, TRUE);
+        _XA_NET_WM_ICON = XInternAtom(pDisplay, "_NET_WM_ICON", FALSE);
     }
 
-    if (_XA_NET_WM_ICON)
-        icon_data = GetWindowProp(pWin, _XA_NET_WM_ICON, &size);
-    if (icon_data) {
-        for (icon = icon_data;
-             icon < &icon_data[size] && *icon;
+    if ((XGetWindowProperty(pDisplay, id, _XA_NET_WM_ICON,
+                            0, MAXINT, FALSE,
+                            AnyPropertyType, &type, &format, &size, &left,
+                            (unsigned char **) &icon_data) == Success) &&
+        (icon_data != NULL)) {
+        for (icon = icon_data; icon < &icon_data[size] && *icon;
              icon = &icon[icon[0] * icon[1] + 2]) {
-            if (icon[0] == iconSize && icon[1] == iconSize)
-                return NetWMToWinIcon(bpp, icon);
-            /* Find the biggest icon and let Windows scale the size */
+            /* Find an exact match to the size we require...  */
+            if (icon[0] == iconSize && icon[1] == iconSize) {
+                winDebug("winXIconToHICON: found %lu x %lu NetIcon\n", icon[0],
+                         icon[1]);
+                hIcon = NetWMToWinIcon(bpp, icon);
+                break;
+            }
+            /* Otherwise, find the biggest icon and let Windows scale the size */
             else if (biggest_size < icon[0]) {
                 biggest_icon = icon;
                 biggest_size = icon[0];
             }
         }
-        if (biggest_icon)
-            return NetWMToWinIcon(bpp, biggest_icon);
-    }
-    winDebug("winXIconToHICON - pWin %x: no suitable NetIcon\n", (int) pWin,
-             iconSize);
 
-    winMultiWindowGetWMHints(pWin, &hints);
-    if (!hints.icon_pixmap)
-        return NULL;
+        if (!hIcon && biggest_icon) {
+            winDebug
+                ("winXIconToHICON: selected %lu x %lu NetIcon for scaling to %u x %u\n",
+                 biggest_icon[0], biggest_icon[1], iconSize, iconSize);
+
+            hIcon = NetWMToWinIcon(bpp, biggest_icon);
+        }
 
-    dixLookupResourceByType((pointer) &iconPtr, hints.icon_pixmap, RT_PIXMAP,
-                            NullClient, DixUnknownAccess);
+        XFree(icon_data);
+    }
 
-    if (!iconPtr)
-        return NULL;
+    if (!hIcon) {
+        winDebug("winXIconToHICON: no suitable NetIcon\n");
+
+        hints = XGetWMHints(pDisplay, id);
+        if (hints) {
+            winDebug("winXIconToHICON: id 0x%x icon_pixmap hint %x\n", id,
+                     hints->icon_pixmap);
+
+            if (hints->icon_pixmap) {
+                Window root;
+                int x, y;
+                unsigned int width, height, border_width, depth;
+                XImage *xImageIcon;
+                XImage *xImageMask = NULL;
+
+                XGetGeometry(pDisplay, hints->icon_pixmap, &root, &x, &y,
+                             &width, &height, &border_width, &depth);
+
+                xImageIcon =
+                    XGetImage(pDisplay, hints->icon_pixmap, 0, 0, width, height,
+                              0xFFFFFFFF, ZPixmap);
+                winDebug("winXIconToHICON: id 0x%x icon Ximage 0x%x\n", id,
+                         xImageIcon);
+
+                if (hints->icon_mask)
+                    xImageMask =
+                        XGetImage(pDisplay, hints->icon_mask, 0, 0, width,
+                                  height, 0xFFFFFFFF, ZPixmap);
+
+                if (xImageIcon) {
+                    int effBPP, stride, maskStride;
+
+                    /* 15 BPP is really 16BPP as far as we care */
+                    if (bpp == 15)
+                        effBPP = 16;
+                    else
+                        effBPP = bpp;
 
-    /* 15 BPP is really 16BPP as far as we care */
-    if (bpp == 15)
-        effBPP = 16;
-    else
-        effBPP = bpp;
+                    /* Need 16-bit aligned rows for DDBitmaps */
+                    stride = ((iconSize * effBPP + 15) & (~15)) / 8;
 
-    /* Need 16-bit aligned rows for DDBitmaps */
-    stride = ((iconSize * effBPP + 15) & (~15)) / 8;
+                    /* Mask is 1-bit deep */
+                    maskStride = ((iconSize * 1 + 15) & (~15)) / 8;
 
-    /* Mask is 1-bit deep */
-    maskStride = ((iconSize * 1 + 15) & (~15)) / 8;
+                    image = malloc(stride * iconSize);
+                    imageMask = malloc(stride * iconSize);
+                    mask = malloc(maskStride * iconSize);
 
-    image = malloc(stride * iconSize);
-    imageMask = malloc(stride * iconSize);
-    /* Default to a completely black mask */
-    mask = calloc(maskStride, iconSize);
+                    /* Default to a completely black mask */
+                    memset(imageMask, 0, stride * iconSize);
+                    memset(mask, 0, maskStride * iconSize);
 
-    winScaleXBitmapToWindows(iconSize, effBPP, iconPtr, image);
-    dixLookupResourceByType((pointer) &maskPtr, hints.icon_mask, RT_PIXMAP,
-                            NullClient, DixUnknownAccess);
+                    winScaleXImageToWindowsIcon(iconSize, effBPP, stride,
+                                                xImageIcon, image);
 
-    if (maskPtr) {
-        winScaleXBitmapToWindows(iconSize, 1, maskPtr, mask);
+                    if (xImageMask) {
+                        winScaleXImageToWindowsIcon(iconSize, 1, maskStride,
+                                                    xImageMask, mask);
+                        winScaleXImageToWindowsIcon(iconSize, effBPP, stride,
+                                                    xImageMask, imageMask);
+                    }
 
-        winScaleXBitmapToWindows(iconSize, effBPP, maskPtr, imageMask);
+                    /* Now we need to set all bits of the icon which are not masked */
+                    /* on to 0 because Color is really an XOR, not an OR function */
+                    dst = image;
+                    src = imageMask;
 
-        /* Now we need to set all bits of the icon which are not masked */
-        /* on to 0 because Color is really an XOR, not an OR function */
-        dst = image;
-        src = imageMask;
+                    for (i = 0; i < (stride * iconSize); i++)
+                        if ((*(src++)))
+                            *(dst++) = 0;
+                        else
+                            dst++;
 
-        for (i = 0; i < (stride * iconSize); i++)
-            if ((*(src++)))
-                *(dst++) = 0;
-            else
-                dst++;
-    }
+                    ii.fIcon = TRUE;
+                    ii.xHotspot = 0;    /* ignored */
+                    ii.yHotspot = 0;    /* ignored */
 
-    ii.fIcon = TRUE;
-    ii.xHotspot = 0;            /* ignored */
-    ii.yHotspot = 0;            /* ignored */
+                    /* Create Win32 mask from pixmap shape */
+                    ii.hbmMask =
+                        CreateBitmap(iconSize, iconSize, planes, 1, mask);
 
-    /* Create Win32 mask from pixmap shape */
-    ii.hbmMask = CreateBitmap(iconSize, iconSize, planes, 1, mask);
+                    /* Create Win32 bitmap from pixmap */
+                    ii.hbmColor =
+                        CreateBitmap(iconSize, iconSize, planes, bpp, image);
 
-    /* Create Win32 bitmap from pixmap */
-    ii.hbmColor = CreateBitmap(iconSize, iconSize, planes, bpp, image);
+                    /* Merge Win32 mask and bitmap into icon */
+                    hIcon = CreateIconIndirect(&ii);
 
-    /* Merge Win32 mask and bitmap into icon */
-    hIcon = CreateIconIndirect(&ii);
+                    /* Release Win32 mask and bitmap */
+                    DeleteObject(ii.hbmMask);
+                    DeleteObject(ii.hbmColor);
 
-    /* Release Win32 mask and bitmap */
-    DeleteObject(ii.hbmMask);
-    DeleteObject(ii.hbmColor);
+                    /* Free X mask and bitmap */
+                    free(mask);
+                    free(image);
+                    free(imageMask);
 
-    /* Free X mask and bitmap */
-    free(mask);
-    free(image);
-    free(imageMask);
+                    if (xImageMask)
+                        XDestroyImage(xImageMask);
 
+                    XDestroyImage(xImageIcon);
+                }
+            }
+            XFree(hints);
+        }
+    }
     return hIcon;
 }
 
 /*
- * Change the Windows window icon 
+ * Change the Windows window icon
  */
 
 #ifdef XWIN_MULTIWINDOW
 void
-winUpdateIcon(Window id)
+winUpdateIcon(HWND hWnd, Display * pDisplay, Window id, HICON hIconNew)
 {
-    WindowPtr pWin;
     HICON hIcon, hIconSmall = NULL, hIconOld;
 
-    dixLookupResourceByType((pointer) &pWin, id, RT_WINDOW, NullClient,
-                            DixUnknownAccess);
-    if (pWin) {
-        winWindowPriv(pWin);
-        if (pWinPriv->hWnd) {
-            hIcon = winOverrideIcon((unsigned long) pWin);
-            if (!hIcon) {
-                hIcon = winXIconToHICON(pWin, GetSystemMetrics(SM_CXICON));
-                if (!hIcon) {
-                    hIcon = g_hIconX;
-                    hIconSmall = g_hSmallIconX;
-                }
-                else {
-                    /* Leave undefined if not found */
-                    hIconSmall =
-                        winXIconToHICON(pWin, GetSystemMetrics(SM_CXSMICON));
-                }
-            }
+    /* Start with the icon from preferences, if any */
+    hIcon = hIconNew;
+    hIconSmall = hIconNew;
 
-            /* Set the large icon */
-            hIconOld = (HICON) SendMessage(pWinPriv->hWnd,
-                                           WM_SETICON, ICON_BIG,
-                                           (LPARAM) hIcon);
+    /* If we still need an icon, try and get the icon from WM_HINTS */
+    if (!hIcon)
+        hIcon = winXIconToHICON(pDisplay, id, GetSystemMetrics(SM_CXICON));
+    if (!hIconSmall)
+        hIconSmall =
+            winXIconToHICON(pDisplay, id, GetSystemMetrics(SM_CXSMICON));
 
-            /* Delete the icon if its not the default */
-            winDestroyIcon(hIconOld);
-
-            /* Same for the small icon */
-            hIconOld = (HICON) SendMessage(pWinPriv->hWnd,
-                                           WM_SETICON, ICON_SMALL,
-                                           (LPARAM) hIconSmall);
-            winDestroyIcon(hIconOld);
-        }
+    /* If we got the small, but not the large one swap them */
+    if (!hIcon && hIconSmall) {
+        hIcon = hIconSmall;
+        hIconSmall = NULL;
     }
+
+    /* Set the large icon */
+    hIconOld = (HICON) SendMessage(hWnd, WM_SETICON, ICON_BIG, (LPARAM) hIcon);
+    /* Delete the old icon if its not the default */
+    winDestroyIcon(hIconOld);
+
+    /* Same for the small icon */
+    hIconOld =
+        (HICON) SendMessage(hWnd, WM_SETICON, ICON_SMALL, (LPARAM) hIconSmall);
+    winDestroyIcon(hIconOld);
 }
 
 void
@@ -591,37 +601,21 @@ winInitGlobalIcons(void)
 }
 
 void
-winSelectIcons(WindowPtr pWin, HICON * pIcon, HICON * pSmallIcon)
+winSelectIcons(HICON * pIcon, HICON * pSmallIcon)
 {
     HICON hIcon, hSmallIcon;
 
     winInitGlobalIcons();
 
-    /* Try and get the icon from WM_HINTS */
-    hIcon = winXIconToHICON(pWin, GetSystemMetrics(SM_CXICON));
-    hSmallIcon = winXIconToHICON(pWin, GetSystemMetrics(SM_CXSMICON));
-
-    /* If we got the small, but not the large one swap them */
-    if (!hIcon && hSmallIcon) {
-        hIcon = hSmallIcon;
-        hSmallIcon = NULL;
-    }
-
-    /* Use default X icon if no icon loaded from WM_HINTS */
-    if (!hIcon) {
-        hIcon = g_hIconX;
-        hSmallIcon = g_hSmallIconX;
-    }
+    /* Use default X icon */
+    hIcon = g_hIconX;
+    hSmallIcon = g_hSmallIconX;
 
     if (pIcon)
         *pIcon = hIcon;
-    else
-        winDestroyIcon(hIcon);
 
     if (pSmallIcon)
         *pSmallIcon = hSmallIcon;
-    else
-        winDestroyIcon(hSmallIcon);
 }
 
 void