[PATCH libX11] fix for Xlib 32-bit request number issues

[Olivier Fourdan]

Hi all,

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?

Cheers,
Olivier

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

Hi Christian,

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 
following ideas:

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 
IntAtom.c

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.

tl;dr:

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 :-)

Cheers,
Olivier



_______________________________________________
xorg-devel at lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel