[ icewm-Bugs-2995174 ] wmclient.cc: GetFullWindowProperty(): programming error

Bugs item #2995174, was opened at 2010-05-01 15:05
Message generated for change (Settings changed) made by captnmark
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100031&aid=2995174&group_id=31

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: icewm-1.3
>Group: crash
Status: Open
Resolution: None
>Priority: 9
Private: No
Submitted By: Stanislav Maslovski (s_i_m)
>Assigned to: Marko Macek (captnmark)
Summary: wmclient.cc: GetFullWindowProperty(): programming error

Initial Comment:
Just in case if nobody reads the mailing list anymore, I am submitting it also with the tracker.

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;
}


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

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=100031&aid=2995174&group_id=31

------------------------------------------------------------------------------
Beautiful is writing same markup. Internet Explorer 9 supports
standards for HTML5, CSS3, SVG 1.1,  ECMAScript5, and DOM L2 & L3.
Spend less time writing and  rewriting code and more time creating great
experiences on the web. Be a part of the beta today.
http://p.sf.net/sfu/beautyoftheweb