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

[Uli Schlachter]

On 16.11.2013 22:37, Jonas Petersen wrote:
> By design the Xlib 32-bit internal request sequence numbers may wrap. There
> is two locations within xcb_io.c that are not wrap-safe. The value of
> last_flushed relies on the request to be sequential all the time. This is
> not given 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.
> 
> Signed-off-by: Jonas Petersen <jnsptrsn1 at gmail.com>
> ---
>  src/xcb_io.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/src/xcb_io.c b/src/xcb_io.c
> index 727c6c7..f2978d0 100644
> --- a/src/xcb_io.c
> +++ b/src/xcb_io.c
> @@ -455,7 +455,7 @@ void _XSend(Display *dpy, const char *data, long size)
>  	static const xReq dummy_request;
>  	static char const pad[3];
>  	struct iovec vec[3];
> -	uint64_t requests;
> +	uint64_t requests, unwrapped_request;
>  	_XExtension *ext;
>  	xcb_connection_t *c = dpy->xcb->connection;
>  	if(dpy->flags & XlibDisplayIOError)
> @@ -464,6 +464,10 @@ void _XSend(Display *dpy, const char *data, long size)
>  	if(dpy->bufptr == dpy->buffer && !size)
>  		return;
>  
> +	/* 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.

> +	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 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.)

>  	/* iff we asked XCB to set aside errors, we must pick those up
>  	 * eventually. iff there are async handlers, we may have just
>  	 * issued requests that will generate replies. in either case,
> @@ -471,10 +475,10 @@ void _XSend(Display *dpy, const char *data, long size)
>  	if(dpy->xcb->event_owner != XlibOwnsEventQueue || dpy->async_handlers)
>  	{
>  		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);
>  	}
> -	requests = dpy->request - dpy->xcb->last_flushed;
> +	requests = unwrapped_request - dpy->xcb->last_flushed;
>  	dpy->xcb->last_flushed = dpy->request;
>  
>  	vec[0].iov_base = dpy->buffer;
> 


-- 
"Every once in a while, declare peace. It confuses the hell out of your enemies"
 - 79th Rule of Acquisition