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

[Jan Smout]

Keith Packard doesn't seem very responsive (as in 'completely ignoring the
subject')

Thank you for persisting, Jonas.

On 24 September 2014 21:13, Jonas Petersen <jnsptrsn1 at gmail.com> wrote:

> By design, on 32-bit systems, the Xlib internal 32-bit request sequence
> numbers may wrap. There is some 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. It also contains some
> additional optimizations.
>
> Signed-off-by: Jonas Petersen <jnsptrsn1 at gmail.com>
> ---
>  src/Xxcbint.h |  2 +-
>  src/xcb_io.c  | 27 +++++++++++++++++++++------
>  2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/src/Xxcbint.h b/src/Xxcbint.h
> index bf41c23..feee775 100644
> --- a/src/Xxcbint.h
> +++ b/src/Xxcbint.h
> @@ -31,7 +31,7 @@ typedef struct _X11XCBPrivate {
>         char *reply_data;
>         int reply_length;
>         int reply_consumed;
> -       uint64_t last_flushed;
> +       unsigned long last_flushed;
>         enum XEventQueueOwner event_owner;
>         XID next_xid;
>
> diff --git a/src/xcb_io.c b/src/xcb_io.c
> index 5987329..03af1f9 100644
> --- a/src/xcb_io.c
> +++ b/src/xcb_io.c
> @@ -59,15 +59,19 @@ static void require_socket(Display *dpy)
>  {
>         if(dpy->bufmax == dpy->buffer)
>         {
> -               uint64_t sent;
> +               uint64_t sent64;
> +               unsigned long sent;
>                 int flags = 0;
>                 /* if we don't own the event queue, we have to ask XCB
>                  * to set our errors aside for us. */
>                 if(dpy->xcb->event_owner != XlibOwnsEventQueue)
>                         flags = XCB_REQUEST_CHECKED;
>                 if(!xcb_take_socket(dpy->xcb->connection, return_socket,
> dpy,
> -                                   flags, &sent))
> +                                   flags, &sent64))
>                         _XIOError(dpy);
> +
> +               sent = sent64;
> +
>                 /* Xlib uses unsigned long for sequence numbers.  XCB
>                  * uses 64-bit internally, but currently exposes an
>                  * unsigned int API.  If these differ, Xlib cannot track
> @@ -77,7 +81,7 @@ static void require_socket(Display *dpy)
>                  * 64-bit sequence numbers. */
>                 if (sizeof(unsigned long) > sizeof(unsigned int) &&
>                     dpy->xcb->event_owner == XlibOwnsEventQueue &&
> -                   (sent - dpy->last_request_read >= (UINT64_C(1) <<
> 32))) {
> +                   (long) (sent - dpy->last_request_read) < 0) {
>                         throw_thread_fail_assert("Sequence number wrapped "
>                                                  "beyond 32 bits while
> Xlib "
>                                                  "did not own the socket",
> @@ -455,7 +459,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 +468,13 @@ void _XSend(Display *dpy, const char *data, long size)
>         if(dpy->bufptr == dpy->buffer && !size)
>                 return;
>
> +       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;
> +
>         /* 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 +482,14 @@ 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 = (uint64_t) dpy->xcb->last_flushed + 1;
> sequence <= unwrapped_request; ++sequence)
> +                       /* On systems where unsigned long is 32 bits, the
> 64-bit sequence
> +                        * passed to append_pending_request might get
> trimmed off.
> +                        * This is logically correct and expected, as it's
> simply
> +                        * 're-wrapping' the 'unwrapped' sequence number.
> */
>                         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;
> --
> 1.9.1
>
> _______________________________________________
> 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
>



-- 
Life is complex, it has a real part and an imaginary part.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/xcb/attachments/20140926/e4308fd6/attachment.html>