[PATCH libX11] xcb_io: Fix Xlib 32-bit request number issues
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 jnsptr...@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@lists.x.org: X.Org development
Archives: http://lists.x.org
[PATCH libX11] xcb_io: Fix Xlib 32-bit request number issues
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 jnsptr...@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@lists.x.org: X.Org development
Archives: http://lists.x.org
Re: [Xcb] [PATCH libX11] xcb_io: Fix Xlib 32-bit request number wrapping bug
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 Petersenjnsptr...@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 Packardkei...@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_tsent64; 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 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [Xcb] [PATCH libX11] xcb_io: Fix Xlib 32-bit request number wrapping bug
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 Petersenjnsptr...@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 Packardkei...@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_tsent64; 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 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11] xcb_io: Fix Xlib 32-bit request number issues
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 jnsptr...@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@lists.x.org: X.Org development
Archives: http://lists.x.org
Re: libX11: 32-bit request number wrapping bug
Am 22.08.2014 um 09:22 schrieb Jan Smout: I cannot stress enough the seriousness of this bug. An application that gets killed by a library without any way to recover is not something to be taken lightly. In a certain sense I got lucky that my app is so heavy on graphics, so I was able to find a solution rather quickly (as did Jonas btw). I suspect most people just see (seemingly) random crashes, shrug and restart whatever they were running and then blame the application programmer for writing shitty software. I find that hard to accept... I totally agree with this! I fact, in the beginning I was blaming my own application software for this. It took me quite a while and lots of effort to realize it's down there deep in the libraries of the os. And being able to reproduce the bug in only 20 hours or so, does not help a lot. Even though, due to heavy graphics, this was already kind of fast. Regards Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [Xcb] [PATCH libX11] xcb_io: Fix Xlib 32-bit request number wrapping bug
Hi Jan,
thanks for pushing this.
I spent really a lot of time (weeks) tracking this down and finding a
solution. Digging down the depths of the operating system, while
actually writting application software. The result is the mentioned
patch. I then posted it here. I think there is approval that the fix
actually does work. Then there was starting some discussion about
implementation details, optimization and possible further problems at
other locations.
At some point I had to take a break, since this had cost me already so
much time. Sorry about that. It's to bad this is still pending.
If nothing happens I might be willing to spend another small amount of
time to help completing this. But my time is limited. I can not promise
anything.
I think this bug is quite serious. It suddenly kills programs without
asking out of nowhere. And it's patient.
By the way, my software now runs on 64-bit, so luckily I'm not affected
anymore (hopefully). But there's probably still plenty of 32-bit systems
endangered by this.
Have you seen? Keith posted a program to reproduce the bug (or confirm
that the patch works) as fast as possible:
/* cc -o nop nop.c `pkg-config --cflags --libs x11` */
#includestdio.h
#includestdint.h
#includeX11/Xlib.h
int
main (int argc, char **argv)
{
uint64_ti = 0;
Display *dpy = XOpenDisplay(NULL);
for (;;) {
++i;
if ((i 0xfff) == 0) {
XFlush(dpy);
printf (0x%llx\n, i);
}
XNoOp(dpy);
}
}
Regards
Jonas
Am 29.07.2014 um 18:56 schrieb Jan Smout:
Hi all,
I recently stumbled into an application that crashed because of this:
https://bugs.freedesktop.org/show_bug.cgi?id=71338
and quickly found the following patch:
http://patchwork.freedesktop.org/patch/16753/
which seems to work fine (the application used to crash in less than
24 hrs. Has been running for 5 days straight with the patch).
Now my question: what is the status of this patch? Are there still
details to be clarified before it can be put into the main tree?
best regards,
Jan
--
Life is complex, it has a real part and an imaginary part.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11] xcb_io: Fix Xlib 32-bit request number wrapping bug
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 jnsptr...@gmail.com
---
src/xcb_io.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
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;
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11] xcb_io: Fix Xlib 32-bit request number wrap bug
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 jnsptr...@gmail.com
---
src/xcb_io.c | 13 ++---
1 file changed, 10 insertions(+), 3 deletions(-)
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;
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
Am 18.11.2013 19:16, schrieb Julien Cristau:
On Sun, Nov 17, 2013 at 21:30:44 +0100, Jonas Petersen wrote:
Am 17.11.2013 20:20, schrieb Mouse:
I guess the sizeof comparison would not be necessary since the
condition should never meet with 64-bit longs.
Unless it's in a code fragment that's used only on machines with
64-bit longs, it will; X runs on systems with 64-bit longs.
I meant the condition dpy-request dpy-xcb_last_flushed should
not meet on systems with 64-bit longs (at least not until the 64-bit
wrap). So the sizeof(uint64_t) sizeof(unsigned long) would not
be necessary. It would just increase the overhead on 64-bit
systems.
The sizeof comparison is a compile-time constant so I'd expect it to be
optimized out by the compiler anyway, so no overhead.
This makes sense. Being curious, I did some tests (with gcc 4.7.2 on
i686). They showed that this code:
int i = 0;
for (;;) {
++i;
if (sizeof(unsigned long) sizeof(unsigned int) i 100) {
printf(%d\n, i);
}
}
on a 32-bit system compiles into the same binary as this code:
for (;;) {
}
and on a 64-bit system it compiles into the same binary as this code:
int i = 0;
for (;;) {
++i;
if (i 100) {
printf(%d\n, i);
}
}
All that without any optimization options.
So adding the size comparison seems to not only not increasing overhead,
but even effectively optimizing for the targets. At least with this
compiler on these architectures.
- Jonas
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
Am 18.11.2013 00:33, schrieb Mouse: *wide = new + ((unsigned long) (new *wide) 16 16); The comment says Treating the comparison as a 1 and shifting it avoids a conditional branch. Only on architectures with conditional moves - and, on those, the version using ? : is likely to compile down to a conditional move anyway. I think that comment should be fixed. What do you mean exactly? Computing the numeric value of (a b) - as opposed to using it in a control-flow construct - is normally implemented much the same way that ((a b) ? 1 : 0) is. By definition of the operator, they are semantically identical anyway. I thought the comment was saying that using (x y) z is avoiding a conditinal branch introduced by using an if or ? : in some way to get the calculation done. That's what I think the comment is saying, yes. I also think it's false - or, at least, it's false more often than it's true, and in a way that's unpredictable from the point of view of anyone who actually needs the comment. On architectures without conditional moves, it's difficult[%] to compute the numerical value of (x y) without a conditional branch. And on architectures with conditional moves, ((x y) ? 1 : 0) will likely use a conditional move rather than a branch anyway. Isn't the comment true then? It depends on the compiler, the target CPU architecture, and possibly other things (like the optimization tradeoffs the compiler has been told to use). But if the optimizer is even half-decent, (x y) z and ((x y) ? 1 : 0) z will compile to exactly the same instructions. I think it is likely enough that it's false that leaving the comment there with that wording is a bad idea; I also see it as a instance of pushing a very very machine-dependent tradeoff up to the C level, something that is better left to the optimizer, or at the very least buried in a machine-dependent preprocessor macro. [%] Not impossible; it can be done with arithemtic and logical operations. I'd render the core of the idea in 32-bit C as ((x-y)31)1 - that's got issues with numbers large enough that their high few bits aren't all the same, but they can be fixed with a little more code. Thanks for your explanation. I see what you mean. I guess it was just from a pure (C) code level point of view then, considering if or ?: as conditional branch as is, without regarding what's happening at the lower levels when compiling. And possibly not regarding the meaning of conditional branch in other contexts. So I guess I will revoke my inspiration then and go for Ulis proposal (using and if including the size comparison). - Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
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 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 2/2] xcb_io: Add comment explaining a mixed type double assignment
Am 17.11.2013 14:06, schrieb Uli Schlachter: Hi, On 16.11.2013 22:37, Jonas Petersen wrote: @@ -83,6 +83,10 @@ static void require_socket(Display *dpy) did not own the socket, xcb_xlib_seq_number_wrapped); } + /* The following line will truncate the 64-bit 'sent' +* to 32-bit when assigning it to 'dpy-request'. The +* truncated value will then be assigned to the 64-bit +* 'dpy-xcb-last_flushed' (which is intended). */ dpy-xcb-last_flushed = dpy-request = sent; dpy-bufmax = dpy-xcb-real_bufmax; } The involved types are: uint64_t sent; uint64_t last_flushed; unsigned long request; So request isn't really a 32-bit type. In fact, on my system, all of these three are the same type. So the comment doesn't really fit. I agree. The comment only applies to systems with 32-bit longs (like mine). Maybe the comment should start out with something like On systems with 32-bit unsigned longs the following line Now let's assume that unsigned long is a 32-bit integer. *Why* is it intended to truncate last_flushed? This means that Xlib uses a 64 bit integer for to tracking the sequence number, but only actually tracks it with 32 bits. This seems wasteful. Why are you sure that this is intended? Well, because when the 32-bit request wraps (which it may), then last_flushed must reflect the wrapped values because they get refered to each other (which fails in the moment when the wrap happens, which then results in the crash). Apart from that I was writing intended because it wouldn't work otherwise. I was hitting this at first when debugging the issue. I thought: damn it's truncating the request, this must be the bug. But it wasn't. Maybe it wasn't strictly speaking intended in first place and it simply works by accident. Which would be on more reason to put some comment clarifying the matter. Separating the assignments might be another measure. I'm glad for any enlightenment in case I'm totally wrong with all of my theories. - Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 0/2] Xlib 32-bit sequence number wrap bug(fix)
Am 17.11.2013 09:26, schrieb Keith Packard:
Jonas Petersen jnsptr...@gmail.com writes:
It might take one or two hours, but when it reaches the 4294 million it will
explode into a Fatal IO error 11.
This reproduces the bug in about 15 minutes on my machine:
/* cc -o nop nop.c `pkg-config --cflags --libs x11` */
#includestdio.h
#includestdint.h
#includeX11/Xlib.h
int
main (int argc, char **argv)
{
uint64_ti = 0;
Display *dpy = XOpenDisplay(NULL);
for (;;) {
++i;
if ((i 0xfff) == 0) {
XFlush(dpy);
printf (0x%llx\n, i);
}
XNoOp(dpy);
}
}
Great, thanks! I was spending some time to find a way to reproduce it as
fast as possible. Haven't thought of XNoOp. I get it in 25 minutes now.
The fastest I got before was 70 minutes.
- Jonas
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 0/2] Xlib 32-bit sequence number wrap bug(fix)
Am 16.11.2013 23:30, schrieb Alan Coopersmith: cc'ing the xcb mailing list, since more people there know about how the Xlib/xcb interaction in xcb_io works. For the people there who didn't see the patches on xorg-devel, you can find them at: http://patchwork.freedesktop.org/patch/15501/ http://patchwork.freedesktop.org/patch/15500/ Thanks! The other replies did not cc the xcb list though. So there is some conversation happening on xorg-devel only now. Should I submit the patch the next time to both lists right away? (It's likely I have to resubmit.) - Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
Am 17.11.2013 20:20, schrieb Mouse: I guess the sizeof comparison would not be necessary since the condition should never meet with 64-bit longs. Unless it's in a code fragment that's used only on machines with 64-bit longs, it will; X runs on systems with 64-bit longs. I meant the condition dpy-request dpy-xcb_last_flushed should not meet on systems with 64-bit longs (at least not until the 64-bit wrap). So the sizeof(uint64_t) sizeof(unsigned long) would not be necessary. It would just increase the overhead on 64-bit systems. And if it does, something is wrong anyway (from my understanding). After all this is the trigger of the bug. Does anyone know whether the bug triggers on systems with 64-bit longs? I would say no, because there is no 32-bit to wrap. But who knows, I have not tried it. - Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
Am 17.11.2013 21:30, schrieb Jonas Petersen: Am 17.11.2013 20:20, schrieb Mouse: And if it does, something is wrong anyway (from my understanding). After all this is the trigger of the bug. Does anyone know whether the bug triggers on systems with 64-bit longs? I would say no, because there is no 32-bit to wrap. But who knows, I have not tried it. I have just run a test on a blank 64-bit Ubuntu 13.10. It shows sizeof(unsigned long) == 8. The bug does *not* trigger. - Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
Am 17.11.2013 20:20, schrieb Mouse: *wide = new + ((unsigned long) (new *wide) 16 16); The comment says Treating the comparison as a 1 and shifting it avoids a conditional branch. Only on architectures with conditional moves - and, on those, the version using ? : is likely to compile down to a conditional move anyway. I think that comment should be fixed. What do you mean exactly? I thought the comment was saying that using (x y) z is avoiding a conditinal branch introduced by using an if or ? : in some way to get the calculation done. Isn't the comment true then? - Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 0/2] Xlib 32-bit sequence number wrap bug(fix)
Hi,
I'm now submitting this the third time. I think it's an important issue and
it needs to be reviewed. Please someone take a look at it, there's basically
just 3 lines of code involved.
If anybody wants more information, code to reproduce the failure, or changes
in the patch I submitted, please tell me.
--
Here's two patches. The first one fixes a 32-bit sequence wrap bug. The
second patch only adds a comment to another relevant statement.
The patches contain some details. Here is the whole story for who might be
interested:
Xlib (libx11) will crash an application with a Fatal IO error 11 (Resource
temporarily unavailable) after 4 294 967 296 requests to the server. That is
when the Xlib internal 32-bit sequence wraps.
Most applications probably will hardly reach this number, but if they do,
they have a chance to die a mysterious death. For example the application I'm
working on did always crash after about 20 hours when I started to do some
stress testing. It does some intensive drawing through Xlib using gktmm2,
pixmaps and gc drawing at 40 frames per second in full hd resolution (on
Ubuntu). Some optimizations did extend the grace to about 35 hours but it
would still crash.
What then followed was some frustrating weeks of digging and debugging to
realize that it's not in my application, nor in gtkmm, gtk or glib but that
it's this little bug in Xlib which exists since 2006-10-06 apparently.
It took a while to turn out that the number 0x1 (2^32) has some
relevance. (Much) later it turned out it can be reproduced with Xlib only,
using this code for example:
while(1) {
XDrawPoint(display, drawable, gc, x, y);
XFlush(display);
}
It might take one or two hours, but when it reaches the 4294 million it will
explode into a Fatal IO error 11.
What I then learned is that even though Xlib uses internal 32bit sequence
numbers they get (smartly) widened to 64bit in the process so that the 32bit
sequence may wrap without any disruption in the widened 64bit sequence.
Obviously there must be something wrong with that.
The Fatal IO error is issued in _XReply() when it's not getting a reply where
there should be one, but the cause is earlier in _XSend() in the moment when
the Xlib 32-bit sequence number wraps.
The problem is that when it wraps to 0, the value of 'last_flushed' will
still be at the upper boundary (e.g. 0x). There is two locations in
_XSend() (xcb_io.c) that fail in this state because they rely on those values
being sequential all the time, the first location is:
requests = dpy-request - dpy-xcb-last_flushed;
I case of request = 0x0 and last_flushed = 0x it will assign
0x0001 to 'requests' and then to XCB as a number (amount) of
requests. This is the main killer.
The second location is this:
for(sequence = dpy-xcb-last_flushed + 1; sequence = dpy-request; \
++sequence)
I case of request = 0x0 (less than last_flushed) there is no chance to enter
the loop ever and as a result some requests are ignored.
The solution is to unwrap dpy-request at these two locations and thus
retain the sequence related to last_flushed.
uint64_t unwrapped_request = ((uint64_t)(dpy-request \
dpy-xcb-last_flushed) 32) + dpy-request;
It creates a temporary 64-bit request number which has bit 8 set if 'request'
is less than 'last_flushed'. It is then used in the two locations instead of
dpy-request.
I'm not sure if it might be more efficient to use that statement inplace,
instead of using a variable.
There is another line in require_socket() that worried me at first:
dpy-xcb-last_flushed = dpy-request = sent;
That's a 64-bit, 32-bit, 64-bit assignment. It will truncate 'sent' to 32-bit
when assinging it to 'request' and then also assign the truncated value to
the (64-bit) 'last_flushed'.
But it seems inteded. I have added a note explaining that for the next poor
soul debugging sequence issues... :-)
- Jonas
Jonas Petersen (2):
xcb_io: Fix Xlib 32-bit request number wrapping
xcb_io: Add comment explaining a mixed type double assignment
src/xcb_io.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
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 jnsptr...@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'. */
+ unwrapped_request = ((uint64_t)(dpy-request dpy-xcb-last_flushed)
32) + dpy-request;
+
/* 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;
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 2/2] xcb_io: Add comment explaining a mixed type double assignment
The assignment might be confusing at first. So I added a note. Signed-off-by: Jonas Petersen jnsptr...@gmail.com --- src/xcb_io.c |4 1 file changed, 4 insertions(+) diff --git a/src/xcb_io.c b/src/xcb_io.c index f2978d0..acb1e3b 100644 --- a/src/xcb_io.c +++ b/src/xcb_io.c @@ -83,6 +83,10 @@ static void require_socket(Display *dpy) did not own the socket, xcb_xlib_seq_number_wrapped); } + /* The following line will truncate the 64-bit 'sent' +* to 32-bit when assigning it to 'dpy-request'. The +* truncated value will then be assigned to the 64-bit +* 'dpy-xcb-last_flushed' (which is intended). */ dpy-xcb-last_flushed = dpy-request = sent; dpy-bufmax = dpy-xcb-real_bufmax; } -- 1.7.10.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
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 jnsptr...@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'. */
+ unwrapped_request = ((uint64_t)(dpy-request dpy-xcb-last_flushed)
32) + dpy-request;
+
/* 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;
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 2/2] xcb_io: Add comment explaining a mixed type double assignment
The assignment might be confusing at first. So I added a note. Signed-off-by: Jonas Petersen jnsptr...@gmail.com --- src/xcb_io.c |4 1 file changed, 4 insertions(+) diff --git a/src/xcb_io.c b/src/xcb_io.c index f2978d0..acb1e3b 100644 --- a/src/xcb_io.c +++ b/src/xcb_io.c @@ -83,6 +83,10 @@ static void require_socket(Display *dpy) did not own the socket, xcb_xlib_seq_number_wrapped); } + /* The following line will truncate the 64-bit 'sent' +* to 32-bit when assigning it to 'dpy-request'. The +* truncated value will then be assigned to the 64-bit +* 'dpy-xcb-last_flushed' (which is intended). */ dpy-xcb-last_flushed = dpy-request = sent; dpy-bufmax = dpy-xcb-real_bufmax; } -- 1.7.10.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 0/2] Xlib 32-bit sequence number wrap bug(fix)
Hi,
I'm now resubmitting this. I think it's an important issue and it needs to be
reviewed. Please someone take a look at it, there's basically just 3 lines of
code involved.
If anybody wants more information, code to reproduce the failure, or changes
in the patch I submitted, please tell me.
--
Here's two patches. The first one fixes a 32-bit sequence wrap bug. The
second patch only adds a comment to another relevant statement.
The patches contain some details. Here is the whole story for who might be
interested:
Xlib (libx11) will crash an application with a Fatal IO error 11 (Resource
temporarily unavailable) after 4 294 967 296 requests to the server. That is
when the Xlib internal 32-bit sequence wraps.
Most applications probably will hardly reach this number, but if they do,
they have a chance to die a mysterious death. For example the application I'm
working on did always crash after about 20 hours when I started to do some
stress testing. It does some intensive drawing through Xlib using gktmm2,
pixmaps and gc drawing at 40 frames per second in full hd resolution (on
Ubuntu). Some optimizations did extend the grace to about 35 hours but it
would still crash.
What then followed was some frustrating weeks of digging and debugging to
realize that it's not in my application, nor in gtkmm, gtk or glib but that
it's this little bug in Xlib which exists since 2006-10-06 apparently.
It took a while to turn out that the number 0x1 (2^32) has some
relevance. (Much) later it turned out it can be reproduced with Xlib only,
using this code for example:
while(1) {
XDrawPoint(display, drawable, gc, x, y);
XFlush(display);
}
It might take one or two hours, but when it reaches the 4294 million it will
explode into a Fatal IO error 11.
What I then learned is that even though Xlib uses internal 32bit sequence
numbers they get (smartly) widened to 64bit in the process so that the 32bit
sequence may wrap without any disruption in the widened 64bit sequence.
Obviously there must be something wrong with that.
The Fatal IO error is issued in _XReply() when it's not getting a reply where
there should be one, but the cause is earlier in _XSend() in the moment when
the Xlib 32-bit sequence number wraps.
The problem is that when it wraps to 0, the value of 'last_flushed' will
still be at the upper boundary (e.g. 0x). There is two locations in
_XSend() (xcb_io.c) that fail in this state because they rely on those values
being sequential all the time, the first location is:
requests = dpy-request - dpy-xcb-last_flushed;
I case of request = 0x0 and last_flushed = 0x it will assign
0x0001 to 'requests' and then to XCB as a number (amount) of
requests. This is the main killer.
The second location is this:
for(sequence = dpy-xcb-last_flushed + 1; sequence = dpy-request; \
++sequence)
I case of request = 0x0 (less than last_flushed) there is no chance to enter
the loop ever and as a result some requests are ignored.
The solution is to unwrap dpy-request at these two locations and thus
retain the sequence related to last_flushed.
uint64_t unwrapped_request = ((uint64_t)(dpy-request \
dpy-xcb-last_flushed) 32) + dpy-request;
It creates a temporary 64-bit request number which has bit 8 set if 'request'
is less than 'last_flushed'. It is then used in the two locations instead of
dpy-request.
I'm not sure if it might be more efficient to use that statement inplace,
instead of using a variable.
There is another line in require_socket() that worried me at first:
dpy-xcb-last_flushed = dpy-request = sent;
That's a 64-bit, 32-bit, 64-bit assignment. It will truncate 'sent' to 32-bit
when assinging it to 'request' and then also assign the truncated value to
the (64-bit) 'last_flushed'.
But it seems inteded. I have added a note explaining that for the next poor
soul debugging sequence issues... :-)
- Jonas
Jonas Petersen (2):
xcb_io: Fix Xlib 32-bit request number wrapping
xcb_io: Add comment explaining a mixed type double assignment
src/xcb_io.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Is there anything wrong with my patch?
Am 03.11.2013 21:41, schrieb Connor Behan: On 03/11/13 09:33 AM, jnsptr...@gmail.com wrote: Should I be worried that there was not a single response to it so far? Or might it just possibly take a couple of weeks to get noticed? It seems replies usually don't take that long. Do I need to resubmit it? That happens a lot. Feel free to resubmit it or hang out in the #xorg-devel IRC channel. Good to know. I'll resubmit it then in a while if nothing happens. Maybe I'll check out the irc channel as well. Thanks Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Is there anything wrong with my patch?
Hi there, about a week ago I submitted a patch to fix a 32-bit sequence wrapping issue in Xlib: [PATCH libX11 0/2] Xlib 32-bit request sequence wrap bug(fix) [PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping [PATCH libX11 2/2] xcb_io: Add comment explaining a mixed type double assignment Should I be worried that there was not a single response to it so far? Or might it just possibly take a couple of weeks to get noticed? It seems replies usually don't take that long. Do I need to resubmit it? Any advice would be appreciated. Jonas ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 1/2] xcb_io: Fix Xlib 32-bit request number wrapping
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 jnsptr...@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'. */
+ unwrapped_request = ((uint64_t)(dpy-request dpy-xcb-last_flushed)
32) + dpy-request;
+
/* 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;
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 0/2] Xlib 32-bit request sequence wrap bug(fix)
Hi,
Here's two patches. The first one fixes a 32-bit sequence wrap bug. The
second patch only adds a comment to another relevant statement.
The patches contain some details. Here is the whole story for who might be
interested:
Xlib (libx11) will crash an application with a Fatal IO error 11 (Resource
temporarily unavailable) after 4 294 967 296 requests to the server. That is
when the Xlib internal 32-bit sequence wraps.
Most applications probably will hardly reach this number, but if they do,
they have a chance to die a mysterious death. For example the application I'm
working on did always crash after about 20 hours when I started to do some
stress testing. It does some intensive drawing through Xlib using gktmm2,
pixmaps and gc drawing at 40 frames per second in full hd resolution (on
Ubuntu). Some optimizations did extend the grace to about 35 hours but it
would still crash.
What then followed was some frustrating weeks of digging and debugging to
realize that it's not in my application, nor in gtkmm, gtk or glib but that
it's this little bug in Xlib which exists since 2006-10-06 apparently.
It took a while to turn out that the number 0x1 (2^32) has some
relevance. (Much) later it turned out it can be reproduced with Xlib only,
using this code for example:
while(1) {
XDrawPoint(display, drawable, gc, x, y);
XFlush(display);
}
It might take one or two hours, but when it reaches the 4294 million it will
explode into a Fatal IO error 11.
What I then learned is that even though Xlib uses internal 32bit sequence
numbers they get (smartly) widened to 64bit in the process so that the 32bit
sequence may wrap without any disruption in the widened 64bit sequence.
Obviously there must be something wrong with that.
The Fatal IO error is issued in _XReply() when it's not getting a reply where
there should be one, but the cause is earlier in _XSend() in the moment when
the Xlib 32-bit sequence number wraps.
The problem is that when it wraps to 0, the value of 'last_flushed' will
still be at the upper boundary (e.g. 0x). There is two locations in
_XSend() (xcb_io.c) that fail in this state because they rely on those values
being sequential all the time, the first location is:
requests = dpy-request - dpy-xcb-last_flushed;
I case of request = 0x0 and last_flushed = 0x it will assign
0x0001 to 'requests' and then to XCB as a number (amount) of
requests. This is the main killer.
The second location is this:
for(sequence = dpy-xcb-last_flushed + 1; sequence = dpy-request; \
++sequence)
I case of request = 0x0 (less than last_flushed) there is no chance to enter
the loop ever and as a result some requests are ignored.
The solution is to unwrap dpy-request at these two locations and thus
retain the sequence related to last_flushed.
uint64_t unwrapped_request = ((uint64_t)(dpy-request \
dpy-xcb-last_flushed) 32) + dpy-request;
It creates a temporary 64-bit request number which has bit 8 set if 'request'
is less than 'last_flushed'. It is then used in the two locations instead of
dpy-request.
I'm not sure if it might be more efficient to use that statement inplace,
instead of using a variable.
There is another line in require_socket() that worried me at first:
dpy-xcb-last_flushed = dpy-request = sent;
That's a 64-bit, 32-bit, 64-bit assignment. It will truncate 'sent' to 32-bit
when assinging it to 'request' and then also assign the truncated value to
the (64-bit) 'last_flushed'.
But it seems inteded. I have added a note explaining that for the next poor
soul debugging sequence issues... :-)
- Jonas
Jonas Petersen (2):
xcb_io: Fix Xlib 32-bit request number wrapping
xcb_io: Add comment explaining a mixed type double assignment
src/xcb_io.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
--
1.7.10.4
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH libX11 2/2] xcb_io: Add comment explaining a mixed type double assignment
The assignment might be confusing at first. So I added a note. Signed-off-by: Jonas Petersen jnsptr...@gmail.com --- src/xcb_io.c |4 1 file changed, 4 insertions(+) diff --git a/src/xcb_io.c b/src/xcb_io.c index f2978d0..acb1e3b 100644 --- a/src/xcb_io.c +++ b/src/xcb_io.c @@ -83,6 +83,10 @@ static void require_socket(Display *dpy) did not own the socket, xcb_xlib_seq_number_wrapped); } + /* The following line will truncate the 64-bit 'sent' +* to 32-bit when assigning it to 'dpy-request'. The +* truncated value will then be assigned to the 64-bit +* 'dpy-xcb-last_flushed' (which is intended). */ dpy-xcb-last_flushed = dpy-request = sent; dpy-bufmax = dpy-xcb-real_bufmax; } -- 1.7.10.4 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
