Re: Alignment error in libXi
On Sun, Mar 27, 2011 at 10:42:14PM +0200, Matthieu Herrb wrote: On Thu, Mar 24, 2011 at 11:51:32PM -0700, Jeremy Huddleston wrote: Don't feel too bad. I missed it too and bugged Peter about it before I pushed the release ;) Unfortunatly, as Peter forsaid it, there are other issues. I'm currently fighting with the code for XISelectEvents(). It uses Data32() to send a xXIEventMask structure. This is very wrong and fails miserablily on sparc64, for at least 2 reasons: 1. Data32() suffers from the infamous 'use unsigned long for 32 bits data' API mis-design, so on 64 bits machines one can't pass a pointer to a 32 bit only data to Data32(): it has one chance over 2 to be mis-aligned, and will read 4 extra random bytes on the stack. 2. since xXIEventMask contains two uint_16 values, which are not properly swapped, this will not work across client/server with different endianness. swapping is handled by the server anyway, so the library shouldn't need to worry about it. can't we just replace the Data32() call with Data() here? Aside from the API issues, the only difference is that Data32 pads out, right? Given that xXIEventMask is always 4 bytes we don't need the padding. Cheers, Peter This can all be seen whil trying Gtk+3's gtk3-demo on an OpenBSD/sparc64 machine, but probably on other 64 bits big endian machines too. For now I have the ugly patch below. Any better suggestion ? And there are other issues... --- src/XISelEv.c 4 Sep 2010 10:17:03 - 1.2 +++ src/XISelEv.c 27 Mar 2011 15:09:31 - @@ -32,6 +32,7 @@ in this Software without prior written a #include stdint.h +#include X11/Xarch.h #include X11/Xlibint.h #include X11/extensions/XI2proto.h #include X11/extensions/XInput2.h @@ -46,6 +47,7 @@ XISelectEvents(Display* dpy, Window win, XIEventMask *current; xXISelectEventsReq *req; xXIEventMask mask; +unsigned long long_mask; int i; int len = 0; int r = Success; @@ -83,7 +85,12 @@ XISelectEvents(Display* dpy, Window win, * and they need to be padded with 0 */ buff = calloc(1, mask.mask_len * 4); memcpy(buff, current-mask, current-mask_len); -Data32(dpy, mask, sizeof(xXIEventMask)); +#if X_BYTE_ORDER == X_BIG_ENDIAN + long_mask = mask.deviceid 16 | mask.mask_len; +#else + long_mask = mask.mask_len 16 | mask.deviceid; +#endif + Data32(dpy, long_mask, sizeof(xXIEventMask)); Data(dpy, buff, mask.mask_len * 4); free(buff); } -- Matthieu Herrb ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: Alignment error in libXi
On Thu, Mar 24, 2011 at 11:51:32PM -0700, Jeremy Huddleston wrote: Don't feel too bad. I missed it too and bugged Peter about it before I pushed the release ;) Unfortunatly, as Peter forsaid it, there are other issues. I'm currently fighting with the code for XISelectEvents(). It uses Data32() to send a xXIEventMask structure. This is very wrong and fails miserablily on sparc64, for at least 2 reasons: 1. Data32() suffers from the infamous 'use unsigned long for 32 bits data' API mis-design, so on 64 bits machines one can't pass a pointer to a 32 bit only data to Data32(): it has one chance over 2 to be mis-aligned, and will read 4 extra random bytes on the stack. 2. since xXIEventMask contains two uint_16 values, which are not properly swapped, this will not work across client/server with different endianness. This can all be seen whil trying Gtk+3's gtk3-demo on an OpenBSD/sparc64 machine, but probably on other 64 bits big endian machines too. For now I have the ugly patch below. Any better suggestion ? And there are other issues... --- src/XISelEv.c 4 Sep 2010 10:17:03 - 1.2 +++ src/XISelEv.c 27 Mar 2011 15:09:31 - @@ -32,6 +32,7 @@ in this Software without prior written a #include stdint.h +#include X11/Xarch.h #include X11/Xlibint.h #include X11/extensions/XI2proto.h #include X11/extensions/XInput2.h @@ -46,6 +47,7 @@ XISelectEvents(Display* dpy, Window win, XIEventMask *current; xXISelectEventsReq *req; xXIEventMask mask; +unsigned long long_mask; int i; int len = 0; int r = Success; @@ -83,7 +85,12 @@ XISelectEvents(Display* dpy, Window win, * and they need to be padded with 0 */ buff = calloc(1, mask.mask_len * 4); memcpy(buff, current-mask, current-mask_len); -Data32(dpy, mask, sizeof(xXIEventMask)); +#if X_BYTE_ORDER == X_BIG_ENDIAN + long_mask = mask.deviceid 16 | mask.mask_len; +#else + long_mask = mask.mask_len 16 | mask.deviceid; +#endif + Data32(dpy, long_mask, sizeof(xXIEventMask)); Data(dpy, buff, mask.mask_len * 4); free(buff); } -- Matthieu Herrb ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: Alignment error in libXi
On Fri, Mar 25, 2011 at 06:31:38 +, Matt Turner wrote: Oh, I didn't realize it had already been fixed. Odd that it was fixed, reviewed, and tested without another comment in this thread. Guess I just missed the patch. The followup with patch was only on -devel. Cheers, Julien ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: Alignment error in libXi
On Sun, Mar 13, 2011 at 10:11 PM, Christian Weisgerber na...@mips.inka.de wrote: GTK+3 blows up on LP64 archs with strict alignment. This comes down to an unaligned access error in libXi, specifically in the XIButtonClass case in copy_classes() in XExtInt.c. cls_lib-num_buttons = cls_wire-num_buttons; cls_lib-state.mask_len = cls_wire-num_buttons + 7)/8) + 3)/4) * 4; cls_lib-state.mask = next_block(ptr_lib, cls_lib-state.mask_len); memcpy(cls_lib-state.mask, cls_wire[1], cls_lib-state.mask_len); cls_lib-labels = next_block(ptr_lib, cls_lib-num_buttons * sizeof(Atom)); atoms =(uint32_t*)((char*)cls_wire[1] + cls_lib-state.mask_len); for (j = 0; j cls_lib-num_buttons; j++) cls_lib-labels[j] = *atoms++; It's the cls_lib-labels[j] = *atoms++ assignment that blows up. For state.mask, n*4 bytes are allocated from the ptr_lib area. labels is allocated immediately following that, but labels is an array of Atoms, which are longs, i.e, an 8-byte type on LP64 archs. labels can end up on an address that is misaligned for Atom assignments, like it did here: #0 0x0001616ea910 in copy_classes (to=0x165db5600, from=0x161bae020, nclasses=3) at /usr/xenocara/lib/libXi/src/XExtInt.c:1490 1490 cls_lib-labels[j] = *atoms++; (gdb) bt #0 0x0001616ea910 in copy_classes (to=0x165db5600, from=0x161bae020, nclasses=3) at /usr/xenocara/lib/libXi/src/XExtInt.c:1490 #1 0x0001616ecd80 in XIQueryDevice (dpy=0x16b964db0, deviceid=0, ndevices_return=0x1fffe44f0) at /usr/xenocara/lib/libXi/src/XIQueryDevice.c:90 #2 0x00016c30b6b0 in gdk_x11_device_manager_xi2_constructed ( object=0x16f8886a0) at gdkdevicemanager-xi2.c:413 #3 0x00016bf08488 in g_object_newv () from /usr/local/lib/libgobject-2.0.so.2800.0 #4 0x00016bf08a48 in g_object_new_valist () from /usr/local/lib/libgobject-2.0.so.2800.0 #5 0x00016bf07dec in g_object_new () from /usr/local/lib/libgobject-2.0.so.2800.0 #6 0x00016c308c10 in _gdk_x11_device_manager_new (display=0x164582000) at gdkdevicemanager-x11.c:59 #7 0x00016c310a0c in _gdk_x11_display_open (display_name=0x0) at gdkdisplay-x11.c:1228 #8 0x00016c30dff4 in gdk_x11_display_manager_open_display ( manager=0x1659b, name=0x0) at gdkdisplaymanager-x11.c:55 #9 0x00016c2d4130 in gdk_display_manager_open_display ( manager=0x1659b, name=0x0) at gdkdisplaymanager.c:362 #10 0x00016c2d29e4 in gdk_display_open (display_name=0x0) at gdkdisplay.c:1720 #11 0x00016c2c44b4 in gdk_display_open_default_libgtk_only () at gdk.c:341 #12 0x000168a9981c in gtk_init_check (argc=0x1fffe49a8, argv=0x1fffe49b0) at gtkmain.c:1132 #13 0x000168a99878 in gtk_init (argc=0x1fffe49a8, argv=0x1fffe49b0) at gtkmain.c:1184 #14 0x000120031094 in main (argc=1, argv=0x1fffe4a70) at main.c:948 (gdb) i loc cls_lib = (XIButtonClassInfo *) 0x165db5018 cls_wire = (xXIButtonInfo *) 0x161bae020 atoms = (uint32_t *) 0x161bae02c j = 0 any_lib = (XIAnyClassInfo *) 0x165db5018 any_wire = (xXIAnyInfo *) 0x161bae020 ptr_lib = (void *) 0x165db5094 ptr_wire = 0x161bae020 \001 i = 0 len = 0 (gdb) p cls_lib-labels $1 = (Atom *) 0x165db5044 (gdb) p sizeof(Atom) $2 = 8 (gdb) -- Christian naddy Weisgerber na...@mips.inka.de Good analysis. Is this one of those cases where someone, long ago, thought using longs was inherently good? Can we just change labels to be an array of ints, or is that too easy? There's even a comment in the code you quoted: /* Force mask alignment with longs to avoid unaligned * access when accessing the atoms. */ So much for that. Matt ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: Alignment error in libXi
On Mar 24, 2011, at 11:06 PM, Matt Turner wrote: Is this one of those cases where someone, long ago, thought using longs was inherently good? Can we just change labels to be an array of ints, or is that too easy? You can just get the latest libXi release with the fix. ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: Alignment error in libXi
On Fri, Mar 25, 2011 at 6:19 AM, Jeremy Huddleston jerem...@apple.com wrote: On Mar 24, 2011, at 11:06 PM, Matt Turner wrote: Is this one of those cases where someone, long ago, thought using longs was inherently good? Can we just change labels to be an array of ints, or is that too easy? You can just get the latest libXi release with the fix. Oh, I didn't realize it had already been fixed. Odd that it was fixed, reviewed, and tested without another comment in this thread. Guess I just missed the patch. Matt ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Re: Alignment error in libXi
Don't feel too bad. I missed it too and bugged Peter about it before I pushed the release ;) On Mar 24, 2011, at 11:31 PM, Matt Turner wrote: On Fri, Mar 25, 2011 at 6:19 AM, Jeremy Huddleston jerem...@apple.com wrote: On Mar 24, 2011, at 11:06 PM, Matt Turner wrote: Is this one of those cases where someone, long ago, thought using longs was inherently good? Can we just change labels to be an array of ints, or is that too easy? You can just get the latest libXi release with the fix. Oh, I didn't realize it had already been fixed. Odd that it was fixed, reviewed, and tested without another comment in this thread. Guess I just missed the patch. Matt ___ xorg-de...@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
Alignment error in libXi
GTK+3 blows up on LP64 archs with strict alignment. This comes down to an unaligned access error in libXi, specifically in the XIButtonClass case in copy_classes() in XExtInt.c. cls_lib-num_buttons = cls_wire-num_buttons; cls_lib-state.mask_len = cls_wire-num_buttons + 7)/8) + 3)/4) * 4; cls_lib-state.mask = next_block(ptr_lib, cls_lib-state.mask_len); memcpy(cls_lib-state.mask, cls_wire[1], cls_lib-state.mask_len); cls_lib-labels = next_block(ptr_lib, cls_lib-num_buttons * sizeof(Atom)); atoms =(uint32_t*)((char*)cls_wire[1] + cls_lib-state.mask_len); for (j = 0; j cls_lib-num_buttons; j++) cls_lib-labels[j] = *atoms++; It's the cls_lib-labels[j] = *atoms++ assignment that blows up. For state.mask, n*4 bytes are allocated from the ptr_lib area. labels is allocated immediately following that, but labels is an array of Atoms, which are longs, i.e, an 8-byte type on LP64 archs. labels can end up on an address that is misaligned for Atom assignments, like it did here: #0 0x0001616ea910 in copy_classes (to=0x165db5600, from=0x161bae020, nclasses=3) at /usr/xenocara/lib/libXi/src/XExtInt.c:1490 1490cls_lib-labels[j] = *atoms++; (gdb) bt #0 0x0001616ea910 in copy_classes (to=0x165db5600, from=0x161bae020, nclasses=3) at /usr/xenocara/lib/libXi/src/XExtInt.c:1490 #1 0x0001616ecd80 in XIQueryDevice (dpy=0x16b964db0, deviceid=0, ndevices_return=0x1fffe44f0) at /usr/xenocara/lib/libXi/src/XIQueryDevice.c:90 #2 0x00016c30b6b0 in gdk_x11_device_manager_xi2_constructed ( object=0x16f8886a0) at gdkdevicemanager-xi2.c:413 #3 0x00016bf08488 in g_object_newv () from /usr/local/lib/libgobject-2.0.so.2800.0 #4 0x00016bf08a48 in g_object_new_valist () from /usr/local/lib/libgobject-2.0.so.2800.0 #5 0x00016bf07dec in g_object_new () from /usr/local/lib/libgobject-2.0.so.2800.0 #6 0x00016c308c10 in _gdk_x11_device_manager_new (display=0x164582000) at gdkdevicemanager-x11.c:59 #7 0x00016c310a0c in _gdk_x11_display_open (display_name=0x0) at gdkdisplay-x11.c:1228 #8 0x00016c30dff4 in gdk_x11_display_manager_open_display ( manager=0x1659b, name=0x0) at gdkdisplaymanager-x11.c:55 #9 0x00016c2d4130 in gdk_display_manager_open_display ( manager=0x1659b, name=0x0) at gdkdisplaymanager.c:362 #10 0x00016c2d29e4 in gdk_display_open (display_name=0x0) at gdkdisplay.c:1720 #11 0x00016c2c44b4 in gdk_display_open_default_libgtk_only () at gdk.c:341 #12 0x000168a9981c in gtk_init_check (argc=0x1fffe49a8, argv=0x1fffe49b0) at gtkmain.c:1132 #13 0x000168a99878 in gtk_init (argc=0x1fffe49a8, argv=0x1fffe49b0) at gtkmain.c:1184 #14 0x000120031094 in main (argc=1, argv=0x1fffe4a70) at main.c:948 (gdb) i loc cls_lib = (XIButtonClassInfo *) 0x165db5018 cls_wire = (xXIButtonInfo *) 0x161bae020 atoms = (uint32_t *) 0x161bae02c j = 0 any_lib = (XIAnyClassInfo *) 0x165db5018 any_wire = (xXIAnyInfo *) 0x161bae020 ptr_lib = (void *) 0x165db5094 ptr_wire = 0x161bae020 \001 i = 0 len = 0 (gdb) p cls_lib-labels $1 = (Atom *) 0x165db5044 (gdb) p sizeof(Atom) $2 = 8 (gdb) -- Christian naddy Weisgerber na...@mips.inka.de ___ xorg@lists.freedesktop.org: X.Org support Archives: http://lists.freedesktop.org/archives/xorg Info: http://lists.freedesktop.org/mailman/listinfo/xorg Your subscription address: arch...@mail-archive.com
