Merge pull request #2576 from matt335672/list_string_support

List string support

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);
                             }
                         }
                     }

common/list.c

@@ -23,6 +23,7 @@
 #endif
 
 #include <stdlib.h>
+#include <stdarg.h>
 
 #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;

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

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

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));

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;

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,

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);
 

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);
         }
     }

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 */

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);
                 }
 
                 /*