Fix regression in opening local display in waitforx

Commit 80fab03 introduced a way to prevent waitforx going to the network when trying to open a display, and hence potentially blocking. This method turned out to be invalidated by libxcb version 1.16 and 1.17 This change adds an explicit check that the Unix socket for the display in /tmp/.X11-unix/Xn is open before trying to connect to display ':n'. This has the same effect.
matt335672 · Dec 9, 2024 · 44100c5 · 44100c5
1 parent e404509
commit 44100c5
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 28 deletions.
diff --git a/common/xrdp_sockets.h b/common/xrdp_sockets.h
@@ -61,4 +61,10 @@
 #define XRDP_X11RDP_STR       XRDP_SOCKET_PATH "/" XRDP_X11RDP_BASE_STR
 #define XRDP_DISCONNECT_STR   XRDP_SOCKET_PATH "/" XRDP_DISCONNECT_BASE_STR
 
+/* Where X11 stores its Unix Domain Sockets (unlikely to change now) */
+#define X11_UNIX_SOCKET_DIRECTORY "/tmp/.X11-unix"
+
+/*  fullpath to an X11 display socket */
+#define X11_UNIX_SOCKET_STR X11_UNIX_SOCKET_DIRECTORY "/X%d"
+
 #endif
diff --git a/sesman/sesman.c b/sesman/sesman.c
@@ -925,15 +925,15 @@ main(int argc, char **argv)
     LOG(LOG_LEVEL_INFO,
         "starting xrdp-sesman with pid %d", g_pid);
 
-    /* make sure the /tmp/.X11-unix directory exists */
-    if (!g_directory_exist("/tmp/.X11-unix"))
+    /* make sure the X11_UNIX_SOCKET_DIRECTORY exists */
+    if (!g_directory_exist(X11_UNIX_SOCKET_DIRECTORY))
     {
-        if (!g_create_dir("/tmp/.X11-unix"))
+        if (!g_create_dir(X11_UNIX_SOCKET_DIRECTORY))
         {
             LOG(LOG_LEVEL_ERROR,
-                "sesman.c: error creating dir /tmp/.X11-unix");
+                "sesman.c: error creating dir " X11_UNIX_SOCKET_DIRECTORY);
         }
