Merge pull request #3336 from matt335672/fix_x11_socket_check

[regression] code to open local display in waitforx is broken by recent libxcb

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)
     {