Bug 16802 - XChangeProperty() on LP64 archs leads invalid memory access.
Summary: XChangeProperty() on LP64 archs leads invalid memory access.
Status: RESOLVED NOTABUG
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/Xlib (show other bugs)
Version: git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-22 01:07 UTC by MINAMI Hirokazu
Modified: 2009-09-03 03:02 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Description MINAMI Hirokazu 2008-07-22 01:07:48 UTC
On x86-64, a call of XChangeProperty() with format=32bit seems to be handled by _XData32()
 which is defined as:
#ifdef LONG64
int
_XData32(
    Display *dpy,
    register long *data,
    unsigned len)
{
    register int *buf;
    register long i;

    while (len) {
	buf = (int *)dpy->bufptr;
	i = dpy->bufmax - (char *)buf;
	if (!i) {
	    _XFlush(dpy);
	    continue;
	}
	if (len < i)
	    i = len;
	dpy->bufptr = (char *)buf + i;
	len -= i;
	i >>= 2;
	while (--i >= 0)
	    *buf++ = *data++;
    }
    return 0;
}
#endif /* LONG64 */

Though "data" normally contains only "len" bytes here,
2*len bytes [= (len >> 2) * sizeof(long) = (len/4)*8] will be read from it by:
	i = len;
	i >>= 2;
	while (--i >= 0)
	    *buf++ = *data++;

so *data may points outside of valid memory and thismay be relevant to 
 https://bugs.freedesktop.org/show_bug.cgi?id=2748


Do we really need LONG64 specialized version of _XData32() ?
even if so, should it be enabled on LP64 environments?
Comment 1 Zdenek Kabelac 2009-09-02 11:40:50 UTC
It's crashing my application and the function is wrong on the first sight.
Data passed from XChangeProperty() are 32bits - thus there is no point to iterate on 8bytes (long*) and store them to 4bytes (int*).

*buf++ = *data++;

Obviously pointers here must have same type.
Comment 2 Zdenek Kabelac 2009-09-02 12:40:47 UTC
Ok - after checking man page - it appear the application is in fault - it was using 32-bit array for  32-bit quantities - this was obviously bad idea - as the man page specifies the array must have type 'long' - which is 64-bit on 64 bit platform - but is supposed to keep 32-bit entries.

API for XChangeProperty() is somewhat confusing. But I assume the library function is ok - and after fixing application code to use  'long' instead of CARD32 the bug has gone.

I suggest to mark this bug  NOTABUG.
Instead some application using this function on 64 bit platform must be fixed.
Comment 3 Keith Packard 2009-09-02 14:22:21 UTC
Surely the program received a compiler warning about the argument type mismatch -- that should have led the developer to read the manual and discover that Xlib encodes 32-bit values as 'unsigned long' instead of an actual 32-bit type.
Comment 4 Zdenek Kabelac 2009-09-02 14:36:57 UTC
Well unfortunately - the API/prototype of XChangeProperty()

int XChangeProperty(Display *display, Window w, Atom property, Atom
              type, int format, int mode, unsigned char *data, int nelements);

requires there (unsigned char*) - so there is actually no warning as application usually casts everything to char*.

Thus it usually requires inspection of faulty application - which is deeper in the stack trace - on the first sight the 'unusual' memory copy construct in _XData32 looked suspicious.

Hopefully it will help others to notice this issue (using long for 32bit) more easily....
Comment 5 MINAMI Hirokazu 2009-09-02 23:21:31 UTC
Thank to Zdenek. Now I understand what format=32 actually means on LP64 platforms...

So, if we must not call XChangeProperty() to set CARD32[] as a window property,
some calls to XChangeProperty() in Xlib seems to be problematic.


For example, XSetWMProtocols() is implemented in libx11/src/SetWMProto.c
 as follows :
----
Status XSetWMProtocols (Display *dpy, Window w, Atom *protocols, int count)
{
...
    XChangeProperty (dpy, w, prop, XA_ATOM, 32,
                     PropModeReplace, (unsigned char *) protocols, count);
...
}
----

In this case, type of 'protocols' is (Atom*) == (CARD32*) and not (unsigned long*). 
Shouldn't we repack it into unsigned long[] before using as an argument?
Comment 6 Julien Cristau 2009-09-03 03:02:17 UTC
> --- Comment #5 from MINAMI Hirokazu <minami@mistfall.net>  2009-09-02 23:21:31 PST ---
> In this case, type of 'protocols' is (Atom*) == (CARD32*) and not (unsigned
> long*). 
> Shouldn't we repack it into unsigned long[] before using as an argument?
> 
Atom is unsigned long on the client side, not CARD32, so that's ok.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.