From 65ac8e758b6bd1458c8228613db4fec99d54d86f Mon Sep 17 00:00:00 2001 From: Pavel Roskin Date: Sun, 16 Oct 2016 23:40:47 -0700 Subject: [PATCH 1/4] Fix memory leak: free session data Call scp_session_destroy() in the functions that call scp_session_create() and nowhere else. As found by Valgrind, the session data is not freed if the session is created successfully. --- sesman/libscp/libscp_v1s.c | 2 -- sesman/scp.c | 9 ++++++++- sesman/scp_v1.c | 4 ---- sesman/scp_v1_mng.c | 3 --- 4 files changed, 8 insertions(+), 10 deletions(-) diff --git a/sesman/libscp/libscp_v1s.c b/sesman/libscp/libscp_v1s.c index 284c9b52..a03fea38 100644 --- a/sesman/libscp/libscp_v1s.c +++ b/sesman/libscp/libscp_v1s.c @@ -330,7 +330,6 @@ scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s, if (0 != scp_session_set_username(s, buf)) { - scp_session_destroy(s); log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__); return SCP_SERVER_STATE_INTERNAL_ERR; } @@ -342,7 +341,6 @@ scp_v1s_request_password(struct SCP_CONNECTION *c, struct SCP_SESSION *s, if (0 != scp_session_set_password(s, buf)) { - scp_session_destroy(s); log_message(LOG_LEVEL_WARNING, "[v1s:%d] connection aborted: internal error", __LINE__); return SCP_SERVER_STATE_INTERNAL_ERR; } diff --git a/sesman/scp.c b/sesman/scp.c index e4abe004..d81de0ab 100644 --- a/sesman/scp.c +++ b/sesman/scp.c @@ -36,7 +36,7 @@ void *DEFAULT_CC scp_process_start(void *sck) { struct SCP_CONNECTION scon; - struct SCP_SESSION *sdata; + struct SCP_SESSION *sdata = NULL; scon.in_sck = (int)(tintptr)sck; LOG_DBG("started scp thread on socket %d", scon.in_sck); @@ -89,9 +89,16 @@ scp_process_start(void *sck) break; default: log_message(LOG_LEVEL_ALWAYS, "unknown return from scp_vXs_accept()"); + break; } free_stream(scon.in_s); free_stream(scon.out_s); + + if (sdata) + { + scp_session_destroy(sdata); + } + return 0; } diff --git a/sesman/scp_v1.c b/sesman/scp_v1.c index 1501606d..df138147 100644 --- a/sesman/scp_v1.c +++ b/sesman/scp_v1.c @@ -77,7 +77,6 @@ scp_v1_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) default: /* we check the other errors */ parseCommonStates(e, "scp_v1s_list_sessions()"); - scp_session_destroy(s); return; //break; } @@ -88,7 +87,6 @@ scp_v1_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) scp_v1s_deny_connection(c, "Login failed"); log_message( LOG_LEVEL_INFO, "Login failed for user %s. Connection terminated", s->username); - scp_session_destroy(s); return; } @@ -98,7 +96,6 @@ scp_v1_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) scp_v1s_deny_connection(c, "Access to Terminal Server not allowed."); log_message(LOG_LEVEL_INFO, "User %s not allowed on TS. Connection terminated", s->username); - scp_session_destroy(s); return; } @@ -204,7 +201,6 @@ scp_v1_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) } /* cleanup */ - scp_session_destroy(s); auth_end(data); g_free(slist); } diff --git a/sesman/scp_v1_mng.c b/sesman/scp_v1_mng.c index 29496466..61789cce 100644 --- a/sesman/scp_v1_mng.c +++ b/sesman/scp_v1_mng.c @@ -50,7 +50,6 @@ scp_v1_mng_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) scp_v1s_mng_deny_connection(c, "Login failed"); log_message(LOG_LEVEL_INFO, "[MNG] Login failed for user %s. Connection terminated", s->username); - scp_session_destroy(s); auth_end(data); return; } @@ -61,7 +60,6 @@ scp_v1_mng_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) scp_v1s_mng_deny_connection(c, "Access to Terminal Server not allowed."); log_message(LOG_LEVEL_INFO, "[MNG] User %s not allowed on TS. Connection terminated", s->username); - scp_session_destroy(s); auth_end(data); return; } @@ -105,7 +103,6 @@ scp_v1_mng_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) } /* cleanup */ - scp_session_destroy(s); auth_end(data); } From e17a56efb6750e7db8d060bd1a648a946395ef80 Mon Sep 17 00:00:00 2001 From: Pavel Roskin Date: Sun, 16 Oct 2016 23:40:54 -0700 Subject: [PATCH 2/4] Call auth_end() exactly once in scp_v0_process() As discovered by Valgrind, it wasn't called at all in case of a successful session creation, which leaked memory. --- sesman/scp_v0.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/sesman/scp_v0.c b/sesman/scp_v0.c index efa9080c..565dacb8 100644 --- a/sesman/scp_v0.c +++ b/sesman/scp_v0.c @@ -68,8 +68,6 @@ scp_v0_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) s->username); scp_v0s_replyauthentication(c, errorcode); } - - auth_end(data); } else if (data) { @@ -94,8 +92,6 @@ scp_v0_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) } session_reconnect(display, s->username); - auth_end(data); - /* don't set data to null here */ } else { @@ -148,7 +144,6 @@ scp_v0_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) if (display == 0) { - auth_end(data); scp_v0s_deny_connection(c); } else @@ -160,4 +155,5 @@ scp_v0_process(struct SCP_CONNECTION *c, struct SCP_SESSION *s) { scp_v0s_deny_connection(c); } + auth_end(data); } From dc60a80b862a2e88b1179e8478b6d1ffb7c8249f Mon Sep 17 00:00:00 2001 From: Pavel Roskin Date: Sun, 16 Oct 2016 23:41:05 -0700 Subject: [PATCH 3/4] Memory leak fix: keep X server path in the parameter list g_cfg->xorg_params, g_cfg->vnc_params and g_cfg->rdp_params don't have auto_free enabled, so removing an item from one of those lists won't free its contents. It's better not to change those lists, as they represent the actual config file and could be reused. Instead, omit the 0th parameter (the executable path) from copying to xserver_params. Found by Valgrind. --- sesman/session.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/sesman/session.c b/sesman/session.c index 031b27f6..581af1ae 100644 --- a/sesman/session.c +++ b/sesman/session.c @@ -672,14 +672,13 @@ session_start_fork(int width, int height, int bpp, char *username, /* get path of Xorg from config */ xserver = g_strdup((const char *)list_get_item(g_cfg->xorg_params, 0)); - list_remove_item(g_cfg->xorg_params, 0); /* these are the must have parameters */ list_add_item(xserver_params, (tintptr) g_strdup(xserver)); list_add_item(xserver_params, (tintptr) g_strdup(screen)); /* additional parameters from sesman.ini file */ - list_append_list_strdup(g_cfg->xorg_params, xserver_params, 0); + list_append_list_strdup(g_cfg->xorg_params, xserver_params, 1); /* make sure it ends with a zero */ list_add_item(xserver_params, 0); @@ -706,7 +705,6 @@ session_start_fork(int width, int height, int bpp, char *username, /* get path of Xvnc from config */ xserver = g_strdup((const char *)list_get_item(g_cfg->vnc_params, 0)); - list_remove_item(g_cfg->vnc_params, 0); /* these are the must have parameters */ list_add_item(xserver_params, (tintptr)g_strdup(xserver)); @@ -723,7 +721,7 @@ session_start_fork(int width, int height, int bpp, char *username, /* additional parameters from sesman.ini file */ //config_read_xserver_params(SESMAN_SESSION_TYPE_XVNC, // xserver_params); - list_append_list_strdup(g_cfg->vnc_params, xserver_params, 0); + list_append_list_strdup(g_cfg->vnc_params, xserver_params, 1); /* make sure it ends with a zero */ list_add_item(xserver_params, 0); @@ -738,7 +736,6 @@ session_start_fork(int width, int height, int bpp, char *username, /* get path of X11rdp from config */ xserver = g_strdup((const char *)list_get_item(g_cfg->rdp_params, 0)); - list_remove_item(g_cfg->rdp_params, 0); /* these are the must have parameters */ list_add_item(xserver_params, (tintptr)g_strdup(xserver)); @@ -751,7 +748,7 @@ session_start_fork(int width, int height, int bpp, char *username, /* additional parameters from sesman.ini file */ //config_read_xserver_params(SESMAN_SESSION_TYPE_XRDP, // xserver_params); - list_append_list_strdup(g_cfg->rdp_params, xserver_params, 0); + list_append_list_strdup(g_cfg->rdp_params, xserver_params, 1); /* make sure it ends with a zero */ list_add_item(xserver_params, 0); From b28a986071ba3ee1bb8aac7dce7b211ad4f07504 Mon Sep 17 00:00:00 2001 From: Pavel Roskin Date: Thu, 20 Oct 2016 22:22:51 -0700 Subject: [PATCH 4/4] Fix memory leak in xrdp-sesman on config reload --- sesman/config.c | 16 ++++++++++++++++ sesman/config.h | 3 +++ sesman/sig.c | 3 +++ 3 files changed, 22 insertions(+) diff --git a/sesman/config.c b/sesman/config.c index 69ba1c18..0c169938 100644 --- a/sesman/config.c +++ b/sesman/config.c @@ -374,6 +374,7 @@ config_read_rdp_params(int file, struct config_sesman *cs, struct list *param_n, list_clear(param_n); cs->rdp_params = list_create(); + cs->rdp_params->auto_free = 1; file_read_section(file, SESMAN_CFG_RDP_PARAMS, param_n, param_v); @@ -404,6 +405,7 @@ config_read_xorg_params(int file, struct config_sesman *cs, list_clear(param_n); cs->xorg_params = list_create(); + cs->xorg_params->auto_free = 1; file_read_section(file, SESMAN_CFG_XORG_PARAMS, param_n, param_v); @@ -436,6 +438,7 @@ config_read_vnc_params(int file, struct config_sesman *cs, struct list *param_n, list_clear(param_n); cs->vnc_params = list_create(); + cs->vnc_params->auto_free = 1; file_read_section(file, SESMAN_CFG_VNC_PARAMS, param_n, param_v); @@ -466,7 +469,9 @@ config_read_session_variables(int file, struct config_sesman *cs, list_clear(param_n); cs->session_variables1 = list_create(); + cs->session_variables1->auto_free = 1; cs->session_variables2 = list_create(); + cs->session_variables2->auto_free = 1; file_read_section(file, SESMAN_CFG_SESSION_VARIABLES, param_n, param_v); @@ -490,3 +495,14 @@ config_read_session_variables(int file, struct config_sesman *cs, return 0; } + +void +config_free(struct config_sesman *cs) +{ + list_delete(cs->rdp_params); + list_delete(cs->vnc_params); + list_delete(cs->xorg_params); + list_delete(cs->session_variables1); + list_delete(cs->session_variables2); + g_free(cs); +} diff --git a/sesman/config.h b/sesman/config.h index 14fc4f98..2baef632 100644 --- a/sesman/config.h +++ b/sesman/config.h @@ -356,4 +356,7 @@ int DEFAULT_CC config_read_session_variables(int file, struct config_sesman *cs, struct list *param_n, struct list *param_v); +void +config_free(struct config_sesman *cs); + #endif diff --git a/sesman/sig.c b/sesman/sig.c index 13f9c46c..579b5876 100644 --- a/sesman/sig.c +++ b/sesman/sig.c @@ -91,6 +91,9 @@ sig_sesman_reload_cfg(int sig) /* stop logging subsystem */ log_end(); + /* free old config data */ + config_free(g_cfg); + /* replace old config with newly read one */ g_cfg = cfg;