[Xcb] [PATCH libX11] xcb_io: Fix Xlib 32-bit request number wrapping bug

[Keith Packard]

Jonas Petersen <jnsptrsn1 at gmail.com> writes:

> By design, on 32-bit systems, the Xlib internal 32-bit request sequence
> numbers may wrap. There is two locations within xcb_io.c that are not
> wrap-safe though. The value of last_flushed relies on request to be
> sequential all the time. This is not given in the moment when the sequence
> has just wrapped. Applications may then crash with a "Fatal IO error 11
> (Resource temporarily unavailable)".
>
> This patch fixes this by "unwrapping" the sequence number when needed to
> retain the sequence relative to last_flushed.

Reviewed-by: Keith Packard <keithp at keithp.com>

There are some subtleties here which might be mentioned in comments:

> +	unwrapped_request = dpy->request;
> +	/* If there was a sequence number wrap since our last flush,
> +	 * make sure the sequence number we use, stays in sequence
> +	 * with dpy->xcb->last_flush. */
> +	if (sizeof(uint64_t) > sizeof(unsigned long) && dpy->request < dpy->xcb->last_flushed)
> +		unwrapped_request += UINT64_C(1) << 32;

The sizeof(uint64_t) > sizeof (unsigned long) test is an optimization;
dpy->request will *never* be less than last_flushed on a 64-bit system
(well, until we have to deal with 64-bit wrap). However, the check is
also useful to understand the append_pending_request call below.

>  		uint64_t sequence;
> -		for(sequence = dpy->xcb->last_flushed + 1; sequence <= dpy->request; ++sequence)
> +		for(sequence = dpy->xcb->last_flushed + 1; sequence <= unwrapped_request; ++sequence)
>  			append_pending_request(dpy, sequence);

Here, we're passing a 64-bit sequence which may be 'unwrapped', in that
it can contain values above the maximum possible unsigned long sequence
number. However, that only happens (see check above) where unsigned long
is 32 bits. The 'sequence' formal in append_pending_request is an
unsigned long, and so any time the sequence passed might have a high
sequence value, it will get trimmed off by the type conversion.

Do we want an '(unsigned long)' cast in the actual here? It would be
strictly for documentation. Probably best to just mention what's
happening in a comment instead



>  	}
> -	requests = dpy->request - dpy->xcb->last_flushed;
> +	requests = unwrapped_request - dpy->xcb->last_flushed;
>  	dpy->xcb->last_flushed = dpy->request;

I think last_flushed should be changed from uint64_t to unsigned long to
match all of the Xlib types. That would require a cast in the loop
above:

+	for(sequence = (uint64_t) dpy->xcb->last_flushed + 1; sequence <= unwrapped_request; ++sequence)

I think there's also a problem in require_socket -- it compares a 64-bit
value from xcb with a 32-bit local value. I think we need to change that
to:

        uint64_t        sent64;
        unsigned long   sent;

        sent = sent64

        if ((long) (sent - dpy->last_request_read) < 0)
                throw_thread_fail_assert("sequence wrapped")

This would only allow for 31-bit differences.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20131230/d4f1e7da/attachment.pgp>