[PATCH libX11] fix for Xlib 32-bit request number issues
ofourdan at redhat.com
Mon Apr 13 23:58:22 PDT 2015
Anyone got a chance to look further into this? (this is an old bug with several unsuccessful attempted fixes)
Chris, what do you think of the wrap test for the sequence that must remain in 32bits?
----- Original Message -----
From: "Olivier Fourdan" <ofourdan at redhat.com>
To: "Christian Linhart" <chris at demorecorder.com>, xorg-devel at lists.x.org
Sent: Tuesday, March 31, 2015 2:22:11 PM
Subject: Re: [PATCH libX11] fix for Xlib 32-bit request number issues
On 27/03/15 15:50, Olivier Fourdan wrote:
> On 21/03/15 15:32, Christian Linhart wrote:
> I am not a libX11 maintainer, so it's not really a review or anything,
> it's just some comments :-)
So I looked further into your patch (took me some time!) and came to the
1. The changes in Xlib.h are not needed as we already have LONG64
defined for the same purpose and already used in different place in libX11.
2. As a result, the changes in Xlibint.h would rely on LONG64 being
defined (or not) instead of X_UNSIGNED_LONG_IS_32BIT
3. For the _XAsyncErrorState's "min_sequence_number" and
"max_sequence_number", if we cannot change those to be 64bit, then we
ought to use a different technique as assigning these 64bit values will
make no difference.
Thankfully, these are used in comparison, so we can check for a wrap
using something like that for the 32bit version:
#define SEQUENCE_CMP(a,op,b) \
(((a op b) && (b - a op (UINT32_MAX >> 1))) || \
((b op a) && ((UINT32_MAX >> 1) op a - b)))
Which (if my maths are correct) is supposed to compare "a" and "b"
within a 32bit floating window, which is more than enough, as I would
not expect the 32bit wrapping to occur more than once between a request
and its async reply.
That macro is defined and used in XlibAsync.c
4. For the code that uses the standard _XAsyncErrorState struct, there
is no need to use 64bit extended sequence number so the macro above
would work without changing the code - The list of sources is this case
is Fonts.c, ReconfWM.c and obviously XlibAsync.c
5. For others that use their own struct, these can use 64bit sequence
number and therefore benefit from your change, that's GetWAttrs.c and
6. There are some unnecessary changes in your patch that seem unrelate
to the issue being addressed, like for example renaming "sent" as
"xcb_sent" in require_socket() from src/xcb_io.c or splitting the for()
loop in 4 lines in _XSend() from src/xcb_io.c - These are cosmetic
changes and, if really needed, should be part of a separate patch IMHO.
7. These changes will rely on xcb exposing the 64bit sequence number, ie
an API addition in libxcb, so you will need to update the libxcb
required version in configure.ac:
X11_REQUIRES='xproto >= 7.0.17 xextproto xtrans xcb >= 1.1.92'
X11_EXTRA_DEPS="xcb >= 1.1.92"
But obviously that needs to be done once the patches in libxcb are
merged, so this is left out for now.
I am attaching a "git diff" against current git master's head so you can
see what I mean (code is usually easier than a lengthy explanation) - I
leave it to you to decide and send a new patch with "git am" if you
think it's correct (or at least moving in the right direction) - I don't
want to step on your toes here, just trying to help :-)
xorg-devel at lists.x.org: X.Org development
More information about the xorg-devel