From 275eaf7683d56ad2b2953edd2fbd5bca328f3cc1 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Thu, 31 Mar 2022 14:33:06 +0100 Subject: [PATCH 1/3] Rework transport connect logic There are a number of ways the existing transport connect logic in trans_connect could be improved for POSIX compatibility, and also slightly tidied up:- 1) The same socket is re-used for multiple connect attempts following failure which isn't behaviour defined by POSIX.1-2017 (although it works on Linux). 2) An asynchronous connect is started, and then after a short delay connect() is called again on the same socket. POSIX.1-2017 is clear that in this situation EALREADY is returned before the connection is established, but is silent on the behaviour expected when the connection is established. Returning success is an option, but so is returning EISCONN. The current code assumes the connect() call will succeed. 3) The code contains two virtually identical, quite complex loops for TCP and UNIX sockets, differing only in the calls to create a socket and connect it. 4) trans_connect() contains looping and retry logic, but this isn't seen as sufficient by the chansrv connect code in xrdp/xrdp_mm.c and the Xorg connect code in xup/xup.c. Both of these implement their own looping and retry logic on top of the logic in trans_connect(), resulting in slightly unpredictable behaviour with regard to timeouts. 5) A socket number can technically be zero, but in a couple of places this isn't allowed for. This PR attempts to correct the implementation of trans_connect(), and also to simplify the areas it is called from. As part of the PR, the signature of the server_is_term member of the xrdp module interface is changed to match the signature expected by the is_term member of a struct trans. This allows for trans_connect() in xrdp modules to directly access g_is_term() within the main xrdp executable. At the moment this functionality is only used by the xup module. --- common/trans.c | 261 +++++++++++++++++++-------------- common/trans.h | 14 ++ libipm/scp.c | 15 +- mc/mc.h | 2 +- neutrinordp/xrdp-neutrinordp.h | 2 +- vnc/vnc.h | 2 +- xrdp/xrdp.h | 2 - xrdp/xrdp_mm.c | 29 +--- xrdp/xrdp_types.h | 4 +- xup/xup.c | 82 +++-------- xup/xup.h | 2 +- 11 files changed, 201 insertions(+), 214 deletions(-) diff --git a/common/trans.c b/common/trans.c index 7a98986c..a02ec621 100644 --- a/common/trans.c +++ b/common/trans.c @@ -32,6 +32,11 @@ #define MAX_SBYTES 0 +/** Time between polls of is_term when connecting */ +#define CONNECT_TERM_POLL_MS 3000 +/** Time we wait before another connect() attempt if one fails immediately */ +#define CONNECT_DELAY_ON_FAIL_MS 2000 + /*****************************************************************************/ int trans_tls_recv(struct trans *self, char *ptr, int len) @@ -96,6 +101,7 @@ trans_create(int mode, int in_size, int out_size) if (self != NULL) { + self->sck = -1; make_stream(self->in_s); init_stream(self->in_s, in_size); make_stream(self->out_s); @@ -129,12 +135,12 @@ trans_delete(struct trans *self) free_stream(self->in_s); free_stream(self->out_s); - if (self->sck > 0) + if (self->sck >= 0) { g_tcp_close(self->sck); } - self->sck = 0; + self->sck = -1; if (self->listen_filename != 0) { @@ -671,143 +677,182 @@ trans_write_copy(struct trans *self) } /*****************************************************************************/ + +/* Shim to apply the function signature of g_tcp_connect() + * to g_tcp_local_connect() + */ +static int +local_connect_shim(int fd, const char *server, const char *port) +{ + return g_tcp_local_connect(fd, port); +} + +/**************************************************************************//** + * Waits for an asynchronous connect to complete. + * @param self - Transport object + * @param start_time Start time of connect (from g_time3()) + * @param timeout Total wait timeout + * @return 0 - connect succeeded, 1 - Connect failed + * + * If the transport is set up for checking a termination object, this + * on a regular basis. + */ +static int +poll_for_async_connect(struct trans *self, int start_time, int timeout) +{ + int rv = 1; + int ms_remaining = timeout - (g_time3() - start_time); + + while (ms_remaining > 0) + { + int poll_time = ms_remaining; + /* Lower bound for waititng for a result */ + if (poll_time < 100) + { + poll_time = 100; + } + /* Limit the wait time if we need to poll for termination */ + if (self->is_term != NULL && poll_time > CONNECT_TERM_POLL_MS) + { + poll_time = CONNECT_TERM_POLL_MS; + } + + if (g_tcp_can_send(self->sck, poll_time)) + { + /* Connect has finished - return the socket status */ + rv = g_sck_socket_ok(self->sck) ? 0 : 1; + break; + } + + /* Check for program termination */ + if (self->is_term != NULL && self->is_term()) + { + break; + } + + ms_remaining = timeout - (g_time3() - start_time); + } + return rv; +} + +/*****************************************************************************/ + int trans_connect(struct trans *self, const char *server, const char *port, int timeout) { + int start_time = g_time3(); int error; - int now; - int start_time; + int ms_before_next_connect; - start_time = g_time3(); + /* + * Function pointers which we use in the main loop to avoid + * having to switch on the socket mode */ + int (*f_alloc_socket)(void); + int (*f_connect)(int fd, const char *server, const char *port); - if (self->sck != 0) + switch (self->mode) { - g_tcp_close(self->sck); - self->sck = 0; + case TRANS_MODE_TCP: + f_alloc_socket = g_tcp_socket; + f_connect = g_tcp_connect; + break; + + case TRANS_MODE_UNIX: + f_alloc_socket = g_tcp_local_socket; + f_connect = local_connect_shim; + break; + + default: + LOG(LOG_LEVEL_ERROR, "Bad socket mode %d", self->mode); + return 1; } - if (self->mode == TRANS_MODE_TCP) /* tcp */ + while (1) { - self->sck = g_tcp_socket(); + /* Check the program isn't terminating */ + if (self->is_term != NULL && self->is_term()) + { + error = 1; + break; + } + + /* Allocate a new socket */ + if (self->sck >= 0) + { + g_tcp_close(self->sck); + } + self->sck = f_alloc_socket(); + if (self->sck < 0) { - self->status = TRANS_STATUS_DOWN; - return 1; + error = 1; + break; } + + /* Try to connect asynchronously */ g_tcp_set_non_blocking(self->sck); - while (1) + error = f_connect(self->sck, server, port); + if (error == 0) { - error = g_tcp_connect(self->sck, server, port); - if (error == 0) + /* Connect was immediately successful */ + break; + } + + if (g_tcp_last_error_would_block(self->sck)) + { + /* Async connect is in progress */ + if (poll_for_async_connect(self, start_time, timeout) == 0) { + /* Async connect was successful */ + error = 0; break; } - else + /* No need to wait any more before the next connect attempt */ + ms_before_next_connect = 0; + } + else + { + /* Connect failed immediately. Wait a bit before the next + * attempt */ + ms_before_next_connect = CONNECT_DELAY_ON_FAIL_MS; + } + + /* Have we reached the total timeout yet? */ + int ms_left = timeout - (g_time3() - start_time); + if (ms_left <= 0) + { + error = 1; + break; + } + + /* Sleep a bit before trying again */ + if (ms_before_next_connect > 0) + { + if (ms_before_next_connect > ms_left) { - if (timeout < 1) - { - self->status = TRANS_STATUS_DOWN; - return 1; - } - now = g_time3(); - if (now - start_time < timeout) - { - g_sleep(100); - } - else - { - self->status = TRANS_STATUS_DOWN; - return 1; - } - if (self->is_term != NULL) - { - if (self->is_term()) - { - self->status = TRANS_STATUS_DOWN; - return 1; - } - } + ms_before_next_connect = ms_left; } + g_sleep(ms_before_next_connect); } } - else if (self->mode == TRANS_MODE_UNIX) /* unix socket */ + + if (error != 0) { - self->sck = g_tcp_local_socket(); - if (self->sck < 0) + if (self->sck >= 0) { - self->status = TRANS_STATUS_DOWN; - return 1; - } - g_tcp_set_non_blocking(self->sck); - while (1) - { - error = g_tcp_local_connect(self->sck, port); - if (error == 0) - { - break; - } - else - { - if (timeout < 1) - { - self->status = TRANS_STATUS_DOWN; - return 1; - } - now = g_time3(); - if (now - start_time < timeout) - { - g_sleep(100); - } - else - { - self->status = TRANS_STATUS_DOWN; - return 1; - } - if (self->is_term != NULL) - { - if (self->is_term()) - { - self->status = TRANS_STATUS_DOWN; - return 1; - } - } - } + g_tcp_close(self->sck); + self->sck = -1; } + self->status = TRANS_STATUS_DOWN; } else { - self->status = TRANS_STATUS_DOWN; - return 1; + self->status = TRANS_STATUS_UP; /* ok */ + self->type1 = TRANS_TYPE_CLIENT; /* client */ } - if (error == -1) - { - if (g_tcp_last_error_would_block(self->sck)) - { - now = g_time3(); - if (now - start_time < timeout) - { - timeout = timeout - (now - start_time); - } - else - { - timeout = 0; - } - if (g_tcp_can_send(self->sck, timeout)) - { - self->status = TRANS_STATUS_UP; /* ok */ - self->type1 = TRANS_TYPE_CLIENT; /* client */ - return 0; - } - } - - return 1; - } - - self->status = TRANS_STATUS_UP; /* ok */ - self->type1 = TRANS_TYPE_CLIENT; /* client */ - return 0; + return error; } /*****************************************************************************/ diff --git a/common/trans.h b/common/trans.h index 9810cefd..c6b6c4bb 100644 --- a/common/trans.h +++ b/common/trans.h @@ -147,6 +147,20 @@ int trans_write_copy(struct trans *self); int trans_write_copy_s(struct trans *self, struct stream *out_s); +/** + * Connect the transport to the specified destination + * + * @param self Transport + * @param server Destination server (TCP transports only) + * @param port TCP port, or UNIX socket to connect to + * @param timeout in milli-seconds for the operation + * @return 0 for success + * + * Multiple connection attempts may be made within the timeout period. + * + * If the operation is successful, 0 is returned and self->status will + * be TRANS_STATUS_UP + */ int trans_connect(struct trans *self, const char *server, const char *port, int timeout); diff --git a/libipm/scp.c b/libipm/scp.c index 4e1426a3..b3f4fbf6 100644 --- a/libipm/scp.c +++ b/libipm/scp.c @@ -74,8 +74,6 @@ scp_connect(const char *host, const char *port, struct trans *t; if ((t = trans_create(TRANS_MODE_TCP, 128, 128)) != NULL) { - int index; - if (host == NULL) { host = "localhost"; @@ -88,18 +86,7 @@ scp_connect(const char *host, const char *port, t->is_term = term_func; - /* try to connect up to 4 times - * - * term_func can be NULL, so check before calling it */ - index = 4; - while (trans_connect(t, host, port, 3000) != 0 && - !(term_func && term_func()) && - --index > 0) - { - g_sleep(1000); - LOG_DEVEL(LOG_LEVEL_DEBUG, "Connect failed. Trying again..."); - } - + trans_connect(t, host, port, 3000); if (t->status != TRANS_STATUS_UP) { trans_delete(t); diff --git a/mc/mc.h b/mc/mc.h index 990ce7a5..879c298c 100644 --- a/mc/mc.h +++ b/mc/mc.h @@ -57,7 +57,7 @@ struct mod int (*server_set_cursor)(struct mod *v, int x, int y, char *data, char *mask); int (*server_palette)(struct mod *v, int *palette); int (*server_msg)(struct mod *v, const char *msg, int code); - int (*server_is_term)(struct mod *v); + int (*server_is_term)(void); int (*server_set_clip)(struct mod *v, int x, int y, int cx, int cy); int (*server_reset_clip)(struct mod *v); int (*server_set_fgcolor)(struct mod *v, int fgcolor); diff --git a/neutrinordp/xrdp-neutrinordp.h b/neutrinordp/xrdp-neutrinordp.h index cbedb08b..55601c08 100644 --- a/neutrinordp/xrdp-neutrinordp.h +++ b/neutrinordp/xrdp-neutrinordp.h @@ -109,7 +109,7 @@ struct mod int (*server_set_pointer)(struct mod *v, int x, int y, char *data, char *mask); int (*server_palette)(struct mod *v, int *palette); int (*server_msg)(struct mod *v, const char *msg, int code); - int (*server_is_term)(struct mod *v); + int (*server_is_term)(void); int (*server_set_clip)(struct mod *v, int x, int y, int cx, int cy); int (*server_reset_clip)(struct mod *v); int (*server_set_fgcolor)(struct mod *v, int fgcolor); diff --git a/vnc/vnc.h b/vnc/vnc.h index f4ce44b4..5cf65391 100644 --- a/vnc/vnc.h +++ b/vnc/vnc.h @@ -102,7 +102,7 @@ struct vnc int (*server_set_cursor)(struct vnc *v, int x, int y, char *data, char *mask); int (*server_palette)(struct vnc *v, int *palette); int (*server_msg)(struct vnc *v, const char *msg, int code); - int (*server_is_term)(struct vnc *v); + int (*server_is_term)(void); int (*server_set_clip)(struct vnc *v, int x, int y, int cx, int cy); int (*server_reset_clip)(struct vnc *v); int (*server_set_fgcolor)(struct vnc *v, int fgcolor); diff --git a/xrdp/xrdp.h b/xrdp/xrdp.h index 36d8f87a..88ec8814 100644 --- a/xrdp/xrdp.h +++ b/xrdp/xrdp.h @@ -477,8 +477,6 @@ server_palette(struct xrdp_mod *mod, int *palette); int server_msg(struct xrdp_mod *mod, const char *msg, int code); int -server_is_term(struct xrdp_mod *mod); -int server_set_clip(struct xrdp_mod *mod, int x, int y, int cx, int cy); int server_reset_clip(struct xrdp_mod *mod); diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index 796d997f..fa3f9396 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -415,7 +415,7 @@ xrdp_mm_setup_mod1(struct xrdp_mm *self) self->mod->server_set_pointer_ex = server_set_pointer_ex; self->mod->server_palette = server_palette; self->mod->server_msg = server_msg; - self->mod->server_is_term = server_is_term; + self->mod->server_is_term = g_is_term; self->mod->server_set_clip = server_set_clip; self->mod->server_reset_clip = server_reset_clip; self->mod->server_set_fgcolor = server_set_fgcolor; @@ -2356,8 +2356,6 @@ xrdp_mm_sesman_connect(struct xrdp_mm *self, const char *ip) static int xrdp_mm_chansrv_connect(struct xrdp_mm *self, const char *ip, const char *port) { - int index; - if (self->wm->client_info->channels_allowed == 0) { LOG(LOG_LEVEL_DEBUG, "%s: " @@ -2387,22 +2385,8 @@ xrdp_mm_chansrv_connect(struct xrdp_mm *self, const char *ip, const char *port) self->chan_trans->no_stream_init_on_data_in = 1; self->chan_trans->extra_flags = 0; - /* try to connect up to 4 times */ - for (index = 0; index < 4; index++) - { - if (trans_connect(self->chan_trans, ip, port, 3000) == 0) - { - break; - } - if (g_is_term()) - { - break; - } - g_sleep(1000); - LOG(LOG_LEVEL_WARNING, "xrdp_mm_chansrv_connect: connect failed " - "trying again..."); - } - + /* try to connect for up to 10 seconds */ + trans_connect(self->chan_trans, ip, port, 10 * 1000); if (self->chan_trans->status != TRANS_STATUS_UP) { LOG(LOG_LEVEL_ERROR, "xrdp_mm_chansrv_connect: error in " @@ -3368,13 +3352,6 @@ server_msg(struct xrdp_mod *mod, const char *msg, int code) return xrdp_wm_log_msg(wm, LOG_LEVEL_DEBUG, "%s", msg); } -/*****************************************************************************/ -int -server_is_term(struct xrdp_mod *mod) -{ - return g_is_term(); -} - /*****************************************************************************/ int server_set_clip(struct xrdp_mod *mod, int x, int y, int cx, int cy) diff --git a/xrdp/xrdp_types.h b/xrdp/xrdp_types.h index feec7184..8ec731fa 100644 --- a/xrdp/xrdp_types.h +++ b/xrdp/xrdp_types.h @@ -74,7 +74,9 @@ struct xrdp_mod char *data, char *mask); int (*server_palette)(struct xrdp_mod *v, int *palette); int (*server_msg)(struct xrdp_mod *v, const char *msg, int code); - int (*server_is_term)(struct xrdp_mod *v); + /* This one can be assigned directly into the is_term member of + * a struct trans */ + int (*server_is_term)(void); int (*server_set_clip)(struct xrdp_mod *v, int x, int y, int cx, int cy); int (*server_reset_clip)(struct xrdp_mod *v); int (*server_set_fgcolor)(struct xrdp_mod *v, int fgcolor); diff --git a/xup/xup.c b/xup/xup.c index e67d9477..ca36efc7 100644 --- a/xup/xup.c +++ b/xup/xup.c @@ -148,8 +148,7 @@ int lib_mod_connect(struct mod *mod) { int error; - int i; - int use_uds; + int socket_mode; struct stream *s; char con_port[256]; @@ -171,82 +170,47 @@ lib_mod_connect(struct mod *mod) make_stream(s); g_sprintf(con_port, "%s", mod->port); - use_uds = 0; - - if (con_port[0] == '/') - { - use_uds = 1; - } error = 0; mod->sck_closed = 0; - i = 0; - if (use_uds) + if (con_port[0] == '/') { + socket_mode = TRANS_MODE_UNIX; LOG(LOG_LEVEL_INFO, "lib_mod_connect: connecting via UNIX socket"); - mod->trans = trans_create(TRANS_MODE_UNIX, 8 * 8192, 8192); - if (mod->trans == 0) - { - free_stream(s); - return 1; - } } else { + socket_mode = TRANS_MODE_TCP; LOG(LOG_LEVEL_INFO, "lib_mod_connect: connecting via TCP socket"); - mod->trans = trans_create(TRANS_MODE_TCP, 8 * 8192, 8192); - if (mod->trans == 0) - { - free_stream(s); - return 1; - } } + mod->trans = trans_create(socket_mode, 8 * 8192, 8192); + if (mod->trans == 0) + { + free_stream(s); + return 1; + } mod->trans->si = mod->si; mod->trans->my_source = XRDP_SOURCE_MOD; + mod->trans->is_term = mod->server_is_term; - while (1) + /* Give the X server a bit of time to start */ + if (trans_connect(mod->trans, mod->ip, con_port, 30 * 1000) == 0) { - - /* mod->server_msg(mod, "connecting...", 0); */ - - error = -1; - if (trans_connect(mod->trans, mod->ip, con_port, 3000) == 0) - { - LOG_DEVEL(LOG_LEVEL_INFO, "lib_mod_connect: connected to Xserver " - "(Xorg or X11rdp) sck %lld", - (long long) (mod->trans->sck)); - error = 0; - } - - if (error == 0) - { - break; - } - - if (mod->server_is_term(mod)) - { - break; - } - - i++; - - if (i >= 60) - { - mod->server_msg(mod, "connection problem, giving up", 0); - break; - } - - g_sleep(500); + LOG_DEVEL(LOG_LEVEL_INFO, "lib_mod_connect: connected to Xserver " + "(Xorg or X11rdp) sck %lld", + (long long) (mod->trans->sck)); + } + else + { + mod->server_msg(mod, "connection problem, giving up", 0); + error = 1; } - if (error == 0) + if (error == 0 && socket_mode == TRANS_MODE_UNIX) { - if (use_uds) - { - lib_mod_log_peer(mod); - } + lib_mod_log_peer(mod); } if (error == 0) diff --git a/xup/xup.h b/xup/xup.h index 767c459d..4f4b415b 100644 --- a/xup/xup.h +++ b/xup/xup.h @@ -70,7 +70,7 @@ struct mod int (*server_set_cursor)(struct mod *v, int x, int y, char *data, char *mask); int (*server_palette)(struct mod *v, int *palette); int (*server_msg)(struct mod *v, const char *msg, int code); - int (*server_is_term)(struct mod *v); + int (*server_is_term)(void); int (*server_set_clip)(struct mod *v, int x, int y, int cx, int cy); int (*server_reset_clip)(struct mod *v); int (*server_set_fgcolor)(struct mod *v, int fgcolor); From 1d190c6ea821d1b9400d3d85fa5f5d7529d50ca1 Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 1 Apr 2022 11:51:11 +0100 Subject: [PATCH 2/3] Prevent unnecessary close of sck = -1 in trans_listen_address() --- common/trans.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/trans.c b/common/trans.c index a02ec621..eb451073 100644 --- a/common/trans.c +++ b/common/trans.c @@ -863,7 +863,7 @@ trans_connect(struct trans *self, const char *server, const char *port, int trans_listen_address(struct trans *self, const char *port, const char *address) { - if (self->sck != 0) + if (self->sck >= 0) { g_tcp_close(self->sck); } From dc72ca269b519c31d80647c6a33b0cdfd164474e Mon Sep 17 00:00:00 2001 From: matt335672 <30179339+matt335672@users.noreply.github.com> Date: Fri, 1 Apr 2022 13:00:33 +0100 Subject: [PATCH 3/3] Set closed RDP socket to -1 rather than 0 --- libxrdp/xrdp_mcs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libxrdp/xrdp_mcs.c b/libxrdp/xrdp_mcs.c index 83b5e760..4d86aee0 100644 --- a/libxrdp/xrdp_mcs.c +++ b/libxrdp/xrdp_mcs.c @@ -1383,7 +1383,7 @@ close_rdp_socket(struct xrdp_mcs *self) { trans_shutdown_tls_mode(self->iso_layer->trans); g_tcp_close(self->iso_layer->trans->sck); - self->iso_layer->trans->sck = 0 ; + self->iso_layer->trans->sck = -1; LOG_DEVEL(LOG_LEVEL_DEBUG, "xrdp_mcs_disconnect - socket closed"); return; }