diff --git a/common/xrdp_sockets.h b/common/xrdp_sockets.h index ebf044ff..e98b7657 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 6aafbc97..18cd765f 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 ae3037ef..b2c0ea59 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 d4ddaa45..6b3836cb 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 993fd387..c12c2f14 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) {