From 8c92ef59890c6d6e2be456658d3b9c145eda8629 Mon Sep 17 00:00:00 2001

From: Keith Packard <keithp@keithp.com>

Date: Sat, 7 Nov 2020 22:22:47 -0800

Subject: [PATCH libX11] Avoid recursing through _XError due to sequence

 adjustment

This patch is based on research done by Dmitry Osipenko to uncover the

cause of a large class of Xlib lockups.

_XError must unlock and re-lock the display around the call to the

user error handler function. When re-locking the display, two

functions are called to ensure that the display is ready to generate a request:

    _XIDHandler(dpy);

    _XSeqSyncFunction(dpy);

The first ensures that there is at least one XID available to use

(possibly calling _xcb_generate_id to do so). The second makes sure a

reply is received at least every 65535 requests to keep sequence

numbers in sync (possibly generating a GetInputFocus request and

synchronously awaiting the reply).

If the second of these does generate a GetInputFocus request and wait

for the reply, then a pending error will cause recursion into _XError,

which deadlocks the display.

One seemingly easy fix is to have _XError avoid those calls by

invoking InternalLockDisplay instead of LockDisplay. That function

does everything that LockDisplay does *except* call those final two

functions which may end up receiving an error.

However, that doesn't protect the system from applications which call

some legal Xlib function from within their error handler. Any Xlib

function which cannot generate protocol or wait for events is valid,

including many which invoke LockDisplay.

What we need to do is make LockDisplay skip these two function calls

precisely when it is called from within the _XError context for the

same display.

This patch accomplishes this by creating a list of threads in the

display which are in _XError, and then having LockDisplay check the

current thread against those list elements.

Inspired-by: Dmitry Osipenko <digetx@gmail.com>

Signed-off-by: Keith Packard <keithp@keithp.com>

Tested-by: Dmitry Osipenko <digetx@gmail.com>

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

(cherry picked from commit 30ccef3a48029bf4fc31d4abda2d2778d0ad6277)

---

 include/X11/Xlibint.h |  2 ++

 src/OpenDis.c         |  1 +

 src/XlibInt.c         | 10 ++++++++++

 src/locking.c         | 12 ++++++++++++

 src/locking.h         | 12 ++++++++++++

 5 files changed, 37 insertions(+)

diff --git a/include/X11/Xlibint.h b/include/X11/Xlibint.h

index 6b95bcf7..09078e3f 100644

--- a/include/X11/Xlibint.h

+++ b/include/X11/Xlibint.h

@@ -202,6 +202,8 @@ struct _XDisplay

 	unsigned long last_request_read_upper32bit;

 	unsigned long request_upper32bit;

 #endif

+

+	struct _XErrorThreadInfo *error_threads;

 };

 #define XAllocIDs(dpy,ids,n) (*(dpy)->idlist_alloc)(dpy,ids,n)

diff --git a/src/OpenDis.c b/src/OpenDis.c

index 82723578..85901168 100644

--- a/src/OpenDis.c

+++ b/src/OpenDis.c

@@ -201,6 +201,7 @@ XOpenDisplay (

 	X_DPY_SET_LAST_REQUEST_READ(dpy, 0);

 	dpy->default_screen = iscreen;  /* Value returned by ConnectDisplay */

 	dpy->last_req = (char *)&_dummy_request;

+	dpy->error_threads = NULL;

 	/* Initialize the display lock */

 	if (InitDisplayLock(dpy) != 0) {

diff --git a/src/XlibInt.c b/src/XlibInt.c

index 4e45e62b..8771b791 100644

--- a/src/XlibInt.c

+++ b/src/XlibInt.c

@@ -1482,6 +1482,11 @@ int _XError (

     if (_XErrorFunction != NULL) {

 	int rtn_val;

 #ifdef XTHREADS

+	struct _XErrorThreadInfo thread_info = {

+		.error_thread = xthread_self(),

+		.next = dpy->error_threads

+	}, **prev;

+	dpy->error_threads = &thread_info;

 	if (dpy->lock)

 	    (*dpy->lock->user_lock_display)(dpy);

 	UnlockDisplay(dpy);

@@ -1491,6 +1496,11 @@ int _XError (

 	LockDisplay(dpy);

 	if (dpy->lock)

 	    (*dpy->lock->user_unlock_display)(dpy);

+

+	/* unlink thread_info from the list */

+	for (prev = &dpy->error_threads; *prev != &thread_info; prev = &(*prev)->next)

+		;

+	*prev = thread_info.next;

 #endif

 	return rtn_val;

     } else {

diff --git a/src/locking.c b/src/locking.c

index 9f4fe067..bcadc857 100644

--- a/src/locking.c

+++ b/src/locking.c

@@ -453,6 +453,9 @@ static void _XLockDisplay(

     XTHREADS_FILE_LINE_ARGS

     )

 {

+#ifdef XTHREADS

+    struct _XErrorThreadInfo *ti;

+#endif

 #ifdef XTHREADS_WARN

     _XLockDisplayWarn(dpy, file, line);

 #else

@@ -460,6 +463,15 @@ static void _XLockDisplay(

 #endif

     if (dpy->lock->locking_level > 0)

 	_XDisplayLockWait(dpy);

+#ifdef XTHREADS

+    /*

+     * Skip the two function calls below which may generate requests

+     * when LockDisplay is called from within _XError.

+     */

+    for (ti = dpy->error_threads; ti; ti = ti->next)

+	    if (ti->error_thread == xthread_self())

+		    return;

+#endif

     _XIDHandler(dpy);

     _XSeqSyncFunction(dpy);

 }

diff --git a/src/locking.h b/src/locking.h

index 5251a60c..59fc866e 100644

--- a/src/locking.h

+++ b/src/locking.h

@@ -149,6 +149,18 @@ typedef struct _LockInfoRec {

 	xmutex_t	lock;

 } LockInfoRec;

+/* A list of threads currently invoking error handlers on this display

+ * LockDisplay operates differently for these threads, avoiding

+ * generating any requests or reading any events as that can cause

+ * recursion into the error handling code, which will deadlock the

+ * thread.

+ */

+struct _XErrorThreadInfo

+{

+    struct _XErrorThreadInfo *next;

+    xthread_t error_thread;

+};

+

 /* XOpenDis.c */

 extern int (*_XInitDisplayLock_fn)(Display *dpy);

 extern void (*_XFreeDisplayLock_fn)(Display *dpy);

-- 

2.43.0