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

[Christian König]

Am 14.05.2013 20:00, schrieb 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?

I noticed that while looking at the code. Releasing the iolock makes it 
possible for another thread to grab the socket, so I thought it would be 
a good idea to get it back. Going to split it into it's 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

Ups, indeed.

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

Oh sorry, overlooked your initial comment on that. Going to change it as 
well.

Christian.

> 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-----
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb