[PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping

[Jonas Petersen]

Am 17.11.2013 11:58, schrieb Uli Schlachter:
> On 16.11.2013 22:37, Jonas Petersen wrote:
>> +	/* Set bit 8 of 'request' when a 32-bit wrap has just happened
>> +	 * so the sequence stays correct relatively to 'last_flushed'. */
> "1 << 32" does not set bit 8 and this doesn't set anything in request, really.

You're right, sorry, "bit 8" is a typo, it's bit 32 of course. Other 
than that it will indeed set something. Not exactly "in request", but it 
will end up in unwrapped_request.

>
>> +	unwrapped_request = ((uint64_t)(dpy->request < dpy->xcb->last_flushed) << 32) + dpy->request;
> Also, I don't think that this code is intuitive this way.

I agree somewhat on that. I thought efficiency would have precedence 
here. I was actually inspired by static void widen() in xcb_io.c where 
it reads:

   *wide = new + ((unsigned long) (new < *wide) << 16 << 16);

The comment says "Treating the comparison as a 1 and shifting it avoids 
a conditional branch".


> I would propose this instead:
>
>    unwrapped_request = dpy->request;
>    /* If there was a sequence number wrap since our last flush, make sure we
>     * use the correct 64 bit sequence number */
>    if (sizeof(uint64_t) > sizeof(unsigned long)
>           && dpy->request < dpy->xcb_last_flushed)
>       unwrapped_request += UINT64_C(1) << 32;
>
> (I am not sure/convinced if the sizeof comparision is necessary, but I saw
> something like this in require_socket() and then thought that this might be
> necessary on systems where unsigned long already is a 64 bit type.)

I admit I haven't tought it all through on other system configurations. 
So thanks for giving your input.
I guess the sizeof comparison would not be necessary since the condition 
should never meet with 64-bit longs. And if it does, something is wrong 
anyway (from my understanding). After all this is the trigger of the bug.

Btw. as you might have guessed, on my system unsigned longs are 32-bit.

- Jonas


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.x.org/archives/xorg-devel/attachments/20131117/46037522/attachment.html>