-        g_chmod_hex("/tmp/.X11-unix", 0x1777);
+        g_chmod_hex(X11_UNIX_SOCKET_DIRECTORY, 0x1777);
     }
 
     if ((error = pre_session_list_init(MAX_PRE_SESSION_ITEMS)) == 0 &&

diff --git a/sesman/session_list.c b/sesman/session_list.c
@@ -155,13 +155,13 @@ x_server_running_check_ports(int display)
     int x_running;
     int sck;
 
-    g_sprintf(text, "/tmp/.X11-unix/X%d", display);
+    g_snprintf(text, sizeof(text), X11_UNIX_SOCKET_STR, display);
     x_running = g_file_exist(text);
 
     if (!x_running)
     {
         LOG(LOG_LEVEL_DEBUG, "Did not find a running X server at %s", text);
-        g_sprintf(text, "/tmp/.X%d-lock", display);
+        g_snprintf(text, sizeof(text), "/tmp/.X%d-lock", display);
         x_running = g_file_exist(text);
     }
 
@@ -170,7 +170,7 @@ x_server_running_check_ports(int display)
         LOG(LOG_LEVEL_DEBUG, "Did not find a running X server at %s", text);
         if ((sck = g_tcp_socket()) != -1)
         {
-            g_sprintf(text, "59%2.2d", display);
+            g_snprintf(text, sizeof(text), "59%2.2d", display);
             x_running = g_tcp_bind(sck, text);
             g_tcp_close(sck);
         }
@@ -181,7 +181,7 @@ x_server_running_check_ports(int display)
         LOG(LOG_LEVEL_DEBUG, "Did not find a running X server at %s", text);
         if ((sck = g_tcp_socket()) != -1)
         {
-            g_sprintf(text, "60%2.2d", display);
+            g_snprintf(text, sizeof(text), "60%2.2d", display);
             x_running = g_tcp_bind(sck, text);
             g_tcp_close(sck);
         }
@@ -192,7 +192,7 @@ x_server_running_check_ports(int display)
         LOG(LOG_LEVEL_DEBUG, "Did not find a running X server at %s", text);
         if ((sck = g_tcp_socket()) != -1)
         {
-            g_sprintf(text, "62%2.2d", display);
+            g_snprintf(text, sizeof(text), "62%2.2d", display);
             x_running = g_tcp_bind(sck, text);
             g_tcp_close(sck);
         }

diff --git a/waitforx/Makefile.am b/waitforx/Makefile.am
@@ -4,6 +4,7 @@ pkglibexec_PROGRAMS = \
 AM_LDFLAGS = $(X_LIBS) -lX11 -lXrandr
 
 AM_CPPFLAGS = \
+  -DXRDP_SOCKET_ROOT_PATH=\"${socketdir}\" \
   -I$(top_srcdir)/sesman/sesexec \
   -I$(top_srcdir)/common
 

diff --git a/waitforx/waitforx.c b/waitforx/waitforx.c
@@ -5,10 +5,12 @@
 #include <ctype.h>
 #include <sys/signal.h>
 #include <unistd.h>
+#include <limits.h>
 
 #include "config_ac.h"
 #include "os_calls.h"
 #include "string_calls.h"
+#include "xrdp_sockets.h"
 #include "xwait.h" // For return status codes
 
 #define ATTEMPTS 10
@@ -35,16 +37,23 @@ alarm_handler(int signal_num)
  * are unsigned numbers
  *
  * @param display Display string
+ * @param[out] dnum Display number (i.e. 'n' in ':n[.m]')
  * @return boolean
  */
 static int
-is_local_display(const char *display)
+is_local_display(const char *display, int *dnum_out)
 {
     int result = 0;
+    int dnum = 0;
     if (display != NULL && *display++ == ':' && isdigit(*display))
     {
         do
         {
+            if (dnum > (INT_MAX / 10 - 1))
+            {
+                break; // Avoid signed integer overflow
+            }
+            dnum = (dnum * 10) + (*display - '0');
             ++display;
         }
         while (isdigit(*display));
@@ -61,21 +70,80 @@ is_local_display(const char *display)
 
         result = (*display == '\0');
     }
+
+    if (result && (dnum_out != NULL))
+    {
+        *dnum_out = dnum;
+    }
     return result;
 }
 
 /*****************************************************************************/
 static Display *
-open_display(const char *display)
+open_display(const char *display, int dnum)
 {
+    char sock_name[XRDP_SOCKETS_MAXPATH];
     Display *dpy = NULL;
     unsigned int wait = ATTEMPTS;
     unsigned int n;
 
+    // If the display is local, we try to connect to the X11
+    // socket for the display first. If we can't do this, we
+    // don't attempt to open the display.
+    //
+    // This is to ensure the display open code in libxcb doesn't
+    // attempt to connect to the X server over TCP. This can
+    // block if the network is configured in an unexpected way,
+    // which leads to use failing to detect the X server starting
+    // up shortly after.
+    //
+    // Some versions of libxcb support a 'unix:' prefix to the display
+    // string to allow a connection to be restricted to a local socket.
+    // This is not documented, and varies significantly between versions
+    // of libxcb.
+    if (dnum < 0)
+    {
+        // Not a local display
+        sock_name[0] = '\0';
+    }
+    else
+    {
+        snprintf(sock_name, sizeof(sock_name), X11_UNIX_SOCKET_STR, dnum);
+    }
+
     for (n = 1; n <= ATTEMPTS; ++n)
     {
+        int s = -1;
         printf("<D>Opening display '%s'. Attempt %u of %u\n", display, n, wait);
-        dpy = XOpenDisplay(display);
+        if (sock_name[0] != '\0')
+        {
+            if ((s = g_sck_local_socket()) >= 0)
+            {
+                if  (g_sck_local_connect(s, sock_name) == 0)
+                {
+                    printf("<D>Socket check of '%s' succeeded.\n", sock_name);
+                    dpy = XOpenDisplay(display);
+                }
+                else
+                {
+                    printf("<D>Socket check of '%s' failed [%s].\n",
+                           sock_name, g_get_strerror());
+                }
+
+                // Close the file after we try the display open, to prevent
+                // a display reset if this is the last client.
+                g_file_close(s);
+            }
+            else
+            {
+                printf("<D>Can't create local socket.\n");
+            }
+        }
+        else
+        {
+            dpy = XOpenDisplay(display);
+        }
+
         if (dpy != NULL)
         {
             printf("<D>Opened display %s\n", display);
@@ -150,8 +218,8 @@ usage(const char *argv0, int status)
 int
 main(int argc, char **argv)
 {
-    char unix_display[64]; // Used for local (unix) displays only
     const char *display_name = NULL;
+    int dnum;
     int opt;
     int status = XW_STATUS_MISC_ERROR;
     Display *dpy = NULL;
@@ -179,23 +247,12 @@ main(int argc, char **argv)
 
     g_set_alarm(alarm_handler, ALARM_WAIT);
 
-    if (is_local_display(display_name))
+    if (!is_local_display(display_name, &dnum))
     {
-        // Don't use the raw display value, as this goes to the
-        // network if the X server port is not yet open. This can
-        // block if the network is configured in an unexpected way,
-        // which leads to use failing to detect the X server starting
-        // up shortly after.
-        //
-        // This code attempts to use a string such as "unix:10" to open
-        // the display. This is undocumented in the X11 man pages but
-        // is implemented in _xcb_open() from libxcb
-        // (which libX11 is now layered on).
-        snprintf(unix_display, sizeof(unix_display), "unix%s", display_name);
-        display_name = unix_display;
+        dnum = -1;
     }
 
-    dpy = open_display(display_name);
+    dpy = open_display(display_name, dnum);
     if (!dpy)
     {
         printf("<E>Unable to open display %s\n", display_name);