| Summary: | XChangeProperty() on LP64 archs leads invalid memory access. | ||
|---|---|---|---|
| Product: | xorg | Reporter: | MINAMI Hirokazu <minami> |
| Component: | Lib/Xlib | Assignee: | Xorg Project Team <xorg-team> |
| Status: | RESOLVED NOTABUG | QA Contact: | Xorg Project Team <xorg-team> |
| Severity: | normal | ||
| Priority: | medium | CC: | zdenek.kabelac |
| Version: | git | ||
| Hardware: | x86-64 (AMD64) | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
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. 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. 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. 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....
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 #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.
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?