[xcb] don't flag extra reply in xcb_take_socket

Submitted by Erik Kurzinger on Aug. 8, 2018, 10:20 p.m.

Details

Message ID 93533206a58a4951a86c61424e7e3e9d@HQMAIL108.nvidia.com
State New
Headers show
Series "don't flag extra reply in xcb_take_socket" ( rev: 1 ) in X.org

Not browsing as part of any series.

Commit Message

Erik Kurzinger Aug. 8, 2018, 10:20 p.m.
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.

Signed-off-by: Erik Kurzinger <ekurzinger@nvidia.com>
---
 src/xcb_in.c  | 21 +++++++++++++++++++--
 src/xcb_out.c | 10 ++++++++--
 2 files changed, 27 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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;
     }

Comments

+xcb@

On 08/09/2018 12:20 AM, Erik Kurzinger wrote:
> 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.
> 
> Signed-off-by: Erik Kurzinger <ekurzinger@nvidia.com>
> ---
>  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;
>      }
>
On čtvrtek 9. srpna 2018 0:20:01 CEST Erik Kurzinger wrote:
> 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.

There is a downstream bug with the same issue:
https://bugzilla.opensuse.org/show_bug.cgi?id=1101560

I have let the reporter try libxcb with this patch and reportedly it fixed the 
issue for him. Not sure if it can be used as Tested-by, but positive feedback 
nevertheless!

Michal
On 14.08.2018 14:46, Julien Cristau wrote:
> +xcb@

Thanks, Julien.

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

> On 08/09/2018 12:20 AM, Erik Kurzinger wrote:
>> 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;
>>      }
>>