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.

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) */
+#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

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

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);
         }

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
 

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
@@ -29,22 +31,32 @@ alarm_handler(int signal_num)
 
 /*****************************************************************************/
 /***
- * Checks whether display is local.
+ * Checks whether display can be reached via a Unix Domain Socket socket.
  *
- * Local displays are of the form ':n' or ':n.m' where 'n' and 'm'
+ * Local displays can be reached by a Unix Domain socket. The display
+ * string will be of the form ':n' or ':n.m' where 'n' and 'm'
  * are unsigned numbers
  *
  * @param display Display string
- * @return boolean
+ * @param[out] sock_name, or ""
+ * @param sock_name_len Length of sock_name
+ * @return !=0 if sock_name is not NULL
  */
 static int
-is_local_display(const char *display)
+get_display_sock_name(const char *display, char *sock_name,
+                      size_t sock_name_len)
 {
-    int result = 0;
+    int local = 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));
@@ -59,31 +71,91 @@ is_local_display(const char *display)
             while (isdigit(*display));
         }
 
-        result = (*display == '\0');
+        local = (*display == '\0');
     }
-    return result;
+
+    if (local)
+    {
+        snprintf(sock_name, sock_name_len, X11_UNIX_SOCKET_STR, dnum);
+    }
+    else
+    {
+        sock_name[0] = '\0';
+    }
+
+
+    return (sock_name[0] != '\0');
 }
 
 /*****************************************************************************/
 static Display *
 open_display(const char *display)
 {
+    char sock_name[XRDP_SOCKETS_MAXPATH];
+    int local_fd = -1;
     Display *dpy = NULL;
     unsigned int wait = ATTEMPTS;
     unsigned int n;
 
-    for (n = 1; n <= ATTEMPTS; ++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. We can't use it here.
+    if (get_display_sock_name(display, sock_name, sizeof(sock_name)) != 0)
+    {
+        for (n = 1; n <= wait ; ++n)
+        {
+            printf("<D>Opening socket %s. Attempt %u of %u\n",
+                   sock_name, n, wait);
+            if ((local_fd = g_sck_local_socket()) >= 0)
+            {
+                if  (g_sck_local_connect(local_fd, sock_name) == 0)
+                {
+                    printf("<D>Socket '%s' open succeeded.\n", sock_name);
+                    break;
+                }
+                else
+                {
+                    printf("<D>Socket '%s' open failed [%s].\n",
+                           sock_name, g_get_strerror());
+                    g_file_close(local_fd);
+                    local_fd = -1;
+                }
+            }
+            g_sleep(1000);
+        }
+
+        // Subtract the wait time for this stage from the overall wait time
+        wait -= (n - 1);
+    }
+
+    for (n = 1; n <= wait; ++n)
     {
         printf("<D>Opening display '%s'. Attempt %u of %u\n", display, n, wait);
-        dpy = XOpenDisplay(display);
-        if (dpy != NULL)
+        if ((dpy = XOpenDisplay(display)) != NULL)
         {
             printf("<D>Opened display %s\n", display);
             break;
         }
         g_sleep(1000);
     }
 
+    // Close the file after we try the display open, to prevent
+    // a display reset if our connect was the last client.
+    if (local_fd >= 0)
+    {
+        g_file_close(local_fd);
+    }
+
     return dpy;
 }
 
@@ -150,7 +222,6 @@ 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 opt;
     int status = XW_STATUS_MISC_ERROR;
@@ -179,22 +250,6 @@ main(int argc, char **argv)
 
     g_set_alarm(alarm_handler, ALARM_WAIT);
 
-    if (is_local_display(display_name))
-    {
-        // 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;
-    }
-
     dpy = open_display(display_name);
     if (!dpy)
     {