[Xcb] [PATCH] fix deadlock with xcb_take_socket/return_socket v2

[Uli Schlachter]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 14.05.2013 15:54, Christian König wrote:
> 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>

I am OK with this patch. I thought about it and didn't come up with any
alternative solutions. Also, I don't think that this will cause much breakage.
E.g. debian codesearch[0] says that only libX11 uses xcb_take_socket().

Still some comments below.

[0]: http://codesearch.debian.net/search?q=xcb_take_socket

> --- 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
[...]
> @@ -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); +    }

Urgh. This is new and looks like it fixes a different bug (another thread
calling xcb_take_socket() while the current thread waits for a writer). Did
you notice this while looking at the code or did you actually hit this problem
with a test? Is this different enough to deserve its own commit?

> /* 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);

Pfft, unrelated whitespace changes. :-P

[...]
> 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;

I still don't think it matter much, but I would prefer this sequence number to
be an unsigned int, because signed overflow is not safe in C. Is there any
reason against this?

Cheers,
Uli

P.S.: Opinions on this/these issues and patches from others?
- -- 
A learning experience is one of those things that say,
'You know that thing you just did? Don't do that.'
                     -- Douglas Adams
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQEcBAEBCAAGBQJRknuuAAoJECLkKOvLj8sGKLcIANcBFcdoTFAXM/Dmyn/AfN4s
jIU5UlJs6wG9M45haDVbv+tKssau5lDCkPoSZ09F4AXU8ncU8+h9uAWlaE5qPBJy
mG0AcdIocuPZ2wbRsuduozBQpJf/HfLwsbc1Hh5lH1EgCr6kfnnOL7H3tH43McGr
zSFTUNe3KHwWPtu0gbP2qTK/GGuld3cFD1R0l7AdMdkxeHMKpd7PfP/A2BT9cDjo
+NE/PkazxhdZfbtwroDaqrMrMKNvm83YfGxXmgh/w4Pq5+J4kpYL8u939kk+8KOw
fotKbgOI4Cw/mIbTip/m7szA/+XulEr/ydXpppu4F5el9k7uaxLUR7dO5UPC72o=
=H84w
-----END PGP SIGNATURE-----