Accept full path for DefaultWindowManager #1147
Conversation
sesman/config.c
Outdated
| @@ -530,6 +570,9 @@ config_dump(struct config_sesman *config) | |||
| void | |||
| config_free(struct config_sesman *cs) | |||
| { | |||
| g_free(cs->default_wm); | |||
| g_free(cs->reconnect_sh); | |||
| g_free(cs->auth_file_path); | |||
There was a problem hiding this comment.
This is not supposed to be a part of this patch
sesman/config.c
Outdated
| { | ||
| g_strncpy(cf->default_wm, "startwm.sh", 11); | ||
| buf = (char *)g_malloc(1024, 0); | ||
| g_sprintf(buf, "%s/%s", XRDP_CFG_PATH, g_cfg->default_wm); |
There was a problem hiding this comment.
This is prone to buffer overflow
There was a problem hiding this comment.
There was a problem hiding this comment.
Will alloc g_strlen(XRDP_CFG_PATH) + g_strlen(g_cfg->default_wm) + 2.
sesman/config.c
Outdated
| if (cf->reconnect_sh[0] != '/') | ||
| { | ||
| buf = (char *)g_malloc(1024, 0); | ||
| g_sprintf(buf, "%s/%s", XRDP_CFG_PATH, g_cfg->reconnect_sh); |
| @@ -575,8 +575,7 @@ session_start_fork(tbus data, tui8 type, struct SCP_CONNECTION *c, | |||
| } | |||
| /* if we're here something happened to g_execlp3 | |||
| so we try running the default window manager */ | |||
| g_sprintf(text, "%s/%s", XRDP_CFG_PATH, g_cfg->default_wm); | |||
| g_execlp3(text, g_cfg->default_wm, 0); | |||
| g_execlp3(g_cfg->default_wm, g_cfg->default_wm, 0); | |||
There was a problem hiding this comment.
should change the log message properly
log_message(LOG_LEVEL_DEBUG, " argv[0] = %s",
text);
metalefty
force-pushed
the
defaultwm-fullpath
branch
from
40b1e25
to
4489899
Compare
sesman/config.c
Outdated
| /* if default_wm doesn't begin with '/', it's a relative path to XRDP_CFG_PATH */ | ||
| if (cf->default_wm[0] != '/') | ||
| { | ||
| length = g_strlen(XRDP_CFG_PATH) + g_strlen(g_cfg->default_wm) + 2; /* '/' + '\0' */ |
There was a problem hiding this comment.
using strlen on XRDP_CFG_PATH is redundant, you better use sizeof(XRDP_CFG_PATH)-1 since its already known at compile time.
docs/man/sesman.ini.5.in
Outdated
| \fBReconnectScript\fR=\fIfilename\fR | ||
| Full path or relative path if the script which executed when users reconnects | ||
| to the existing session. If the path is not a full path, it will be resolved as | ||
| relative path to \fI@xrdpconfdir@\fR. |
There was a problem hiding this comment.
it worth to note the default setting if empty / not specified
Solves: neutrinolabs#1143 Also, this idea is inspired by Fedora's patch [1]. Some distro wants to put all scripts in libexec directory due to SELinux. This enables distros to put such scripts anywhere. [1] https://src.fedoraproject.org/cgit/rpms/xrdp.git/tree/xrdp-0.9.6-scripts-libexec.patch?id=02f845c1b8cea781313cf3e9efcd6d7d50341824
metalefty
force-pushed
the
defaultwm-fullpath
branch
from
4489899
to
6e16b38
Compare
sesman/config.c
Outdated
| { | ||
| g_strncpy(cf->default_wm, "startwm.sh", 11); | ||
| length = sizeof(XRDP_CFG_PATH) + g_strlen(g_cfg->reconnect_sh) + 1; /* '/' */ |
There was a problem hiding this comment.
i think its better to mention the addition of the null terminator explicitly.
BTW, if we were using strlendirectly gcc would optimize it since it smart enough to detect its applied on a constant value. (but we are using our g_strlen wrapper :)
Changes in neutrinolabs#1147 had a bug. Fixes neutrinolabs#1315. Reported by: Daniel Hoffend
Also,
Solves #1143. Inspired by Fedora's patch[1].
[1] https://src.fedoraproject.org/cgit/rpms/xrdp.git/commit/xrdp-0.9.6-scripts-libexec.patch?id=02f845c1b8cea781313cf3e9efcd6d7d50341824