[Xcb] [PATCH xcb] don't flag extra reply in xcb_take_socket

[Julien Cristau]

If any flags are specified in a call to xcb_take_socket,
they should only be applied to replies for requests sent
after that function returns (and until the socket is
re-acquired by XCB).
Previously, they would also be incorrectly applied to the
reply for the last request sent before the socket was taken.
For instance, in this example program the reply for the
GetInputFocus request gets discarded, even though it was
sent before the socket was taken. This results in the
call to retrieve the reply hanging indefinitely.
static void return_socket(void *closure) {}
int main(void)
{
Display *dpy = XOpenDisplay(NULL);
xcb_connection_t *c = XGetXCBConnection(dpy);
xcb_get_input_focus_cookie_t cookie = xcb_get_input_focus_unchecked(c);
xcb_flush(c);
uint64_t seq;
xcb_take_socket(c, return_socket, dpy, XCB_REQUEST_DISCARD_REPLY, &seq);
xcb_generic_error_t *err;
xcb_get_input_focus_reply(c, cookie, &err);
}
In practice, this has been causing intermittent KWin crashes when
used in combination with the proprietary NVIDIA driver such as
https://bugs.kde.org/show_bug.cgi?id=386370 since when Xlib fails to
retrieve one of these incorrectly discarded replies it triggers
an IO error.
---
src/xcb_in.c | 21 +++++++++++++++++++--
src/xcb_out.c | 10 ++++++++--
2 files changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/xcb_in.c b/src/xcb_in.c
index 73209e0..4333ad3 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -958,8 +958,25 @@ void _xcb_in_replies_done(xcb_connection_t *c)
pend = container_of(c->in.pending_replies_tail, struct pending_reply, next);
if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
{
- pend->last_request = c->out.request;
- pend->workaround = WORKAROUND_NONE;
+ if (XCB_SEQUENCE_COMPARE(pend->first_request, <=, c->out.request)) {
+ pend->last_request = c->out.request;
+ pend->workaround = WORKAROUND_NONE;
+ } else {
+ /* The socket was taken, but no requests were actually sent
+ * so just discard the pending_reply that was created.
+ */
+ struct pending_reply *prev_pend = c->in.pending_replies;
+ if (prev_pend == pend) {
+ c->in.pending_replies = NULL;
+ c->in.pending_replies_tail = &c->in.pending_replies;
+ } else {
+ while (prev_pend->next != pend)
+ prev_pend = prev_pend->next;
+ prev_pend->next = NULL;
+ c->in.pending_replies_tail = &prev_pend->next;
+ }
+ free(pend);
+ }
}
}
}
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..c9593e5 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
{
c->out.return_socket = return_socket;
c->out.socket_closure = closure;
- if(flags)
- _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ if(flags) {
+ /* c->out.request + 1 will be the first request sent by the external
+ * socket owner. If the socket is returned before this request is sent
+ * it will be detected in _xcb_in_replies_done and this pending_reply
+ * will be discarded.
+ */
+ _xcb_in_expect_reply(c, c->out.request + 1, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ }
assert(c->out.request == c->out.request_written);
*sent = c->out.request;
}

[Uli Schlachter]

Thanks, Julien.

And sorry for this mail getting stuck in xorg-devel's moderation queue,
I'll remove that CC if I reply again.

If any flags are specified in a call to xcb_take_socket,
they should only be applied to replies for requests sent
after that function returns (and until the socket is
re-acquired by XCB).
Previously, they would also be incorrectly applied to the
reply for the last request sent before the socket was taken.
For instance, in this example program the reply for the
GetInputFocus request gets discarded, even though it was
sent before the socket was taken. This results in the
call to retrieve the reply hanging indefinitely.

Thanks for figuring this out. Still, I'm slightly confused about this. I
added another GetInputFocus request after the xcb_take_socket(). If I
get the reply for this second request first, everything works fine. If I
get the reply for this second request second, getting the first reply
still hangs. (See attached file)

I fail to understand this behaviour. Since pending replies are applied
when receiving responses from the server, shouldn't the order that I
actually fetch the replies from XCB make no difference?

(Also, I patched my local xcb with just your change to
xcb_take_socket(), expecting this to cause the first request after
taking the socket to be discarded, but this did not happen either)

I feel like I am understanding less than before I started to figure this
out...

Cheers,
Uli

[...]

diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..c9593e5 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
{
c->out.return_socket = return_socket;
c->out.socket_closure = closure;
- if(flags)
- _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ if(flags) {
+ /* c->out.request + 1 will be the first request sent by the external
+ * socket owner. If the socket is returned before this request is sent
+ * it will be detected in _xcb_in_replies_done and this pending_reply
+ * will be discarded.
+ */
+ _xcb_in_expect_reply(c, c->out.request + 1, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ }
assert(c->out.request == c->out.request_written);
*sent = c->out.request;
}

[Uli Schlachter]

On 18.08.2018 09:47, Uli Schlachter wrote:
[...]

Post by Uli Schlachter
Thanks for figuring this out. Still, I'm slightly confused about this. I
added another GetInputFocus request after the xcb_take_socket(). If I
get the reply for this second request first, everything works fine. If I
get the reply for this second request second, getting the first reply
still hangs. (See attached file)

I figured this one out: There is no hang, because when XCB gets a reply
with sequence number 2, it knows for sure there will be no reply with
sequence number 1 and returns NULL. Since the test case did not actually
print anything about the reply, this was hidden.

Post by Uli Schlachter
(Also, I patched my local xcb with just your change to
xcb_take_socket(), expecting this to cause the first request after
taking the socket to be discarded, but this did not happen either)

The above creates a pending reply with first_request = 2 and
last_request = 1. This last_request = 1 causes the pending reply to be
removed in read_packet() because it is considered "too old".

Also, I thought a bit about this and think that your patch is the best

Post by Uli Schlachter
$ LC_ALL=C git am -s /tmp/mail
Patch is empty.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Thus, I feel like I can also propose a tiny improvement to the patch:

Can you remove the code in the new "else"-branch of
_xcb_in_replies_done() with the following (and check if it works)?

Post by Uli Schlachter
/* The socket was taken, but no requests were actually sent,
* so just discard the pending_reply that was created.
*/
struct pending_reply **prev_next = &c->in.pending_replies;
while (*prev_next != pend)
prev_next = &(*prev_next)->next;
*prev_next = NULL;
c->in.pending_replies_tail = prev_next;
free(pend);

Your version of this code keeps a pointer to the previous pending_reply
and has a special case when the to-be-removed pending_reply is the first
entry of the list.

The code above instead only tracks the address of the ->next field of
the previous pending_reply, thus allowing to get rid of the special case
with removing the first entry of the list.

[Feel free to rename prev_next to something good, this is the best that
I came up with]

Cheers,
Uli

[Erik Kurzinger]

Hi Uli,

Thanks for taking a look! I tried modifying the 'else' case in _xcb_in_replies_done
as you suggested, I agree that it's a bit cleaner than what I had.

It still appears to fix the hang in the example program as well as the KWin crash
I had mentioned so everything looks good. I can't think of a better name than
'prev_next' either - it'll have to do :)

Cheers,
Erik

---
src/xcb_in.c | 16 ++++++++++++++--
src/xcb_out.c | 10 ++++++++--
2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/src/xcb_in.c b/src/xcb_in.c
index 73209e0..58fe896 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -958,8 +958,20 @@ void _xcb_in_replies_done(xcb_connection_t *c)
pend = container_of(c->in.pending_replies_tail, struct pending_reply, next);
if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
{
- pend->last_request = c->out.request;
- pend->workaround = WORKAROUND_NONE;
+ if (XCB_SEQUENCE_COMPARE(pend->first_request, <=, c->out.request)) {
+ pend->last_request = c->out.request;
+ pend->workaround = WORKAROUND_NONE;
+ } else {
+ /* The socket was taken, but no requests were actually sent
+ * so just discard the pending_reply that was created.
+ */
+ struct pending_reply **prev_next = &c->in.pending_replies;
+ while (*prev_next != pend)
+ prev_next = &(*prev_next)->next;
+ *prev_next = NULL;
+ c->in.pending_replies_tail = prev_next;
+ free(pend);
+ }
}
}
}
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..c9593e5 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
{
c->out.return_socket = return_socket;
c->out.socket_closure = closure;
- if(flags)
- _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ if(flags) {
+ /* c->out.request + 1 will be the first request sent by the external
+ * socket owner. If the socket is returned before this request is sent
+ * it will be detected in _xcb_in_replies_done and this pending_reply
+ * will be discarded.
+ */
+ _xcb_in_expect_reply(c, c->out.request + 1, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ }
assert(c->out.request == c->out.request_written);
*sent = c->out.request;
}

[Uli Schlachter]

Thanks. I took the commit message from your original mail, added it to
this patch and pushed the result.

Cheers,
Uli

P.S.: And now I'll wait for someone to ask for an 1.13.1 release. Plus
someone who volunteers to do that release.

Post by Erik Kurzinger
Hi Uli,
Thanks for taking a look! I tried modifying the 'else' case in _xcb_in_replies_done
as you suggested, I agree that it's a bit cleaner than what I had.
It still appears to fix the hang in the example program as well as the KWin crash
I had mentioned so everything looks good. I can't think of a better name than
'prev_next' either - it'll have to do :)
Cheers,
Erik
---
src/xcb_in.c | 16 ++++++++++++++--
src/xcb_out.c | 10 ++++++++--
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/xcb_in.c b/src/xcb_in.c
index 73209e0..58fe896 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -958,8 +958,20 @@ void _xcb_in_replies_done(xcb_connection_t *c)
pend = container_of(c->in.pending_replies_tail, struct pending_reply, next);
if(pend->workaround == WORKAROUND_EXTERNAL_SOCKET_OWNER)
{
- pend->last_request = c->out.request;
- pend->workaround = WORKAROUND_NONE;
+ if (XCB_SEQUENCE_COMPARE(pend->first_request, <=, c->out.request)) {
+ pend->last_request = c->out.request;
+ pend->workaround = WORKAROUND_NONE;
+ } else {
+ /* The socket was taken, but no requests were actually sent
+ * so just discard the pending_reply that was created.
+ */
+ struct pending_reply **prev_next = &c->in.pending_replies;
+ while (*prev_next != pend)
+ prev_next = &(*prev_next)->next;
+ *prev_next = NULL;
+ c->in.pending_replies_tail = prev_next;
+ free(pend);
+ }
}
}
}
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 3601a5f..c9593e5 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -387,8 +387,14 @@ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
{
c->out.return_socket = return_socket;
c->out.socket_closure = closure;
- if(flags)
- _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ if(flags) {
+ /* c->out.request + 1 will be the first request sent by the external
+ * socket owner. If the socket is returned before this request is sent
+ * it will be detected in _xcb_in_replies_done and this pending_reply
+ * will be discarded.
+ */
+ _xcb_in_expect_reply(c, c->out.request + 1, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
+ }
assert(c->out.request == c->out.request_written);
*sent = c->out.request;
}

[Uli Schlachter]

Post by Uli Schlachter
P.S.: And now I'll wait for someone to ask for an 1.13.1 release.
Plus someone who volunteers to do that release.

Sigh. No one took my bait.

Okay. I do not know when I will have the time to this, but would
anyone mind if I do an 1.13.1 release based on current master of
libxcb? That would just be 1.13 plus this one commit.

If no one speaks up, I will just do the release when I find the time
for it.

Cheers,
Uli
- --
99 little bugs in the code
99 little bugs in the code
Take one down, patch it around
117 little bugs in the code
-- @irqed