Merge branch 'bug5105-v2-squashed'

Conflicts:
	src/or/transports.c

changes/bug5105

@@ -0,0 +1,11 @@
+  o Minor bugfixes:
+
+    - Ensure that variables set in Tor's environment cannot override
+      environment variables which Tor tries to pass to a managed
+      pluggable-transport proxy.  Previously, Tor would pass every
+      variable in its environment to managed proxies along with the
+      new ones, in such a way that on many operating systems, the
+      inherited environment variables would override those which Tor
+      tried to explicitly set.  Bugfix on 0.2.3.12-alpha for most
+      Unixoid systems; bugfix on 0.2.3.9-alpha for Windows.
+

src/common/util.c

@@ -172,6 +172,35 @@ _tor_malloc_zero(size_t size DMALLOC_PARAMS)
   return result;
 }
 
+/** Allocate a chunk of <b>nmemb</b>*<b>size</b> bytes of memory, fill
+ * the memory with zero bytes, and return a pointer to the result.
+ * Log and terminate the process on error.  (Same as
+ * calloc(<b>nmemb</b>,<b>size</b>), but never returns NULL.)
+ *
+ * XXXX This implementation probably asserts in cases where it could
+ * work, because it only tries dividing SIZE_MAX by size (according to
+ * the calloc(3) man page, the size of an element of the nmemb-element
+ * array to be allocated), not by nmemb (which could in theory be
+ * smaller than size).  Don't do that then.
+ */
+void *
+_tor_calloc(size_t nmemb, size_t size DMALLOC_PARAMS)
+{
+  /* You may ask yourself, "wouldn't it be smart to use calloc instead of
+   * malloc+memset?  Perhaps libc's calloc knows some nifty optimization trick
+   * we don't!"  Indeed it does, but its optimizations are only a big win when
+   * we're allocating something very big (it knows if it just got the memory
+   * from the OS in a pre-zeroed state).  We don't want to use tor_malloc_zero
+   * for big stuff, so we don't bother with calloc. */
+  void *result;
+  size_t max_nmemb = (size == 0) ? SIZE_MAX : SIZE_MAX/size;
+
+  tor_assert(nmemb < max_nmemb);
+
+  result = _tor_malloc_zero((nmemb * size) DMALLOC_FN_ARGS);
+  return result;
+}
+
 /** Change the size of the memory block pointed to by <b>ptr</b> to <b>size</b>
  * bytes long; return the new memory block.  On error, log and
  * terminate. (Like realloc(ptr,size), but never returns NULL.)
@@ -3284,11 +3313,7 @@ process_handle_new(void)
  */
 int
 tor_spawn_background(const char *const filename, const char **argv,
-#ifdef _WIN32
-                     LPVOID envp,
-#else
-                     const char **envp,
-#endif
+                     process_environment_t *env,
                      process_handle_t **process_handle_out)
 {
 #ifdef _WIN32
@@ -3305,8 +3330,6 @@ tor_spawn_background(const char *const filename, const char **argv,
   SECURITY_ATTRIBUTES saAttr;
   char *joined_argv;
 
-  (void)envp; // Unused on Windows
-
   saAttr.nLength = sizeof(SECURITY_ATTRIBUTES);
   saAttr.bInheritHandle = TRUE;
   /* TODO: should we set explicit security attributes? (#2046, comment 5) */
@@ -3371,7 +3394,7 @@ tor_spawn_background(const char *const filename, const char **argv,
   /*(TODO: set CREATE_NEW CONSOLE/PROCESS_GROUP to make GetExitCodeProcess()
    * work?) */
                  0,             // creation flags
-                 envp,          // use parent's environment
+                 (env==NULL) ? NULL : env->windows_environment_block,
                  NULL,          // use parent's current directory
                  &siStartInfo,  // STARTUPINFO pointer
                  &(process_handle->pid));  // receives PROCESS_INFORMATION
@@ -3501,8 +3524,8 @@ tor_spawn_background(const char *const filename, const char **argv,
     /* Call the requested program. We need the cast because
        execvp doesn't define argv as const, even though it
        does not modify the arguments */
-    if (envp)
-      execve(filename, (char *const *) argv, (char*const*)envp);
+    if (env)
+      execve(filename, (char *const *) argv, env->unixoid_environment_block);
     else
       execvp(filename, (char *const *) argv);
 
@@ -3692,6 +3715,162 @@ tor_get_exit_code(const process_handle_t *process_handle,
   return PROCESS_EXIT_EXITED;
 }
 
+/** Return non-zero iff getenv would consider <b>s1</b> and <b>s2</b>
+ * to have the same name as strings in a process's environment. */
+int
+environment_variable_names_equal(const char *s1, const char *s2)
+{
+  size_t s1_name_len = strcspn(s1, "=");
+  size_t s2_name_len = strcspn(s2, "=");
+
+  return (s1_name_len == s2_name_len &&
+          tor_memeq(s1, s2, s1_name_len));
+}
+
+/** Free <b>env</b> (assuming it was produced by
+ * process_environment_make). */
+void
+process_environment_free(process_environment_t *env)
+{
+  if (env == NULL) return;
+
+  /* As both an optimization hack to reduce consing on Unixoid systems
+   * and a nice way to ensure that some otherwise-Windows-specific
+   * code will always get tested before changes to it get merged, the
+   * strings which env->unixoid_environment_block points to are packed
+   * into env->windows_environment_block. */
+  tor_free(env->unixoid_environment_block);
+  tor_free(env->windows_environment_block);
+
+  tor_free(env);
+}
+
+/** Make a process_environment_t containing the environment variables
+ * specified in <b>env_vars</b> (as C strings of the form
+ * "NAME=VALUE"). */
+process_environment_t *
+process_environment_make(struct smartlist_t *env_vars)
+{
+  process_environment_t *env = tor_malloc_zero(sizeof(process_environment_t));
+  size_t n_env_vars = smartlist_len(env_vars);
+  size_t i;
+  size_t total_env_length;
+  smartlist_t *env_vars_sorted;
+
+  tor_assert(n_env_vars + 1 != 0);
+  env->unixoid_environment_block = tor_calloc(n_env_vars + 1, sizeof(char *));
+  /* env->unixoid_environment_block is already NULL-terminated,
+   * because we assume that NULL == 0 (and check that during compilation). */
+
+  total_env_length = 1; /* terminating NUL of terminating empty string */
+  for (i = 0; i < n_env_vars; ++i) {
+    const char *s = smartlist_get(env_vars, i);
+    size_t slen = strlen(s);
+
+    tor_assert(slen + 1 != 0);
+    tor_assert(slen + 1 < SIZE_MAX - total_env_length);
+    total_env_length += slen + 1;
+  }
+
+  env->windows_environment_block = tor_malloc_zero(total_env_length);
+  /* env->windows_environment_block is already
+   * (NUL-terminated-empty-string)-terminated. */
+
+  /* Some versions of Windows supposedly require that environment
+   * blocks be sorted.  Or maybe some Windows programs (or their
+   * runtime libraries) fail to look up strings in non-sorted
+   * environment blocks.
+   *
+   * Also, sorting strings makes it easy to find duplicate environment
+   * variables and environment-variable strings without an '=' on all
+   * OSes, and they can cause badness.  Let's complain about those. */
+  env_vars_sorted = smartlist_new();
+  smartlist_add_all(env_vars_sorted, env_vars);
+  smartlist_sort_strings(env_vars_sorted);
+
+  /* Now copy the strings into the environment blocks. */
+  {
+    char *cp = env->windows_environment_block;
+    const char *prev_env_var = NULL;
+
+    for (i = 0; i < n_env_vars; ++i) {
+      const char *s = smartlist_get(env_vars_sorted, i);
+      size_t slen = strlen(s);
+      size_t s_name_len = strcspn(s, "=");
+
+      if (s_name_len == slen) {
+        log_warn(LD_GENERAL,
+                 "Preparing an environment containing a variable "
+                 "without a value: %s",
+                 s);
+      }
+      if (prev_env_var != NULL &&
+          environment_variable_names_equal(s, prev_env_var)) {
+        log_warn(LD_GENERAL,
+                 "Preparing an environment containing two variables "
+                 "with the same name: %s and %s",
+                 prev_env_var, s);
+      }
+
+      prev_env_var = s;
+
+      /* Actually copy the string into the environment. */
+      memcpy(cp, s, slen+1);
+      env->unixoid_environment_block[i] = cp;
+      cp += slen+1;
+    }
+
+    tor_assert(cp == env->windows_environment_block + total_env_length - 1);
+  }
+
+  return env;
+}
+
+/** Return a newly allocated smartlist containing every variable in
+ * this process's environment, as a NUL-terminated string of the form
+ * "NAME=VALUE".  Note that on some/many/most/all OSes, the parent
+ * process can put strings not of that form in our environment;
+ * callers should try to not get crashed by that.
+ *
+ * The returned strings are heap-allocated, and must be freed by the
+ * caller. */
+struct smartlist_t *
+get_current_process_environment_variables(void)
+{
+  smartlist_t *sl = smartlist_new();
+
+  char **environ_tmp; /* Not const char ** ? Really? */
+  for (environ_tmp = environ; *environ_tmp; ++environ_tmp) {
+    smartlist_add(sl, tor_strdup(*environ_tmp));
+  }
+
+  return sl;
+}
+
+/** For each string s in <b>env_vars</b> such that
+ * environment_variable_names_equal(s, <b>new_var</b>), remove it; if
+ * <b>free_p</b> is non-zero, call <b>free_old</b>(s).  If
+ * <b>new_var</b> contains '=', insert it into <b>env_vars</b>. */
+void
+set_environment_variable_in_smartlist(struct smartlist_t *env_vars,
+                                      const char *new_var,
+                                      void (*free_old)(void*),
+                                      int free_p)
+{
+  SMARTLIST_FOREACH_BEGIN(env_vars, const char *, s) {
+    if (environment_variable_names_equal(s, new_var)) {
+      SMARTLIST_DEL_CURRENT(env_vars, s);
+      if (free_p) {
+        free_old((void *)s);
+      }
+    }
+  } SMARTLIST_FOREACH_END(s);
+
+  if (strchr(new_var, '=') != NULL) {
+    smartlist_add(env_vars, (void *)new_var);
+  }
+}
+
 #ifdef _WIN32
 /** Read from a handle <b>h</b> into <b>buf</b>, up to <b>count</b> bytes.  If
  * <b>hProcess</b> is NULL, the function will return immediately if there is

src/common/util.h

@@ -73,6 +73,7 @@
 void *_tor_malloc(size_t size DMALLOC_PARAMS) ATTR_MALLOC;
 void *_tor_malloc_zero(size_t size DMALLOC_PARAMS) ATTR_MALLOC;
 void *_tor_malloc_roundup(size_t *size DMALLOC_PARAMS) ATTR_MALLOC;
+void *_tor_calloc(size_t nmemb, size_t size DMALLOC_PARAMS) ATTR_MALLOC;
 void *_tor_realloc(void *ptr, size_t size DMALLOC_PARAMS);
 char *_tor_strdup(const char *s DMALLOC_PARAMS) ATTR_MALLOC ATTR_NONNULL((1));
 char *_tor_strndup(const char *s, size_t n DMALLOC_PARAMS)
@@ -107,6 +108,7 @@ extern int dmalloc_free(const char *file, const int line, void *pnt,
 
 #define tor_malloc(size)       _tor_malloc(size DMALLOC_ARGS)
 #define tor_malloc_zero(size)  _tor_malloc_zero(size DMALLOC_ARGS)
+#define tor_calloc(nmemb,size) _tor_calloc(nmemb, size DMALLOC_ARGS)
 #define tor_malloc_roundup(szp) _tor_malloc_roundup(szp DMALLOC_ARGS)
 #define tor_realloc(ptr, size) _tor_realloc(ptr, size DMALLOC_ARGS)
 #define tor_strdup(s)          _tor_strdup(s DMALLOC_ARGS)
@@ -363,12 +365,9 @@ void tor_check_port_forwarding(const char *filename,
                                int dir_port, int or_port, time_t now);
 
 typedef struct process_handle_t process_handle_t;
+typedef struct process_environment_t process_environment_t;
 int tor_spawn_background(const char *const filename, const char **argv,
-#ifdef _WIN32
-                         LPVOID envp,
-#else
-                         const char **envp,
-#endif
+                         process_environment_t *env,
                          process_handle_t **process_handle_out);
 
 #define SPAWN_ERROR_MESSAGE "ERR: Failed to spawn background process - code "
@@ -377,6 +376,27 @@ int tor_spawn_background(const char *const filename, const char **argv,
 HANDLE load_windows_system_library(const TCHAR *library_name);
 #endif
 
+int environment_variable_names_equal(const char *s1, const char *s2);
+
+struct process_environment_t {
+  /** A pointer to a sorted empty-string-terminated sequence of
+   * NUL-terminated strings of the form "NAME=VALUE". */
+  char *windows_environment_block;
+  /** A pointer to a NULL-terminated array of pointers to
+   * NUL-terminated strings of the form "NAME=VALUE". */
+  char **unixoid_environment_block;
+};
+
+process_environment_t *process_environment_make(struct smartlist_t *env_vars);
+void process_environment_free(process_environment_t *env);
+
+struct smartlist_t *get_current_process_environment_variables(void);
+
+void set_environment_variable_in_smartlist(struct smartlist_t *env_vars,
+                                           const char *new_var,
+                                           void (*free_old)(void*),
+                                           int free_p);
+
 /* Values of process_handle_t.status. PROCESS_STATUS_NOTRUNNING must be
  * 0 because tor_check_port_forwarding depends on this being the initial
  * statue of the static instance of process_handle_t */

src/or/transports.c

@@ -91,13 +91,8 @@
 #include "util.h"
 #include "router.h"
 
-#ifdef _WIN32
-static void set_managed_proxy_environment(LPVOID *envp,
-                                          const managed_proxy_t *mp);
-#else
-static void set_managed_proxy_environment(char ***envp,
-                                         const managed_proxy_t *mp);
-#endif
+static process_environment_t *
+create_managed_proxy_environment(const managed_proxy_t *mp);
 
 static INLINE int proxy_configuration_finished(const managed_proxy_t *mp);
 
@@ -287,35 +282,23 @@ launch_managed_proxy(managed_proxy_t *mp)
 {
   int retval;
 
-#ifdef _WIN32
-
-  LPVOID envp=NULL;
-
-  set_managed_proxy_environment(&envp, mp);
-  tor_assert(envp);
+  process_environment_t *env = create_managed_proxy_environment(mp);
 
+#ifdef _WIN32
   /* Passing NULL as lpApplicationName makes Windows search for the .exe */
-  retval = tor_spawn_background(NULL, (const char **)mp->argv, envp,
+  retval = tor_spawn_background(NULL,
+                                (const char **)mp->argv,
+                                env,
                                 &mp->process_handle);
-
-  tor_free(envp);
-
 #else
-
-  char **envp=NULL;
-
-  /* prepare the environment variables for the managed proxy */
-  set_managed_proxy_environment(&envp, mp);
-  tor_assert(envp);
-
-  retval = tor_spawn_background(mp->argv[0], (const char **)mp->argv,
-                                (const char **)envp, &mp->process_handle);
-
-  /* free the memory allocated by set_managed_proxy_environment(). */
-  free_execve_args(envp);
-
+  retval = tor_spawn_background(mp->argv[0],
+                                (const char **)mp->argv,
+                                env,
+                                &mp->process_handle);
 #endif
 
+  process_environment_free(env);
+
   if (retval == PROCESS_STATUS_ERROR) {
     log_warn(LD_GENERAL, "Managed proxy at '%s' failed at launch.",
              mp->argv[0]);
@@ -969,170 +952,80 @@ get_bindaddr_for_server_proxy(const managed_proxy_t *mp)
   return bindaddr_result;
 }
 
-#ifdef _WIN32
-
-/** Prepare the environment <b>envp</b> of managed proxy <b>mp</b>.
- *  <b>envp</b> is allocated on the heap and should be freed by the
- *  caller after its use. */
-static void
-set_managed_proxy_environment(LPVOID *envp, const managed_proxy_t *mp)
+/** Return a newly allocated process_environment_t * for <b>mp</b>'s
+ * process. */
+static process_environment_t *
+create_managed_proxy_environment(const managed_proxy_t *mp)
 {
   const or_options_t *options = get_options();
 
-  char *tmp=NULL;
-  char *state_tmp=NULL;
-  char *state_env=NULL;
-  char *transports_to_launch=NULL;
-  char *transports_env=NULL;
-  char *bindaddr_tmp=NULL;
-  char *bindaddr_env=NULL;
-  char *orport_env=NULL;
-
-  char version_env[31]; /* XXX temp */
-  char extended_env[43]; /* XXX temp */
-
-  int env_size = 0;
-
-  /* A smartlist carrying all the env. variables that the managed
-     proxy should inherit. */
+  /* Environment variables to be added to or set in mp's environment. */
   smartlist_t *envs = smartlist_new();
+  /* XXXX The next time someone touches this code, shorten the name of
+   * set_environment_variable_in_smartlist, add a
+   * set_env_var_in_smartlist_asprintf function, and get rid of the
+   * silly extra envs smartlist. */
 
-  /* Copy the whole environment of the Tor process.
-     It should also copy PATH and HOME of the Tor process.*/
-  char **environ_tmp = environ;
-  while (*environ_tmp)
-    smartlist_add(envs, *environ_tmp++);
+  /* The final environment to be passed to mp. */
+  smartlist_t *merged_env_vars = get_current_process_environment_variables();
 
-  /* Create the TOR_PT_* environment variables. */
-  state_tmp = get_datadir_fname("pt_state/"); /* XXX temp */
-  tor_asprintf(&state_env, "TOR_PT_STATE_LOCATION=%s", state_tmp);
+  process_environment_t *env;
 
-  strcpy(version_env, "TOR_PT_MANAGED_TRANSPORT_VER=1");
-
-  transports_to_launch =
-    smartlist_join_strings(mp->transports_to_launch, ",", 0, NULL);
-
-  tor_asprintf(&transports_env,
-               mp->is_server ?
-               "TOR_PT_SERVER_TRANSPORTS=%s" : "TOR_PT_CLIENT_TRANSPORTS=%s",
-               transports_to_launch);
-
-  smartlist_add(envs, state_env);
-  smartlist_add(envs, version_env);
-  smartlist_add(envs, transports_env);
+  {
+    char *state_tmp = get_datadir_fname("pt_state/"); /* XXX temp */
+    smartlist_add_asprintf(envs, "TOR_PT_STATE_LOCATION=%s", state_tmp);
+    tor_free(state_tmp);
+  }
 
-  if (mp->is_server) {
-    tor_asprintf(&orport_env, "TOR_PT_ORPORT=127.0.0.1:%s",
-                 options->ORPort->value);
+  smartlist_add(envs, tor_strdup("TOR_PT_MANAGED_TRANSPORT_VER=1"));
 
-    bindaddr_tmp = get_bindaddr_for_server_proxy(mp);
-    tor_asprintf(&bindaddr_env, "TOR_PT_SERVER_BINDADDR=%s", bindaddr_tmp);
+  {
+    char *transports_to_launch =
+      smartlist_join_strings(mp->transports_to_launch, ",", 0, NULL);
 
-    strlcpy(extended_env, "TOR_PT_EXTENDED_SERVER_PORT=",
-            sizeof(extended_env));
+    smartlist_add_asprintf(envs,
+                           mp->is_server ?
+                           "TOR_PT_SERVER_TRANSPORTS=%s" :
+                           "TOR_PT_CLIENT_TRANSPORTS=%s",
+                           transports_to_launch);
 
-    smartlist_add(envs, orport_env);
-    smartlist_add(envs, extended_env);
-    smartlist_add(envs, bindaddr_env);
+    tor_free(transports_to_launch);
   }
 
-  /* It seems like some versions of Windows need a sorted lpEnvironment
-     block. */
-  smartlist_sort_strings(envs);
-
-  /* An environment block consists of a null-terminated block of
-     null-terminated strings: */
-
-  /* Calculate the block's size. */
-  SMARTLIST_FOREACH(envs, const char *, s,
-                    env_size += strlen(s) + 1);
-  env_size += 1; /* space for last NUL */
-
-  *envp = tor_malloc(env_size);
-  tmp = *envp;
-
-  /* Create the block. */
-  SMARTLIST_FOREACH_BEGIN(envs, const char *, s) {
-    memcpy(tmp, s, strlen(s)); /* copy the env. variable string */
-    tmp += strlen(s);
-    memset(tmp, '\0', 1); /* append NUL at the end of the string */
-    tmp += 1;
-  } SMARTLIST_FOREACH_END(s);
-  memset(tmp, '\0', 1); /* last NUL */
-
-  /* Finally, free the whole mess. */
-  tor_free(state_tmp);
-  tor_free(state_env);
-  tor_free(transports_to_launch);
-  tor_free(transports_env);
-  tor_free(bindaddr_tmp);
-  tor_free(bindaddr_env);
-  tor_free(orport_env);
-
-  smartlist_free(envs);
-}
+  if (mp->is_server) {
+    smartlist_add_asprintf(envs, "TOR_PT_ORPORT=127.0.0.1:%s",
+                           options->ORPort->value);
 
-#else /* _WIN32 */
+    {
+      char *bindaddr_tmp = get_bindaddr_for_server_proxy(mp);
+      smartlist_add_asprintf(envs, "TOR_PT_SERVER_BINDADDR=%s", bindaddr_tmp);
+      tor_free(bindaddr_tmp);
+    }
 
-/** Prepare the environment <b>envp</b> of managed proxy <b>mp</b>.
- *  <b>envp</b> is allocated on the heap and should be freed by the
- *  caller after its use. */
-static void
-set_managed_proxy_environment(char ***envp, const managed_proxy_t *mp)
-{
-  const or_options_t *options = get_options();
-  char **tmp=NULL;
-  char *state_loc=NULL;
-  char *transports_to_launch=NULL;
-  char *bindaddr=NULL;
-  int environ_size=0;
-  char **environ_tmp = get_environment();
-  char **environ_save = environ_tmp;
-
-  int n_envs = mp->is_server ? ENVIRON_SIZE_SERVER : ENVIRON_SIZE_CLIENT;
-
-  while (*environ_tmp) {
-    environ_size++;
-    environ_tmp++;
+    /* XXX023 Remove the '=' here once versions of obfsproxy which
+     * assert that this env var exists are sufficiently dead.
+     *
+     * (If we remove this line entirely, some joker will stick this
+     * variable in Tor's environment and crash PTs that try to parse
+     * it even when not run in server mode.) */
+    smartlist_add(envs, tor_strdup("TOR_PT_EXTENDED_SERVER_PORT="));
   }
-  environ_tmp = environ_save;
 
-  /* allocate enough space for our env. vars and a NULL pointer */
-  *envp = tor_malloc(sizeof(char*)*(environ_size+n_envs+1));
-  tmp = *envp;
+  SMARTLIST_FOREACH_BEGIN(envs, const char *, env_var) {
+    set_environment_variable_in_smartlist(merged_env_vars, env_var,
+                                          _tor_free, 1);
+  } SMARTLIST_FOREACH_END(env_var);
 
-  state_loc = get_datadir_fname("pt_state/"); /* XXX temp */
-  transports_to_launch =
-    smartlist_join_strings(mp->transports_to_launch, ",", 0, NULL);
+  env = process_environment_make(merged_env_vars);
 
-  while (*environ_tmp) {
-    *tmp = tor_strdup(*environ_tmp);
-    tmp++, environ_tmp++;
-  }
+  smartlist_free(envs);
 
-  tor_asprintf(tmp++, "TOR_PT_STATE_LOCATION=%s", state_loc);
-  tor_asprintf(tmp++, "TOR_PT_MANAGED_TRANSPORT_VER=1"); /* temp */
-  if (mp->is_server) {
-    bindaddr = get_bindaddr_for_server_proxy(mp);
-
-    /* XXX temp */
-    tor_asprintf(tmp++, "TOR_PT_ORPORT=127.0.0.1:%d",
-                 router_get_advertised_or_port(options));
-    tor_asprintf(tmp++, "TOR_PT_SERVER_BINDADDR=%s", bindaddr);
-    tor_asprintf(tmp++, "TOR_PT_SERVER_TRANSPORTS=%s", transports_to_launch);
-    tor_asprintf(tmp++, "TOR_PT_EXTENDED_SERVER_PORT=");
-  } else {
-    tor_asprintf(tmp++, "TOR_PT_CLIENT_TRANSPORTS=%s", transports_to_launch);
-  }
-  *tmp = NULL;
+  SMARTLIST_FOREACH(merged_env_vars, void *, x, tor_free(x));
+  smartlist_free(merged_env_vars);
 
-  tor_free(state_loc);
-  tor_free(transports_to_launch);
-  tor_free(bindaddr);
+  return env;
 }
 
-#endif /* _WIN32 */
-
 /** Create and return a new managed proxy for <b>transport</b> using
  *  <b>proxy_argv</b>. If <b>is_server</b> is true, it's a server
  *  managed proxy. */

src/test/test_util.c

@@ -1856,6 +1856,245 @@ test_util_eat_whitespace(void *ptr)
   ;
 }
 
+/** Return a newly allocated smartlist containing the lines of text in
+ * <b>lines</b>.  The returned strings are heap-allocated, and must be
+ * freed by the caller.
+ *
+ * XXXX? Move to container.[hc] ? */
+static smartlist_t *
+smartlist_new_from_text_lines(const char *lines)
+{
+  smartlist_t *sl = smartlist_new();
+  char *last_line;
+
+  smartlist_split_string(sl, lines, "\n", 0, 0);
+
+  last_line = smartlist_pop_last(sl);
+  if (last_line != NULL && *last_line != '\0') {
+    smartlist_add(sl, last_line);
+  }
+
+  return sl;
+}
+
+/** Test smartlist_new_from_text_lines */
+static void
+test_util_sl_new_from_text_lines(void *ptr)
+{
+  (void)ptr;
+
+  { /* Normal usage */
+    smartlist_t *sl = smartlist_new_from_text_lines("foo\nbar\nbaz\n");
+    int sl_len = smartlist_len(sl);
+
+    tt_want_int_op(sl_len, ==, 3);
+
+    if (sl_len > 0) tt_want_str_op(smartlist_get(sl, 0), ==, "foo");
+    if (sl_len > 1) tt_want_str_op(smartlist_get(sl, 1), ==, "bar");
+    if (sl_len > 2) tt_want_str_op(smartlist_get(sl, 2), ==, "baz");
+
+    SMARTLIST_FOREACH(sl, void *, x, tor_free(x));
+    smartlist_free(sl);
+  }
+
+  { /* No final newline */
+    smartlist_t *sl = smartlist_new_from_text_lines("foo\nbar\nbaz");
+    int sl_len = smartlist_len(sl);
+
+    tt_want_int_op(sl_len, ==, 3);
+
+    if (sl_len > 0) tt_want_str_op(smartlist_get(sl, 0), ==, "foo");
+    if (sl_len > 1) tt_want_str_op(smartlist_get(sl, 1), ==, "bar");
+    if (sl_len > 2) tt_want_str_op(smartlist_get(sl, 2), ==, "baz");
+
+    SMARTLIST_FOREACH(sl, void *, x, tor_free(x));
+    smartlist_free(sl);
+  }
+
+  { /* No newlines */
+    smartlist_t *sl = smartlist_new_from_text_lines("foo");
+    int sl_len = smartlist_len(sl);
+
+    tt_want_int_op(sl_len, ==, 1);
+
+    if (sl_len > 0) tt_want_str_op(smartlist_get(sl, 0), ==, "foo");
+
+    SMARTLIST_FOREACH(sl, void *, x, tor_free(x));
+    smartlist_free(sl);
+  }
+
+  { /* No text at all */
+    smartlist_t *sl = smartlist_new_from_text_lines("");
+    int sl_len = smartlist_len(sl);
+
+    tt_want_int_op(sl_len, ==, 0);
+
+    SMARTLIST_FOREACH(sl, void *, x, tor_free(x));
+    smartlist_free(sl);
+  }
+}
+
+/** Test process_environment_make */
+static void
+test_util_make_environment(void *ptr)
+{
+  const char *env_vars_string =
+    "PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/bin\n"
+    "HOME=/home/foozer\n";
+  const char expected_windows_env_block[] =
+    "HOME=/home/foozer\000"
+    "PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/bin\000"
+    "\000";
+  size_t expected_windows_env_block_len = sizeof(expected_windows_env_block);
+
+  smartlist_t *env_vars = smartlist_new_from_text_lines(env_vars_string);
+  smartlist_t *env_vars_sorted = smartlist_new();
+  smartlist_t *env_vars_in_unixoid_env_block_sorted = smartlist_new();
+
+  process_environment_t *env;
+
+  (void)ptr;
+
+  env = process_environment_make(env_vars);
+
+  /* Check that the Windows environment block is correct. */
+  tt_want(tor_memeq(expected_windows_env_block, env->windows_environment_block,
+                    expected_windows_env_block_len));
+
+  /* Now for the Unixoid environment block.  We don't care which order
+   * these environment variables are in, so we sort both lists first. */
+
+  smartlist_add_all(env_vars_sorted, env_vars);
+
+  {
+    char **v;
+    for (v = env->unixoid_environment_block; *v; ++v) {
+      smartlist_add(env_vars_in_unixoid_env_block_sorted, *v);
+    }
+  }
+
+  smartlist_sort_strings(env_vars_sorted);
+  smartlist_sort_strings(env_vars_in_unixoid_env_block_sorted);
+
+  tt_want_int_op(smartlist_len(env_vars_sorted), ==,
+                 smartlist_len(env_vars_in_unixoid_env_block_sorted));
+  {
+    int len = smartlist_len(env_vars_sorted);
+    int i;
+
+    if (smartlist_len(env_vars_in_unixoid_env_block_sorted) < len) {
+      len = smartlist_len(env_vars_in_unixoid_env_block_sorted);
+    }
+
+    for (i = 0; i < len; ++i) {
+      tt_want_str_op(smartlist_get(env_vars_sorted, i), ==,
+                     smartlist_get(env_vars_in_unixoid_env_block_sorted, i));
+    }
+  }
+
+  /* Clean up. */
+  smartlist_free(env_vars_in_unixoid_env_block_sorted);
+  smartlist_free(env_vars_sorted);
+
+  SMARTLIST_FOREACH(env_vars, char *, x, tor_free(x));
+  smartlist_free(env_vars);
+
+  process_environment_free(env);
+}
+
+/** Test set_environment_variable_in_smartlist */
+static void
+test_util_set_env_var_in_sl(void *ptr)
+{
+  /* The environment variables in these strings are in arbitrary
+   * order; we sort the resulting lists before comparing them.
+   *
+   * (They *will not* end up in the order shown in
+   * expected_resulting_env_vars_string.) */
+
+  const char *base_env_vars_string =
+    "PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/bin\n"
+    "HOME=/home/foozer\n"
+    "TERM=xterm\n"
+    "SHELL=/bin/ksh\n"
+    "USER=foozer\n"
+    "LOGNAME=foozer\n"
+    "USERNAME=foozer\n"
+    "LANG=en_US.utf8\n"
+    ;
+
+  const char *new_env_vars_string =
+    "TERM=putty\n"
+    "DISPLAY=:18.0\n"
+    ;
+
+  const char *expected_resulting_env_vars_string =
+    "PATH=/bin:/sbin:/usr/bin:/usr/sbin:/usr/local/bin:/usr/local/bin\n"
+    "HOME=/home/foozer\n"
+    "TERM=putty\n"
+    "SHELL=/bin/ksh\n"
+    "USER=foozer\n"
+    "LOGNAME=foozer\n"
+    "USERNAME=foozer\n"
+    "LANG=en_US.utf8\n"
+    "DISPLAY=:18.0\n"
+    ;
+
+  smartlist_t *merged_env_vars =
+    smartlist_new_from_text_lines(base_env_vars_string);
+  smartlist_t *new_env_vars =
+    smartlist_new_from_text_lines(new_env_vars_string);
+  smartlist_t *expected_resulting_env_vars =
+    smartlist_new_from_text_lines(expected_resulting_env_vars_string);
+
+  /* Elements of merged_env_vars are heap-allocated, and must be
+   * freed.  Some of them are (or should) be freed by
+   * set_environment_variable_in_smartlist.
+   *
+   * Elements of new_env_vars are heap-allocated, but are copied into
+   * merged_env_vars, so they are not freed separately at the end of
+   * the function.
+   *
+   * Elements of expected_resulting_env_vars are heap-allocated, and
+   * must be freed. */
+
+  (void)ptr;
+
+  SMARTLIST_FOREACH(new_env_vars, char *, env_var,
+                    set_environment_variable_in_smartlist(merged_env_vars,
+                                                          env_var,
+                                                          _tor_free,
+                                                          1));
+
+  smartlist_sort_strings(merged_env_vars);
+  smartlist_sort_strings(expected_resulting_env_vars);
+
+  tt_want_int_op(smartlist_len(merged_env_vars), ==,
+                 smartlist_len(expected_resulting_env_vars));
+  {
+    int len = smartlist_len(merged_env_vars);
+    int i;
+
+    if (smartlist_len(expected_resulting_env_vars) < len) {
+      len = smartlist_len(expected_resulting_env_vars);
+    }
+
+    for (i = 0; i < len; ++i) {
+      tt_want_str_op(smartlist_get(merged_env_vars, i), ==,
+                     smartlist_get(expected_resulting_env_vars, i));
+    }
+  }
+
+  /* Clean up. */
+  SMARTLIST_FOREACH(merged_env_vars, char *, x, tor_free(x));
+  smartlist_free(merged_env_vars);
+
+  smartlist_free(new_env_vars);
+
+  SMARTLIST_FOREACH(expected_resulting_env_vars, char *, x, tor_free(x));
+  smartlist_free(expected_resulting_env_vars);
+}
+
 #define UTIL_LEGACY(name)                                               \
   { #name, legacy_test_helper, 0, &legacy_setup, test_util_ ## name }
 
@@ -1895,6 +2134,9 @@ struct testcase_t util_tests[] = {
   UTIL_TEST(split_lines, 0),
   UTIL_TEST(n_bits_set, 0),
   UTIL_TEST(eat_whitespace, 0),
+  UTIL_TEST(sl_new_from_text_lines, 0),
+  UTIL_TEST(make_environment, 0),
+  UTIL_TEST(set_env_var_in_sl, 0),
   END_OF_TESTCASES
 };