wmclient.cc: GetFullWindowProperty(): programming error

Hello,

The file wmclient.cc in the current CVS contains the following code
(below) which in the past was found responsible for crashes when
loading large icon data. I see that CVS incorporated a patch from
Debian that was meant to prevent those crashes.

I believe that this patch is wrong (and actually I have seen the same
crashes even with this patch). The reason for the crash is not that
some applications provide icons of unbelivably huge sizes (like the
comment suggests), but a rather simple programming mistake.

The documentation on XGetWindowProperty() describes the meaning of the
4th parameter of this function as:

long_offset  Specifies the offset in the specified property (in 32-bit
             quantities) where the data is to be retrieved.

This parameter is always set to 0 in the code below. Therefore, for a
property with a total size greater than the requested long_length
(currently, 16384*32; in the past this value grew up several times),
the first and _all_ subsequent calls of XGetWindowProperty() read the
_same_ chunk of property data that starts at offset 0 and ends around
16384*32. In the past, this reading continued indefinitely until a
crash; in the code below it continues until an artificial 2 Mb
boundary is reached (this logic also introduces a memleak, btw,
because XFree(property) gets skipped in this case). In any case, such
a behaviour is simply wrong.

In the icewm 1.2 tree that I maintain for quite some time for myself
(due to historical reasons; now the situation with icewm in Debian is
better and I am willing to switch to the official version) I have
corrected those segfaults as is shown in the second snipplet. This
works on a 32-bit machine, but corrections for 64-bits are obvious.
Sorry for not providing a direct patch!

/* snipplet from current CVS */
static void *GetFullWindowProperty(Display *display, Window handle, Atom propAtom, int &itemCount, int itemSize1)
{
    void *data = NULL;
    itemCount = 0;
    int itemSize = itemSize1;
    if (itemSize1 == 32)
        itemSize = sizeof(long) * 8;

    {
        Atom r_type;
        int r_format;
        unsigned long nitems;
        unsigned long bytes_remain;
        unsigned char *prop;

        while (XGetWindowProperty(display, handle,
                               propAtom, 0, 16384*32, False, AnyPropertyType,
                               &r_type, &r_format, &nitems, &bytes_remain,
                               &prop) == Success && prop && bytes_remain == 0)
        {
            if (r_format == itemSize1 && nitems > 0) {
                data = realloc(data, (itemCount + nitems) * itemSize / 8);

                // access to memory beyound 256MiB causes crashes! But anyhow, size
                // >>2MiB looks suspicious. Detect this case ASAP. However, if
                // the usable icon is somewhere in the beginning, it's okay to
                // return truncated data.
                if(itemCount * itemSize / 8 >= 2097152)
                   break;

                memcpy((char *)data + itemCount * itemSize / 8, prop, nitems * itemSize / 8);
                itemCount += nitems;
                XFree(prop);
                if (bytes_remain == 0)
                    break;
                continue;
            }
            XFree(prop);
            free(data);
            itemCount = 0;
            return NULL;
        }
    }
    return data;
}

/* sniplet from my own tree */

static void *GetFullWindowProperty(Display *display, Window handle, Atom propAtom, int &itemCount, int itemSize)
{
    void *data = NULL;
    itemCount = 0;

    {
        Atom r_type;
        int r_format;
        unsigned long nitems;
        unsigned long bytes_remain;
        unsigned char *prop;

        while (XGetWindowProperty(display, handle,
                               propAtom, (itemCount * itemSize) / 32, 1024*32, False, AnyPropertyType,
                               &r_type, &r_format, &nitems, &bytes_remain,
                               &prop) == Success && prop)
        {
            if (r_format == itemSize && nitems > 0) {
                data = realloc(data, (itemCount + nitems) * itemSize / 8);
                memcpy((char *)data + itemCount * itemSize / 8, prop, nitems * itemSize / 8);
                itemCount += nitems;
                XFree(prop);
                if (bytes_remain == 0)
                    break;
                continue;
            }
            XFree(prop);
            free(data);
            itemCount = 0;
            return NULL;
        }
    }
    return data;
}

-- 
Stanislav

------------------------------------------------------------------------------