diff --git a/common/file.c b/common/file.c index 3a66bba3..a0ef888a 100644 --- a/common/file.c +++ b/common/file.c @@ -100,7 +100,7 @@ l_file_read_sections(int fd, int max_file_size, struct list *names) { if (line_lookup_for_section_name(text, FILE_MAX_LINE_BYTES) != 0) { - list_add_item(names, (tbus)g_strdup(text)); + list_add_strdup(names, text); } } } @@ -286,7 +286,7 @@ l_file_read_section(int fd, int max_file_size, const char *section, if (g_strlen(text) > 0) { file_split_name_value(text, name, value); - list_add_item(names, (tbus)g_strdup(name)); + list_add_strdup(names, name); if (value[0] == '$') { @@ -294,16 +294,16 @@ l_file_read_section(int fd, int max_file_size, const char *section, if (lvalue != 0) { - list_add_item(values, (tbus)g_strdup(lvalue)); + list_add_strdup(values, lvalue); } else { - list_add_item(values, (tbus)g_strdup("")); + list_add_strdup(values, ""); } } else { - list_add_item(values, (tbus)g_strdup(value)); + list_add_strdup(values, value); } } } diff --git a/common/list.c b/common/list.c index 79375e03..b5039869 100644 --- a/common/list.c +++ b/common/list.c @@ -23,6 +23,7 @@ #endif #include +#include #include "arch.h" #include "os_calls.h" @@ -232,6 +233,65 @@ list_insert_item(struct list *self, int index, tbus item) return 1; } + +/******************************************************************************/ +int +list_add_strdup(struct list *self, const char *str) +{ + int rv; + char *dup; + + if (str == NULL) + { + rv = list_add_item(self, (tintptr)str); + } + else if ((dup = g_strdup(str)) == NULL) + { + rv = 0; + } + else + { + rv = list_add_item(self, (tintptr)dup); + if (!rv) + { + g_free(dup); + } + } + + return rv; +} + +/******************************************************************************/ +int +list_add_strdup_multi(struct list *self, ...) +{ + va_list ap; + int entry_count = self->count; + const char *s; + int rv = 1; + + va_start(ap, self); + while ((s = va_arg(ap, const char *)) != NULL) + { + if (!list_add_strdup(self, s)) + { + rv = 0; + break; + } + } + va_end(ap); + + if (rv == 0) + { + // Remove the additional items we added + while (self->count > entry_count) + { + list_remove_item(self, self->count - 1); + } + } + + return rv; +} /******************************************************************************/ /* append one list to another using strdup for each item in the list */ /* begins copy at start_index, a zero based index on the source list */ @@ -245,23 +305,7 @@ list_append_list_strdup(struct list *self, struct list *dest, int start_index) for (index = start_index; index < self->count; index++) { const char *item = (const char *)list_get_item(self, index); - char *dup; - if (item == NULL) - { - // This shouldn't really happen, but if it does we'll - // copy the item anyway. - dup = NULL; - } - else - { - dup = g_strdup(item); - if (dup == NULL) - { - rv = 0; - break; - } - } - if (!list_add_item(dest, (tbus)dup)) + if (!list_add_strdup(dest, item)) { rv = 0; break; diff --git a/common/list.h b/common/list.h index 41b9fe76..c330fa8a 100644 --- a/common/list.h +++ b/common/list.h @@ -113,4 +113,37 @@ list_dump_items(struct list *self); struct list * split_string_into_list(const char *str, char character); +/** + * As list_add_item() but for a C string + * + * This is a convenience function for a common operation + * @param self List to append to + * @param str String to append + * + * The passed-in string is strdup'd onto the list, so if auto_free + * isn't set, memory leaks will occur. + * + * A NULL pointer will be added as a NULL entry. + * + * @result 0 if any memory allocation failure occurred. In this case + * the list is unchanged. + */ + +int +list_add_strdup(struct list *self, const char *str); + +/** + * Add multiple strings to a list + * + * This is a convenience function for a common operation + * @param self List to append to + * @param ... Strings to append. Terminate the list with a NULL. + * + * @result 0 if any memory allocation failure occurred. In this case + * the list is unchanged. + */ + +int +list_add_strdup_multi(struct list *self, ...); + #endif diff --git a/common/os_calls.c b/common/os_calls.c index f51f47b9..db5d00b4 100644 --- a/common/os_calls.c +++ b/common/os_calls.c @@ -2717,23 +2717,47 @@ g_execvp(const char *p1, char *args[]) g_strnjoin(args_str, ARGS_STR_LEN, " ", (const char **) args, args_len); LOG(LOG_LEVEL_DEBUG, - "Calling exec (executable: %s, arguments: %s)", + "Calling exec (excutable: %s, arguments: %s)", p1, args_str); - g_rm_temp_dir(); rv = execvp(p1, args); /* should not get here */ + int saved_errno = errno; + LOG(LOG_LEVEL_ERROR, - "Error calling exec (executable: %s, arguments: %s) " + "Error calling exec (excutable: %s, arguments: %s) " "returned errno: %d, description: %s", p1, args_str, g_get_errno(), g_get_strerror()); - g_mk_socket_path(0); + errno = saved_errno; return rv; #endif } +/*****************************************************************************/ +int +g_execvp_list(const char *file, struct list *argv) +{ + int rv = -1; + + /* Push a terminating NULL onto the list for the system call */ + if (!list_add_item(argv, (tintptr)NULL)) + { + LOG(LOG_LEVEL_ERROR, "No memory for exec to terminate list"); + errno = ENOMEM; + } + else + { + /* Read the argv argument straight from the list */ + rv = g_execvp(file, (char **)argv->items); + + /* should not get here */ + list_remove_item(argv, argv->count - 1); // Lose terminating NULL + } + return rv; +} + /*****************************************************************************/ /* does not work in win32 */ int diff --git a/common/os_calls.h b/common/os_calls.h index 067da5a6..5304ae66 100644 --- a/common/os_calls.h +++ b/common/os_calls.h @@ -32,6 +32,8 @@ struct exit_status uint8_t signal_no; }; +struct list; + #define g_tcp_can_recv g_sck_can_recv #define g_tcp_can_send g_sck_can_send #define g_tcp_recv g_sck_recv @@ -234,6 +236,18 @@ int g_system(const char *aexec); char *g_get_strerror(void); int g_get_errno(void); int g_execvp(const char *p1, char *args[]); +/** + * Issues an execvp() call + * + * @param file Executable + * @param argv Argument list for executable. + * + * argv does not need to be NULL terminated - the call takes care + * of this. + * + * @return Only if an error has occurred - use g_get_errno() or equivalent + */ +int g_execvp_list(const char *file, struct list *argv); int g_execlp3(const char *a1, const char *a2, const char *a3); unsigned int g_set_alarm(void (*func)(int), unsigned int secs); void g_signal_child_stop(void (*func)(int)); diff --git a/sesman/config.c b/sesman/config.c index fc477ca0..34ef69ae 100644 --- a/sesman/config.c +++ b/sesman/config.c @@ -452,8 +452,8 @@ config_read_xorg_params(int file, struct config_sesman *cs, for (i = 0; i < param_n->count; i++) { - list_add_item(cs->xorg_params, - (long) g_strdup((char *) list_get_item(param_v, i))); + list_add_strdup(cs->xorg_params, + (const char *) list_get_item(param_v, i)); } return 0; @@ -485,7 +485,8 @@ config_read_vnc_params(int file, struct config_sesman *cs, struct list *param_n, for (i = 0; i < param_n->count; i++) { - list_add_item(cs->vnc_params, (long)g_strdup((char *)list_get_item(param_v, i))); + list_add_strdup(cs->vnc_params, + (const char *)list_get_item(param_v, i)); } return 0; @@ -510,10 +511,10 @@ config_read_session_variables(int file, struct config_sesman *cs, for (i = 0; i < param_n->count; i++) { - list_add_item(cs->env_names, - (tintptr) g_strdup((char *) list_get_item(param_n, i))); - list_add_item(cs->env_values, - (tintptr) g_strdup((char *) list_get_item(param_v, i))); + list_add_strdup(cs->env_names, + (const char *) list_get_item(param_n, i)); + list_add_strdup(cs->env_values, + (const char *) list_get_item(param_v, i)); } return 0; diff --git a/sesman/session.c b/sesman/session.c index 77af95cb..3bfe5860 100644 --- a/sesman/session.c +++ b/sesman/session.c @@ -344,7 +344,7 @@ static int session_start_chansrv(int uid, int display) { struct list *chansrv_params; - char exe_path[262]; + const char *exe_path = XRDP_SBIN_PATH "/xrdp-chansrv"; int chansrv_pid; chansrv_pid = g_fork(); @@ -357,18 +357,15 @@ session_start_chansrv(int uid, int display) chansrv_params->auto_free = 1; /* building parameters */ - g_snprintf(exe_path, sizeof(exe_path), "%s/xrdp-chansrv", - XRDP_SBIN_PATH); - list_add_item(chansrv_params, (intptr_t) g_strdup(exe_path)); - list_add_item(chansrv_params, 0); /* mandatory */ + list_add_strdup(chansrv_params, exe_path); env_set_user(uid, 0, display, g_cfg->env_names, g_cfg->env_values); /* executing chansrv */ - g_execvp(exe_path, (char **) (chansrv_params->items)); + g_execvp_list(exe_path, chansrv_params); /* should not get here */ list_delete(chansrv_params); @@ -422,7 +419,6 @@ session_start(struct auth_info *auth_info, char execvpparams[2048]; char *xserver = NULL; /* absolute/relative path to Xorg/Xvnc */ char *passwd_file; - char **pp1 = (char **)NULL; struct session_chain *temp = (struct session_chain *)NULL; struct list *xserver_params = (struct list *)NULL; char authfile[256]; /* The filename for storing xauth information */ @@ -759,19 +755,14 @@ session_start(struct auth_info *auth_info, xserver = g_strdup((const char *)list_get_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)); - list_add_item(xserver_params, (tintptr) g_strdup("-auth")); - list_add_item(xserver_params, (tintptr) g_strdup(authfile)); + list_add_strdup_multi(xserver_params, + xserver, screen, + "-auth", authfile, + NULL); /* additional parameters from sesman.ini file */ list_append_list_strdup(g_cfg->xorg_params, xserver_params, 1); - /* make sure it ends with a zero */ - list_add_item(xserver_params, 0); - - pp1 = (char **) xserver_params->items; - /* some args are passed via env vars */ g_sprintf(geometry, "%d", s->width); g_setenv("XRDP_START_WIDTH", geometry, 1); @@ -791,27 +782,19 @@ session_start(struct auth_info *auth_info, xserver = g_strdup((const char *)list_get_item(g_cfg->vnc_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)); - list_add_item(xserver_params, (tintptr)g_strdup("-auth")); - list_add_item(xserver_params, (tintptr)g_strdup(authfile)); - list_add_item(xserver_params, (tintptr)g_strdup("-geometry")); - list_add_item(xserver_params, (tintptr)g_strdup(geometry)); - list_add_item(xserver_params, (tintptr)g_strdup("-depth")); - list_add_item(xserver_params, (tintptr)g_strdup(depth)); - list_add_item(xserver_params, (tintptr)g_strdup("-rfbauth")); - list_add_item(xserver_params, (tintptr)g_strdup(passwd_file)); - + list_add_strdup_multi(xserver_params, + xserver, screen, + "-auth", authfile, + "-geometry", geometry, + "-depth", depth, + "-rfbauth", passwd_file, + NULL); g_free(passwd_file); /* additional parameters from sesman.ini file */ //config_read_xserver_params(SCP_SESSION_TYPE_XVNC, // xserver_params); list_append_list_strdup(g_cfg->vnc_params, xserver_params, 1); - - /* make sure it ends with a zero */ - list_add_item(xserver_params, 0); - pp1 = (char **)xserver_params->items; } else { @@ -826,7 +809,7 @@ session_start(struct auth_info *auth_info, /* fire up X server */ LOG(LOG_LEVEL_INFO, "Starting X server on display %d: %s", display, dumpItemsToString(xserver_params, execvpparams, 2048)); - g_execvp(xserver, pp1); + g_execvp_list(xserver, xserver_params); /* should not get here */ LOG(LOG_LEVEL_ERROR, diff --git a/tests/common/test_list_calls.c b/tests/common/test_list_calls.c index a4a48fd7..75705e08 100644 --- a/tests/common/test_list_calls.c +++ b/tests/common/test_list_calls.c @@ -65,7 +65,15 @@ START_TEST(test_list__simple_auto_free) { char strval[64]; g_snprintf(strval, sizeof(strval), "%d", i); - list_add_item(lst, (tintptr)g_strdup(strval)); + // Odds, use list_add_item/strdup, evens use list_add_strdup + if ((i % 2) != 0) + { + list_add_item(lst, (tintptr)g_strdup(strval)); + } + else + { + list_add_strdup(lst, strval); + } } list_remove_item(lst, 0); @@ -116,6 +124,29 @@ START_TEST(test_list__simple_append_list) } END_TEST +START_TEST(test_list__simple_strdup_multi) +{ + int i; + struct list *lst = list_create(); + lst->auto_free = 1; + + list_add_strdup_multi(lst, + "0", "1", "2", "3", "4", "5", + "6", "7", "8", "9", "10", "11", + NULL); + + ck_assert_int_eq(lst->count, 12); + + for (i = 0 ; i < lst->count; ++i) + { + int val = g_atoi((const char *)list_get_item(lst, i)); + ck_assert_int_eq(val, i); + } + + list_delete(lst); +} +END_TEST + int split_string_append_fragment(const char **start, const char *end, struct list *list); @@ -172,6 +203,7 @@ make_suite_test_list(void) tcase_add_test(tc_simple, test_list__simple); tcase_add_test(tc_simple, test_list__simple_auto_free); tcase_add_test(tc_simple, test_list__simple_append_list); + tcase_add_test(tc_simple, test_list__simple_strdup_multi); tcase_add_test(tc_simple, test_list__append_fragment); tcase_add_test(tc_simple, test_list__split_string_into_list); diff --git a/xrdp/xrdp_login_wnd.c b/xrdp/xrdp_login_wnd.c index a97467ab..8b6062f4 100644 --- a/xrdp/xrdp_login_wnd.c +++ b/xrdp/xrdp_login_wnd.c @@ -652,11 +652,11 @@ xrdp_wm_login_fill_in_combo(struct xrdp_wm *self, struct xrdp_bitmap *b) g_strncpy(name, r, 255); } - list_add_item(mod_data->names, (long)g_strdup(q)); - list_add_item(mod_data->values, (long)g_strdup(r)); + list_add_strdup(mod_data->names, q); + list_add_strdup(mod_data->values, r); } - list_add_item(b->string_list, (long)g_strdup(name)); + list_add_strdup(b->string_list, name); list_add_item(b->data_list, (long)mod_data); } } diff --git a/xrdp/xrdp_mm.c b/xrdp/xrdp_mm.c index c6dee2a8..9644c8ec 100644 --- a/xrdp/xrdp_mm.c +++ b/xrdp/xrdp_mm.c @@ -486,8 +486,8 @@ xrdp_mm_setup_mod2(struct xrdp_mm *self) the module should use the last one */ if (g_strlen(text) > 0) { - list_add_item(self->login_names, (long)g_strdup("port")); - list_add_item(self->login_values, (long)g_strdup(text)); + list_add_strdup(self->login_names, "port"); + list_add_strdup(self->login_values, text); } /* always set these */ diff --git a/xrdp/xrdp_wm.c b/xrdp/xrdp_wm.c index f4915247..dc546220 100644 --- a/xrdp/xrdp_wm.c +++ b/xrdp/xrdp_wm.c @@ -758,8 +758,8 @@ xrdp_wm_init(struct xrdp_wm *self) } } - list_add_item(self->mm->login_names, (long)g_strdup(q)); - list_add_item(self->mm->login_values, (long)g_strdup(r)); + list_add_strdup(self->mm->login_names, q); + list_add_strdup(self->mm->login_values, r); } /*