[Mesa-dev] [PATCH] fix deadlock with libX11

[Christian König]

From: Christian König <christian.koenig at amd.com>

libX11 acquires the display lock before calling "xcb_take_socket" and inside
the "return_socket" callback, this can lead to a classic deadlock situation
with the XCB iolock. Releasing the lock while calling "return_socket" only
partially helps because we still try to avoid calling return_socket from
multiple threads by using an extra condition.

This patch removes the unnecessary extra signaling and just documents that
"return_socket" might be called by multiple threads at the same time (which
works fine with how libX11 is using the interface).

Signed-off-by: Christian König <christian.koenig at amd.com>
---
 src/xcb_out.c |   36 +++++++++++++++++++-----------------
 src/xcbext.h  |    6 ++++--
 src/xcbint.h  |    7 ++++---
 3 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/src/xcb_out.c b/src/xcb_out.c
index 405f963..5eddab8 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -86,20 +86,23 @@ static void send_sync(xcb_connection_t *c)
 
 static void get_socket_back(xcb_connection_t *c)
 {
-    while(c->out.return_socket && c->out.socket_moving)
-        pthread_cond_wait(&c->out.socket_cond, &c->iolock);
-    if(!c->out.return_socket)
-        return;
-
-    c->out.socket_moving = 1;
-    pthread_mutex_unlock(&c->iolock);
-    c->out.return_socket(c->out.socket_closure);
-    pthread_mutex_lock(&c->iolock);
-    c->out.socket_moving = 0;
-
-    pthread_cond_broadcast(&c->out.socket_cond);
-    c->out.return_socket = 0;
-    c->out.socket_closure = 0;
+    while (c->out.return_socket) {
+        /* we are about to release the lock,
+           so make a copy of the current status */
+        xcb_return_socket_func_t return_socket = c->out.return_socket;
+        void *socket_closure = c->out.socket_closure;
+        int socket_seq = c->out.socket_seq;
+
+        pthread_mutex_unlock(&c->iolock);
+        return_socket(socket_closure);
+        pthread_mutex_lock(&c->iolock);
+
+        /* make sure nobody else has acquired the socket */
+        if (socket_seq == c->out.socket_seq) {
+            c->out.return_socket = 0;
+            c->out.socket_closure = 0;
+        }
+    }
     _xcb_in_replies_done(c);
 }
 
@@ -278,6 +281,7 @@ 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;
+        ++c->out.socket_seq;
         if(flags)
             _xcb_in_expect_reply(c, c->out.request, WORKAROUND_EXTERNAL_SOCKET_OWNER, flags);
         assert(c->out.request == c->out.request_written);
@@ -314,11 +318,9 @@ int xcb_flush(xcb_connection_t *c)
 
 int _xcb_out_init(_xcb_out *out)
 {
-    if(pthread_cond_init(&out->socket_cond, 0))
-        return 0;
     out->return_socket = 0;
     out->socket_closure = 0;
-    out->socket_moving = 0;
+    out->socket_seq = 0;
 
     if(pthread_cond_init(&out->cond, 0))
         return 0;
diff --git a/src/xcbext.h b/src/xcbext.h
index 98b3c93..f8520a0 100644
--- a/src/xcbext.h
+++ b/src/xcbext.h
@@ -66,8 +66,10 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect
  * callback which XCB can call when it wants the write side of the
  * socket back to make a request. This callback synchronizes with the
  * external socket owner and flushes any output queues if appropriate.
- * If you are sending requests which won't cause a reply, please note the
- * comment for xcb_writev which explains some sequence number wrap issues.
+ * It is called without any locks held and so might be called from different
+ * threads at the same time. If you are sending requests which won't
+ * cause a reply, please note the comment for xcb_writev which explains
+ * some sequence number wrap issues.
  * */
 int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), void *closure, int flags, uint64_t *sent);
 
diff --git a/src/xcbint.h b/src/xcbint.h
index f9e5a52..c68b2fa 100644
--- a/src/xcbint.h
+++ b/src/xcbint.h
@@ -79,14 +79,15 @@ void *_xcb_map_remove(_xcb_map *q, unsigned int key);
 
 /* xcb_out.c */
 
+typedef void (*xcb_return_socket_func_t)(void *closure);
+
 typedef struct _xcb_out {
     pthread_cond_t cond;
     int writing;
 
-    pthread_cond_t socket_cond;
-    void (*return_socket)(void *closure);
+    xcb_return_socket_func_t return_socket;
     void *socket_closure;
-    int socket_moving;
+    int socket_seq;
 
     char queue[XCB_QUEUE_BUFFER_SIZE];
     int queue_len;
-- 
1.7.9.5