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

[Christian König]

Am 16.05.2013 02:02, schrieb 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?

Not much, changing socket ownership is protected by the iolock. So when 
two threads call into xcb_take_socket the thread who can acquire the 
iolock wins and gets the socket. The other thread gets the iolock after 
the first returns from xcb_take_socket and so calls return_socket to get 
the socket back to xcb before giving it out again.

But that's exactly the argument why the current implementation doesn't 
work correctly, any caller of xcb_take_socket must make sure that the 
socket isn't stolen by another thread. The conventional way to do so is 
to use a locking primitive, which is acquired by the caller of 
xcb_take_socket and inside return_socket.

So calling return_socket with the iolock held (and yes using 
socket_moving has the same effect of holding the iolock) is just a 
classic case of acquiring locking primitives in different order.

Christian.

> 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