[Xcb] [PATCH] fix deadlock with xcb_take_socket/return_socket v2
Christian König
deathsimple at vodafone.de
Tue May 14 06:54:30 PDT 2013
From: Christian König <christian.koenig at amd.com>
To prevent different threads from stealing the socket from each other the
caller of "xcb_take_socket" must hold a lock that is also acquired in
"return_socket". Unfortunately xcb tries to prevent calling return_socket
from multiple threads and this can lead to a deadlock situation.
A simple example:
- X11 has taken the socket
- Thread A has locked the display.
- Thread B does xcb_no_operation() and thus ends up in libX11's return_socket(),
waiting for the display lock.
- Thread A calls e.g. xcb_no_operation(), too, ends up in return_socket() and
because socket_moving == 1, ends up waiting for thread B
=> Deadlock
This patch allows calling return_socket from different threads at the same time
an so resolves the deadlock situation.
Partially fixes: https://bugs.freedesktop.org/show_bug.cgi?id=20708
v2: fixes additional pthread_cond_wait dependencies,
rework comments and patch description
Signed-off-by: Christian König <christian.koenig at amd.com>
---
src/xcb_out.c | 54 ++++++++++++++++++++++++++++++------------------------
src/xcbext.h | 1 +
src/xcbint.h | 7 ++++---
3 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/src/xcb_out.c b/src/xcb_out.c
index 405f963..4987942 100644
--- a/src/xcb_out.c
+++ b/src/xcb_out.c
@@ -86,21 +86,24 @@ 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;
- _xcb_in_replies_done(c);
+ 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);
+ }
+ }
}
/* Public interface */
@@ -237,9 +240,10 @@ unsigned int xcb_send_request(xcb_connection_t *c, int flags, struct iovec *vect
/* get a sequence number and arrange for delivery. */
pthread_mutex_lock(&c->iolock);
/* wait for other writing threads to get out of my way. */
- while(c->out.writing)
+ while(c->out.writing) {
pthread_cond_wait(&c->out.cond, &c->iolock);
- get_socket_back(c);
+ get_socket_back(c);
+ }
/* send GetInputFocus (sync_req) when 64k-2 requests have been sent without
* a reply. */
@@ -272,12 +276,13 @@ int xcb_take_socket(xcb_connection_t *c, void (*return_socket)(void *closure), v
* write requests, so keep flushing until we're done
*/
do
- ret = _xcb_out_flush_to(c, c->out.request);
+ ret = _xcb_out_flush_to(c, c->out.request);
while (ret && c->out.request != c->out.request_written);
if(ret)
{
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 +319,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;
@@ -356,9 +359,10 @@ int _xcb_out_send(xcb_connection_t *c, struct iovec *vector, int count)
void _xcb_out_send_sync(xcb_connection_t *c)
{
/* wait for other writing threads to get out of my way. */
- while(c->out.writing)
+ while(c->out.writing) {
pthread_cond_wait(&c->out.cond, &c->iolock);
- get_socket_back(c);
+ get_socket_back(c);
+ }
send_sync(c);
}
@@ -375,8 +379,10 @@ int _xcb_out_flush_to(xcb_connection_t *c, uint64_t request)
c->out.queue_len = 0;
return _xcb_out_send(c, &vec, 1);
}
- while(c->out.writing)
+ while(c->out.writing) {
pthread_cond_wait(&c->out.cond, &c->iolock);
+ get_socket_back(c);
+ }
assert(XCB_SEQUENCE_COMPARE(c->out.request_written, >=, request));
return 1;
}
diff --git a/src/xcbext.h b/src/xcbext.h
index 98b3c93..4e1f2f7 100644
--- a/src/xcbext.h
+++ b/src/xcbext.h
@@ -66,6 +66,7 @@ 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.
+ * The callback 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.
* */
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
More information about the Xcb
mailing list