[Xcb] [PATCH 3/3] fix pthread_cond_wait and get_socket_back dependency

[Jamey Sharp]

Thanks for splitting the original patch into these independent parts--it
helped a lot for review!

After reviewing the code this patch touches, I think there are two
possibilities.

- Either this patch does not completely address race conditions around
  socket ownership;

- Or there are existing, interlocking, invariants that make this safe,
  and they just need to be better documented.

Consider, for example: What happens if xcb_take_socket is called from
two threads at once?

I'm not excited to merge this patch as-is, because it makes the
invariants more obscure than they already were. I would rather see a
patch that makes the code obviously correct. As it stands, I can't
convince myself that the original code is wrong, and I can't convince
myself that the replacement code is right.

Thanks,
Jamey

On Wed, May 15, 2013 at 11:21:37AM +0200, Christian König wrote:
> From: Christian König <christian.koenig at amd.com>
> 
> Both functions may drop the iolock, so it is possible that somebody else has
> acquired the socket or started writing data.
> 
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
>  src/xcb_out.c |   16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/src/xcb_out.c b/src/xcb_out.c
> index 429fa99..b9901ca 100644
> --- a/src/xcb_out.c
> +++ b/src/xcb_out.c
> @@ -239,10 +239,12 @@ 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);
> +    get_socket_back(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 GetInputFocus (sync_req) when 64k-2 requests have been sent without
>       * a reply. */
> @@ -357,10 +359,12 @@ int _xcb_out_send(xcb_connection_t *c, struct iovec *vector, int count)
>  
>  void _xcb_out_send_sync(xcb_connection_t *c)
>  {
> +    get_socket_back(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);
>  }
>  
> @@ -377,8 +381,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;
>  }
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Xcb mailing list
> Xcb at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xcb
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/xcb/attachments/20130515/a49fc0c2/attachment.pgp>