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

[Uli Schlachter]

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.

Thanks. I was mostly trying to convince myself. :-)

> 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.

Not really. We just need to make sure that no other request can be sent while
the iolock is released. And since today I think that the current code already
does this:

All code sending a request has to go through send_request() (This is where the
sequence number is actually allocated via ++c->out.request). Callers of
send_request() are xcb_send_request() and send_sync() -> _xcb_out_send_sync() /
xcb_send_request(). The important part here is that the following code is
executed in all case:

  while (c->out.writing)
    pthread_cond_wait(&c->out.cond, &c->iolock);

Now remember that the iolock is only released in _xcb_conn_wait() and that,
because we are writing, c->out.writing is positive during this time. This means
that while the lock is released, no other thread can actually allocate sequence
numbers and any thread that tries will wait on the condition variable out.cond.
When the write is done, the current thread will get the lock again, decrease
c->out.writing and continue its job.

Thus, the send_sync() requests are already atomic in the current code.


After having figured all of this out today, who wants to write this down? And
where? Some Wiki page called "ThreadSafetyInternals"?

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

For the patch: If my reasoning above is correct, this can be simplified to
adding prepare_socket_request() and calling it in the right places. One comment
for that function:

> +    /* We're about to append data to out.queue, so we need to
> +     * atomically test for Xlib owning the socket *and* some other
> +     * thread currently writing.
> +     *
> +     * If Xlib owns the socket, we aren't allowed to allocate request
> +     * IDs as Xlib is doing that.
> +     *
> +     * If some other thread is writing to the socket, we assume it's
> +     * writing from out.queue, and so we can't stick data there.
> +     *
> +     * We satisfy this condition by first calling get_socket_back
> +     * (which may drop the lock, but will return when XCB owns the
> +     * socket again) and then checking for another writing thread and
> +     * escaping the loop if we're ready to go.
> +     */

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...

Uli
-- 
A learning experience is one of those things that say,
'You know that thing you just did? Don't do that.'
                     -- Douglas Adams