| Message ID | 1387148814-22819-1-git-send-email-jnsptrsn1@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
diff --git a/src/xcb_io.c b/src/xcb_io.c index 727c6c7..34ca46e 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,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 +478,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;
Jonas Petersen <jnsptrsn1@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@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.
Hi Keith, I went back in time and found your last post regarding this topic. You did make some suggestions that got lost, probably due to my discontinuing. Sorry about that. So let me pick it up again from there, in order to hopefully get the case going. I will post an updated patch including your suggestions. Please see my inline comments in this mail, particularly on your last point. I tested the patch against a fresh Ubuntu Desktop 14.04.1 i386 (libx11-1.6.2). I verified that your test (nop.c) will fail before and succeed after applying the patch. Am 30.12.2013 um 21:04 schrieb Keith Packard: > Jonas Petersen<jnsptrsn1@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@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. Yes, it's an optimization. It will have an effect at compile time and it will make it end up only in 32-bit binaries. Actually there was some discussion about that at some other branch of this topic. > >> 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 Agreed, I will add a comment. >> } >> - 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. By examining the usages of last_flushed, I'd say it makes a lot of sense changing it to unsigned long. Added it to the patch. > That would require a cast in the loop > above: > > + for(sequence = (uint64_t) dpy->xcb->last_flushed + 1; sequence <= unwrapped_request; ++sequence) Added that as well. > 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. Here I'm not sure if I really get what you mean. So you would introduce another local variable instead of casting where appropriate? Please see my patch if I applied that right. By the way, the line following next... dpy->xcb->last_flushed = dpy->request = sent; ...now also is much more obvious. Because before it was a 64-32-64-bit assignment. Now it is 32-32-32. In my ealier patches I had added a comment to that line (which now would be obsolete): /* Note: The following line will truncate the 64bit 'sent' * to 32bit for 'dpy->request'. This truncated value will * then be assigned to the 64-bit 'dpy->xcb->last_flushed' * (which is intended). */ dpy->xcb->last_flushed = dpy->request = sent; Regards Jonas
Hi Keith, (2nd try) I went back in time and found your last post regarding this topic. You did make some suggestions that got lost, probably due to my discontinuing. Sorry about that. So let me pick it up again from there, in order to hopefully get the case going. I will post an updated patch including your suggestions. Please see my inline comments in this mail, particularly on your last point. I tested the patch against a fresh Ubuntu Desktop 14.04.1 i386 (libx11-1.6.2). I verified that your test (nop.c) will fail before and succeed after applying the patch. Am 30.12.2013 um 21:04 schrieb Keith Packard: > Jonas Petersen<jnsptrsn1@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@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. Yes, it's an optimization. It will have an effect at compile time and it will make it end up only in 32-bit binaries. Actually there was some discussion about that at some other branch of this topic. > >> 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 Agreed, I will add a comment. >> } >> - 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. By examining the usages of last_flushed, I'd say it makes a lot of sense changing it to unsigned long. Added it to the patch. > That would require a cast in the loop > above: > > + for(sequence = (uint64_t) dpy->xcb->last_flushed + 1; sequence <= unwrapped_request; ++sequence) Added that as well. > 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. Here I'm not sure if I really get what you mean. So you would introduce another local variable instead of casting where appropriate? Please see my patch if I applied that right. By the way, the line following next... dpy->xcb->last_flushed = dpy->request = sent; ...now also is much more obvious. Because before it was a 64-32-64-bit assignment. Now it is 32-32-32. In my ealier patches I had added a comment to that line (which now would be obsolete): /* Note: The following line will truncate the 64bit 'sent' * to 32bit for 'dpy->request'. This truncated value will * then be assigned to the 64-bit 'dpy->xcb->last_flushed' * (which is intended). */ dpy->xcb->last_flushed = dpy->request = sent; Regards Jonas
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. Signed-off-by: Jonas Petersen <jnsptrsn1@gmail.com> --- src/xcb_io.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)