From 2a190a2264b1496f12199d6cd59415a4456eb39e Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Mon, 9 Dec 2024 20:51:03 +0000 Subject: [PATCH] Fix regression in opening local display in waitforx Commit 80fab03198854268fc2258d7bdea2013e5d109f7 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 | 6 +++ sesman/sesman.c | 10 ++-- sesman/session_list.c | 10 ++-- waitforx/Makefile.am | 1 + waitforx/waitforx.c | 109 +++++++++++++++++++++++++++++++----------- 5 files changed, 99 insertions(+), 37 deletions(-) diff --git a/common/xrdp_sockets.h b/common/xrdp_sockets.h index ebf044fff2..e98b7657fd 100644 --- 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) */ +#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 index 6aafbc97d1..18cd765f0c 100644 --- 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 index ae3037ef26..b2c0ea59de 100644 --- 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 index d4ddaa45ab..6b3836cb3f 100644 --- 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 index 993fd38796..c12c2f1430 100644 --- a/waitforx/waitforx.c +++ b/waitforx/waitforx.c @@ -5,10 +5,12 @@ #include #include #include +#include #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,24 +71,77 @@ 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("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("Socket '%s' open succeeded.\n", sock_name); + break; + } + else + { + printf("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("Opening display '%s'. Attempt %u of %u\n", display, n, wait); - dpy = XOpenDisplay(display); - if (dpy != NULL) + if ((dpy = XOpenDisplay(display)) != NULL) { printf("Opened display %s\n", display); break; @@ -84,6 +149,13 @@ open_display(const char *display) 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) {