[Xcb] xcb recursive write not protected in all cases?

[Uli Schlachter]

Hi,

On 26.12.2013 19:07, Keith Packard wrote:
> Uli Schlachter <psychon at znc.in> writes:
>> TL;DR: I think that having to call get_socket_back() and waiting for
>> c->out.writing to go to zero in a loop is the only issue we have due to the
>> iolock getting dropped. The send_sync()s that were mentioned before
>> seem safe to me.
> 
> Thanks for your careful analysis. I think you've correctly identified
> all of the requirements for making sure we don't have inconsistencies in
> the various paths related to writing requests.
> 
> The one thing that I think is still broken is the sequence number
> requirements around wrapping and taking care of long periods without a
> reply or event. In either case, we must atomically test the sequence
> number condition and generate a request. That means we must not drop
> iolock between the condition test and the request generation, and my
> proposed patch of sticking the check for the socket inside send_request
> would break that.
> 
> I think it's actually pretty easy to fix though. This patch creates a
> new function, prepare_socket_request, which makes sure the current
> thread can write a request, and then uses it in xcb_send_request and
> _xcb_out_send_sync to fix the issues identified above. Then, it sticks
> the sequence number checks in a loop and calls send_sync() *and*
> prepare_socket_request() until both sequence conditions are
> simultaneously true. At that point, it can call send_request knowing
> that the socket is ready to send the request and the sequence number has

Someone just showed up in FreeNode/#xcb and reported this issue again. His
failed assertion is "xcb_conn.c:186: write_vec: Assertion `!c->out.queue_len'
failed", too. He reported this bug at [0] (he originally only showed up in #xcb
to ask how to get debug symbols).

So this issue is still out there and apparently it is biting people.

Keith, could you reword the comment from your patch a bit and send a new version
with my Reviewed-by? IMO the patch still does more than really necessary, but it
definitely is better than the current state of affairs.

I am refering to this (from my other reply to this mail):

> I would prefer calling this "external socket owner" instead of "Xlib owning the
> socket", because the API isn't Xlib-specific (and I once saw xcb_take_socket()
> being (ab)used by something else).
> 
> Also, with Xlib, the issue isn't just about allocating request IDs, but the
> actual write has to be synchronized, too. So something like "If the socket is
> owned externaly, we have to get ownership back before we can generate requests".
> Although I guess this last point goes into bike shedding land...

Everyone else, does anyone mind if this patch gets committed?

Cheers,
Uli

[0] https://bugs.kde.org/show_bug.cgi?id=331977
-- 
Happiness can't be found -- it finds you.
 - Majic