Move XID and sync handling from SyncHandle to LockDisplay to fix races.

XID and sync handling happened via _XPrivSyncHandler, assigned to
dpy->synchandler and called from SyncHandle.  _XPrivSyncHandler thus ran
without the Display lock, so manipulating the Display caused races, and
these races led to assertions in multithreaded code (demonstrated via
ico).

In the XTHREADS case, after you've called XInitThreads, we can hook
LockDisplay and UnlockDisplay.  Use that to run _XIDHandler and
_XSeqSyncHandler from LockDisplay rather than SyncHandle; we then know
that we hold the lock, and thus we can avoid races.  We think it makes
sense to do these both from LockDisplay rather than UnlockDisplay, so
that you know you have valid sync and a valid XID before you start
setting up the request you locked to prepare.

In the !XTHREADS case, or if you haven't called XInitThreads, you don't
get to use Xlib from multiple threads, so we can use the logic we have
now (with synchandler and savedsynchandler) without any concern about
races.

This approach gets a bit exciting when the XID and sequence sync
handlers drop and re-acquire the Display lock. Reacquisition will re-run
the handlers, but they return immediately unless they have work to do,
so they can't recurse more than once.  In the worst case, if both of
them have work to do, we can nest the Display lock three deep.  In the
case of the _XIDHandler, we drop the lock to call xcb_generate_id, which
takes the socket back if it needs to request more XIDs, and taking the
socket back will reacquire the lock; we take care to avoid letting
_XIDHandler run again and re-enter XCB from the return_socket callback
(which causes Very Bad Things, and is Not Allowed).

Tested with ico (with 1 and 20 threads), and with several test programs
for XID and sequence sync.  Tested with and without XInitThreads(), and
with and without XCB.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=23192

Signed-off-by: Jamey Sharp <jamey@minilop.net>
Signed-off-by: Josh Triplett <josh@freedesktop.org>

1 files changed, 8 insertions, 17 deletions

diff --git a/src/xcb_io.c b/src/xcb_io.c
index 2f305da7..1459ef7f 100644
--- a/src/xcb_io.c
+++ b/src/xcb_io.c
@@ -382,19 +382,14 @@ static const XID inval_id = ~0UL;
 
 int _XIDHandler(Display *dpy)
 {
-	XID next;
-
-	if (dpy->xcb->next_xid != inval_id)
-	    return 0;
-
-	next = xcb_generate_id(dpy->xcb->connection);
-	LockDisplay(dpy);
-	dpy->xcb->next_xid = next;
-#ifdef XTHREADS
-	if (dpy->lock)
-		(*dpy->lock->user_unlock_display)(dpy);
-#endif
-	UnlockDisplay(dpy);
+	if (dpy->xcb->next_xid == inval_id)
+	{
+		/* We drop the Display lock to call xcb_generate_id, and
+		 * xcb_generate_id might take the socket back, which will call
+		 * LockDisplay. Avoid recursing. */
+		dpy->xcb->next_xid = inval_id - 1;
+		_XAllocIDs(dpy, &dpy->xcb->next_xid, 1);
+	}
 	return 0;
 }
 
@@ -403,10 +398,6 @@ XID _XAllocID(Display *dpy)
 {
 	XID ret = dpy->xcb->next_xid;
 	assert (ret != inval_id);
-#ifdef XTHREADS
-	if (dpy->lock)
-		(*dpy->lock->user_lock_display)(dpy);
-#endif
 	dpy->xcb->next_xid = inval_id;
 	_XSetPrivSyncFunction(dpy);
 	return ret;