1 files changed, 294 insertions, 106 deletions

diff --git a/src/common/util.c b/src/common/util.c
index 5eb0f9a69b..04cc6b12c6 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -24,6 +24,9 @@
 #include "torint.h"
 #include "container.h"
 #include "address.h"
+#include "sandbox.h"
+#include "backtrace.h"
+#include "util_process.h"
 
 #ifdef _WIN32
 #include <io.h>
@@ -94,6 +97,23 @@
 #endif
 
 /* =====
+ * Assertion helper.
+ * ===== */
+/** Helper for tor_assert: report the assertion failure. */
+void
+tor_assertion_failed_(const char *fname, unsigned int line,
+                      const char *func, const char *expr)
+{
+  char buf[256];
+  log_err(LD_BUG, "%s:%u: %s: Assertion %s failed; aborting.",
+          fname, line, func, expr);
+  tor_snprintf(buf, sizeof(buf),
+               "Assertion %s failed in %s at %s:%u",
+               expr, func, fname, line);
+  log_backtrace(LOG_ERR, LD_BUG, buf);
+}
+
+/* =====
  * Memory management
  * ===== */
 #ifdef USE_DMALLOC
@@ -284,7 +304,7 @@ tor_memdup_(const void *mem, size_t len DMALLOC_PARAMS)
 /** As tor_memdup(), but add an extra 0 byte at the end of the resulting
  * memory. */
 void *
-tor_memdup_nulterm(const void *mem, size_t len DMALLOC_PARAMS)
+tor_memdup_nulterm_(const void *mem, size_t len DMALLOC_PARAMS)
 {
   char *dup;
   tor_assert(len < SIZE_T_CEILING+1);
@@ -879,6 +899,39 @@ tor_digest_is_zero(const char *digest)
   return tor_memeq(digest, ZERO_DIGEST, DIGEST_LEN);
 }
 
+/** Return true if <b>string</b> is a valid 'key=[value]' string.
+ *  "value" is optional, to indicate the empty string. Log at logging
+ *  <b>severity</b> if something ugly happens. */
+int
+string_is_key_value(int severity, const char *string)
+{
+  /* position of equal sign in string */
+  const char *equal_sign_pos = NULL;
+
+  tor_assert(string);
+
+  if (strlen(string) < 2) { /* "x=" is shortest args string */
+    tor_log(severity, LD_GENERAL, "'%s' is too short to be a k=v value.",
+            escaped(string));
+    return 0;
+  }
+
+  equal_sign_pos = strchr(string, '=');
+  if (!equal_sign_pos) {
+    tor_log(severity, LD_GENERAL, "'%s' is not a k=v value.", escaped(string));
+    return 0;
+  }
+
+  /* validate that the '=' is not in the beginning of the string. */
+  if (equal_sign_pos == string) {
+    tor_log(severity, LD_GENERAL, "'%s' is not a valid k=v value.",
+            escaped(string));
+    return 0;
+  }
+
+  return 1;
+}
+
 /** Return true iff the DIGEST256_LEN bytes in digest are all zero. */
 int
 tor_digest256_is_zero(const char *digest)
@@ -1076,6 +1129,9 @@ base16_decode(char *dest, size_t destlen, const char *src, size_t srclen)
     return -1;
   if (destlen < srclen/2 || destlen > SIZE_T_CEILING)
     return -1;
+
+  memset(dest, 0, destlen);
+
   end = src+srclen;
   while (src<end) {
     v1 = hex_decode_digit_(*src);
@@ -1190,6 +1246,43 @@ escaped(const char *s)
   return escaped_val_;
 }
 
+/** Return a newly allocated string equal to <b>string</b>, except that every
+ * character in <b>chars_to_escape</b> is preceded by a backslash. */
+char *
+tor_escape_str_for_pt_args(const char *string, const char *chars_to_escape)
+{
+  char *new_string = NULL;
+  char *new_cp = NULL;
+  size_t length, new_length;
+
+  tor_assert(string);
+
+  length = strlen(string);
+
+  if (!length) /* If we were given the empty string, return the same. */
+    return tor_strdup("");
+  /* (new_length > SIZE_MAX) => ((length * 2) + 1 > SIZE_MAX) =>
+     (length*2 > SIZE_MAX - 1) => (length > (SIZE_MAX - 1)/2) */
+  if (length > (SIZE_MAX - 1)/2) /* check for overflow */
+    return NULL;
+
+  /* this should be enough even if all characters must be escaped */
+  new_length = (length * 2) + 1;
+
+  new_string = new_cp = tor_malloc(new_length);
+
+  while (*string) {
+    if (strchr(chars_to_escape, *string))
+      *new_cp++ = '\\';
+
+    *new_cp++ = *string++;
+  }
+
+  *new_cp = '\0'; /* NUL-terminate the new string */
+
+  return new_string;
+}
+
 /* =====
  * Time
  * ===== */
@@ -1427,7 +1520,7 @@ void
 format_iso_time_nospace_usec(char *buf, const struct timeval *tv)
 {
   tor_assert(tv);
-  format_iso_time_nospace(buf, tv->tv_sec);
+  format_iso_time_nospace(buf, (time_t)tv->tv_sec);
   tor_snprintf(buf+ISO_TIME_LEN, 8, ".%06d", (int)tv->tv_usec);
 }
 
@@ -1741,7 +1834,8 @@ file_status(const char *fname)
   int r;
   f = tor_strdup(fname);
   clean_name_for_stat(f);
-  r = stat(f, &st);
+  log_debug(LD_FS, "stat()ing %s", f);
+  r = stat(sandbox_intern_string(f), &st);
   tor_free(f);
   if (r) {
     if (errno == ENOENT) {
@@ -1781,7 +1875,7 @@ check_private_dir(const char *dirname, cpd_check_t check,
   char *f;
 #ifndef _WIN32
   int mask;
-  struct passwd *pw = NULL;
+  const struct passwd *pw = NULL;
   uid_t running_uid;
   gid_t running_gid;
 #else
@@ -1791,7 +1885,8 @@ check_private_dir(const char *dirname, cpd_check_t check,
   tor_assert(dirname);
   f = tor_strdup(dirname);
   clean_name_for_stat(f);
-  r = stat(f, &st);
+  log_debug(LD_FS, "stat()ing %s", f);
+  r = stat(sandbox_intern_string(f), &st);
   tor_free(f);
   if (r) {
     if (errno != ENOENT) {
@@ -1827,7 +1922,7 @@ check_private_dir(const char *dirname, cpd_check_t check,
   if (effective_user) {
     /* Look up the user and group information.
      * If we have a problem, bail out. */
-    pw = getpwnam(effective_user);
+    pw = tor_getpwnam(effective_user);
     if (pw == NULL) {
       log_warn(LD_CONFIG, "Error setting configured user: %s not found",
                effective_user);
@@ -1841,13 +1936,13 @@ check_private_dir(const char *dirname, cpd_check_t check,
   }
 
   if (st.st_uid != running_uid) {
-    struct passwd *pw = NULL;
+    const struct passwd *pw = NULL;
     char *process_ownername = NULL;
 
-    pw = getpwuid(running_uid);
+    pw = tor_getpwuid(running_uid);
     process_ownername = pw ? tor_strdup(pw->pw_name) : tor_strdup("<unknown>");
 
-    pw = getpwuid(st.st_uid);
+    pw = tor_getpwuid(st.st_uid);
 
     log_warn(LD_FS, "%s is not owned by this user (%s, %d) but by "
         "%s (%d). Perhaps you are running Tor as the wrong user?",
@@ -1913,7 +2008,8 @@ write_str_to_file(const char *fname, const char *str, int bin)
 #ifdef _WIN32
   if (!bin && strchr(str, '\r')) {
     log_warn(LD_BUG,
-             "We're writing a text string that already contains a CR.");
+             "We're writing a text string that already contains a CR to %s",
+             escaped(fname));
   }
 #endif
   return write_bytes_to_file(fname, str, strlen(str), bin);
@@ -1977,8 +2073,10 @@ start_writing_to_file(const char *fname, int open_flags, int mode,
     open_flags &= ~O_EXCL;
     new_file->rename_on_close = 1;
   }
+#if O_BINARY != 0
   if (open_flags & O_BINARY)
     new_file->binary = 1;
+#endif
 
   new_file->fd = tor_open_cloexec(open_name, open_flags, mode);
   if (new_file->fd < 0) {
@@ -2050,6 +2148,7 @@ static int
 finish_writing_to_file_impl(open_file_t *file_data, int abort_write)
 {
   int r = 0;
+
   tor_assert(file_data && file_data->filename);
   if (file_data->stdio_file) {
     if (fclose(file_data->stdio_file)) {
@@ -2066,7 +2165,13 @@ finish_writing_to_file_impl(open_file_t *file_data, int abort_write)
   if (file_data->rename_on_close) {
     tor_assert(file_data->tempname && file_data->filename);
     if (abort_write) {
-      unlink(file_data->tempname);
+      int res = unlink(file_data->tempname);
+      if (res != 0) {
+        /* We couldn't unlink and we'll leave a mess behind */
+        log_warn(LD_FS, "Failed to unlink %s: %s",
+                 file_data->tempname, strerror(errno));
+        r = -1;
+      }
     } else {
       tor_assert(strcmp(file_data->filename, file_data->tempname));
       if (replace_file(file_data->tempname, file_data->filename)) {
@@ -2132,12 +2237,20 @@ write_chunks_to_file_impl(const char *fname, const smartlist_t *chunks,
   return -1;
 }
 
-/** Given a smartlist of sized_chunk_t, write them atomically to a file
- * <b>fname</b>, overwriting or creating the file as necessary. */
+/** Given a smartlist of sized_chunk_t, write them to a file
+ * <b>fname</b>, overwriting or creating the file as necessary.
+ * If <b>no_tempfile</b> is 0 then the file will be written
+ * atomically. */
 int
-write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin)
+write_chunks_to_file(const char *fname, const smartlist_t *chunks, int bin,
+                     int no_tempfile)
 {
   int flags = OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT);
+
+  if (no_tempfile) {
+    /* O_APPEND stops write_chunks_to_file from using tempfiles */
+    flags |= O_APPEND;
+  }
   return write_chunks_to_file_impl(fname, chunks, flags);
 }
 
@@ -2158,9 +2271,9 @@ write_bytes_to_file_impl(const char *fname, const char *str, size_t len,
 
 /** As write_str_to_file, but does not assume a NUL-terminated
  * string. Instead, we write <b>len</b> bytes, starting at <b>str</b>. */
-int
-write_bytes_to_file(const char *fname, const char *str, size_t len,
-                    int bin)
+MOCK_IMPL(int,
+write_bytes_to_file,(const char *fname, const char *str, size_t len,
+                     int bin))
 {
   return write_bytes_to_file_impl(fname, str, len,
                                   OPEN_FLAGS_REPLACE|(bin?O_BINARY:O_TEXT));
@@ -2927,7 +3040,7 @@ tor_vsscanf(const char *buf, const char *pattern, va_list ap)
 /** Minimal sscanf replacement: parse <b>buf</b> according to <b>pattern</b>
  * and store the results in the corresponding argument fields.  Differs from
  * sscanf in that:
- * <ul><li>It only handles %u, %lu, %x, %lx, %<NUM>s, %d, %ld, %lf, and %c.
+ * <ul><li>It only handles %u, %lu, %x, %lx, %[NUM]s, %d, %ld, %lf, and %c.
  *     <li>It only handles decimal inputs for %lf. (12.3, not 1.23e1)
  *     <li>It does not handle arbitrarily long widths.
  *     <li>Numbers do not consume any space characters.
@@ -3022,9 +3135,10 @@ tor_listdir(const char *dirname)
   FindClose(handle);
   tor_free(pattern);
 #else
+  const char *prot_dname = sandbox_intern_string(dirname);
   DIR *d;
   struct dirent *de;
-  if (!(d = opendir(dirname)))
+  if (!(d = opendir(prot_dname)))
     return NULL;
 
   result = smartlist_new();
@@ -3320,14 +3434,59 @@ tor_join_win_cmdline(const char *argv[])
   return joined_argv;
 }
 
+/* As format_{hex,dex}_number_sigsafe, but takes a <b>radix</b> argument
+ * in range 2..16 inclusive. */
+static int
+format_number_sigsafe(unsigned long x, char *buf, int buf_len,
+                      unsigned int radix)
+{
+  unsigned long tmp;
+  int len;
+  char *cp;
+
+  /* NOT tor_assert. This needs to be safe to run from within a signal handler,
+   * and from within the 'tor_assert() has failed' code. */
+  if (radix < 2 || radix > 16)
+    return 0;
+
+  /* Count how many digits we need. */
+  tmp = x;
+  len = 1;
+  while (tmp >= radix) {
+    tmp /= radix;
+    ++len;
+  }
+
+  /* Not long enough */
+  if (!buf || len >= buf_len)
+    return 0;
+
+  cp = buf + len;
+  *cp = '\0';
+  do {
+    unsigned digit = (unsigned) (x % radix);
+    tor_assert(cp > buf);
+    --cp;
+    *cp = "0123456789ABCDEF"[digit];
+    x /= radix;
+  } while (x);
+
+  /* NOT tor_assert; see above. */
+  if (cp != buf) {
+    abort();
+  }
+
+  return len;
+}
+
 /**
- * Helper function to output hex numbers, called by
- * format_helper_exit_status().  This writes the hexadecimal digits of x into
- * buf, up to max_len digits, and returns the actual number of digits written.
- * If there is insufficient space, it will write nothing and return 0.
+ * Helper function to output hex numbers from within a signal handler.
+ *
+ * Writes the nul-terminated hexadecimal digits of <b>x</b> into a buffer
+ * <b>buf</b> of size <b>buf_len</b>, and return the actual number of digits
+ * written, not counting the terminal NUL.
  *
- * This function DOES NOT add a terminating NUL character to its output: be
- * careful!
+ * If there is insufficient space, write nothing and return 0.
  *
  * This accepts an unsigned int because format_helper_exit_status() needs to
  * call it with a signed int and an unsigned char, and since the C standard
@@ -3342,46 +3501,19 @@ tor_join_win_cmdline(const char *argv[])
  * arbitrary C functions.
  */
 int
-format_hex_number_for_helper_exit_status(unsigned int x, char *buf,
-                                         int max_len)
+format_hex_number_sigsafe(unsigned long x, char *buf, int buf_len)
 {
-  int len;
-  unsigned int tmp;
-  char *cur;
-
-  /* Sanity check */
-  if (!buf || max_len <= 0)
-    return 0;
-
-  /* How many chars do we need for x? */
-  if (x > 0) {
-    len = 0;
-    tmp = x;
-    while (tmp > 0) {
-      tmp >>= 4;
-      ++len;
-    }
-  } else {
-    len = 1;
-  }
-
-  /* Bail if we would go past the end of the buffer */
-  if (len > max_len)
-    return 0;
-
-  /* Point to last one */
-  cur = buf + len - 1;
-
-  /* Convert x to hex */
-  do {
-    *cur-- = "0123456789ABCDEF"[x & 0xf];
-    x >>= 4;
-  } while (x != 0 && cur >= buf);
+  return format_number_sigsafe(x, buf, buf_len, 16);
+}
 
-  /* Return len */
-  return len;
+/** As format_hex_number_sigsafe, but format the number in base 10. */
+int
+format_dec_number_sigsafe(unsigned long x, char *buf, int buf_len)
+{
+  return format_number_sigsafe(x, buf, buf_len, 10);
 }
 
+#ifndef _WIN32
 /** Format <b>child_state</b> and <b>saved_errno</b> as a hex string placed in
  * <b>hex_errno</b>.  Called between fork and _exit, so must be signal-handler
  * safe.
@@ -3397,7 +3529,7 @@ format_hex_number_for_helper_exit_status(unsigned int x, char *buf,
  * On success return the number of characters added to hex_errno, not counting
  * the terminating NUL; return -1 on error.
  */
-int
+STATIC int
 format_helper_exit_status(unsigned char child_state, int saved_errno,
                           char *hex_errno)
 {
@@ -3428,8 +3560,8 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
   cur = hex_errno;
 
   /* Emit child_state */
-  written = format_hex_number_for_helper_exit_status(child_state,
-                                                     cur, left);
+  written = format_hex_number_sigsafe(child_state, cur, left);
+
   if (written <= 0)
     goto err;
 
@@ -3458,8 +3590,7 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
   }
 
   /* Emit unsigned_errno */
-  written = format_hex_number_for_helper_exit_status(unsigned_errno,
-                                                     cur, left);
+  written = format_hex_number_sigsafe(unsigned_errno, cur, left);
 
   if (written <= 0)
     goto err;
@@ -3490,6 +3621,7 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
  done:
   return res;
 }
+#endif
 
 /* Maximum number of file descriptors, if we cannot get it via sysconf() */
 #define DEFAULT_MAX_FD 256
@@ -3501,13 +3633,7 @@ tor_terminate_process(process_handle_t *process_handle)
 {
 #ifdef _WIN32
   if (tor_get_exit_code(process_handle, 0, NULL) == PROCESS_EXIT_RUNNING) {
-    HANDLE handle;
-    /* If the signal is outside of what GenerateConsoleCtrlEvent can use,
-       attempt to open and terminate the process. */
-    handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE,
-                         process_handle->pid.dwProcessId);
-    if (!handle)
-      return -1;
+    HANDLE handle = process_handle->pid.hProcess;
 
     if (!TerminateProcess(handle, 0))
       return -1;
@@ -3515,7 +3641,10 @@ tor_terminate_process(process_handle_t *process_handle)
       return 0;
   }
 #else /* Unix */
-  return kill(process_handle->pid, SIGTERM);
+  if (process_handle->waitpid_cb) {
+    /* We haven't got a waitpid yet, so we can just kill off the process. */
+    return kill(process_handle->pid, SIGTERM);
+  }
 #endif
 
   return -1;
@@ -3564,6 +3693,23 @@ process_handle_new(void)
   return out;
 }
 
+#ifndef _WIN32
+/** Invoked when a process that we've launched via tor_spawn_background() has
+ * been found to have terminated.
+ */
+static void
+process_handle_waitpid_cb(int status, void *arg)
+{
+  process_handle_t *process_handle = arg;
+
+  process_handle->waitpid_exit_status = status;
+  clear_waitpid_callback(process_handle->waitpid_cb);
+  if (process_handle->status == PROCESS_STATUS_RUNNING)
+    process_handle->status = PROCESS_STATUS_NOTRUNNING;
+  process_handle->waitpid_cb = 0;
+}
+#endif
+
 /**
  * @name child-process states
  *
@@ -3685,7 +3831,7 @@ tor_spawn_background(const char *const filename, const char **argv,
                  TRUE,          // handles are inherited
   /*(TODO: set CREATE_NEW CONSOLE/PROCESS_GROUP to make GetExitCodeProcess()
    * work?) */
-                 0,             // creation flags
+                 CREATE_NO_WINDOW,             // creation flags
                  (env==NULL) ? NULL : env->windows_environment_block,
                  NULL,          // use parent's current directory
                  &siStartInfo,  // STARTUPINFO pointer
@@ -3880,6 +4026,10 @@ tor_spawn_background(const char *const filename, const char **argv,
             strerror(errno));
   }
 
+  process_handle->waitpid_cb = set_waitpid_callback(pid,
+                                                    process_handle_waitpid_cb,
+                                                    process_handle);
+
   process_handle->stderr_pipe = stderr_pipe[0];
   retval = close(stderr_pipe[1]);
 
@@ -3906,9 +4056,9 @@ tor_spawn_background(const char *const filename, const char **argv,
  *  <b>process_handle</b>.
  *  If <b>also_terminate_process</b> is true, also terminate the
  *  process of the process handle. */
-void
-tor_process_handle_destroy(process_handle_t *process_handle,
-                           int also_terminate_process)
+MOCK_IMPL(void,
+tor_process_handle_destroy,(process_handle_t *process_handle,
+                            int also_terminate_process))
 {
   if (!process_handle)
     return;
@@ -3944,6 +4094,8 @@ tor_process_handle_destroy(process_handle_t *process_handle,
 
   if (process_handle->stderr_handle)
     fclose(process_handle->stderr_handle);
+
+  clear_waitpid_callback(process_handle->waitpid_cb);
 #endif
 
   memset(process_handle, 0x0f, sizeof(process_handle_t));
@@ -3961,7 +4113,7 @@ tor_process_handle_destroy(process_handle_t *process_handle,
  * probably not work in Tor, because waitpid() is called in main.c to reap any
  * terminated child processes.*/
 int
-tor_get_exit_code(const process_handle_t *process_handle,
+tor_get_exit_code(process_handle_t *process_handle,
                   int block, int *exit_code)
 {
 #ifdef _WIN32
@@ -4001,7 +4153,20 @@ tor_get_exit_code(const process_handle_t *process_handle,
   int stat_loc;
   int retval;
 
-  retval = waitpid(process_handle->pid, &stat_loc, block?0:WNOHANG);
+  if (process_handle->waitpid_cb) {
+    /* We haven't processed a SIGCHLD yet. */
+    retval = waitpid(process_handle->pid, &stat_loc, block?0:WNOHANG);
+    if (retval == process_handle->pid) {
+      clear_waitpid_callback(process_handle->waitpid_cb);
+      process_handle->waitpid_cb = NULL;
+      process_handle->waitpid_exit_status = stat_loc;
+    }
+  } else {
+    /* We already got a SIGCHLD for this process, and handled it. */
+    retval = process_handle->pid;
+    stat_loc = process_handle->waitpid_exit_status;
+  }
+
   if (!block && 0 == retval) {
     /* Process has not exited */
     return PROCESS_EXIT_RUNNING;
@@ -4412,14 +4577,38 @@ stream_status_to_string(enum stream_status stream_status)
   }
 }
 
+/* DOCDOC */
+static void
+log_portfw_spawn_error_message(const char *buf,
+                               const char *executable, int *child_status)
+{
+  /* Parse error message */
+  int retval, child_state, saved_errno;
+  retval = tor_sscanf(buf, SPAWN_ERROR_MESSAGE "%x/%x",
+                      &child_state, &saved_errno);
+  if (retval == 2) {
+    log_warn(LD_GENERAL,
+             "Failed to start child process \"%s\" in state %d: %s",
+             executable, child_state, strerror(saved_errno));
+    if (child_status)
+      *child_status = 1;
+  } else {
+    /* Failed to parse message from child process, log it as a
+       warning */
+    log_warn(LD_GENERAL,
+             "Unexpected message from port forwarding helper \"%s\": %s",
+             executable, buf);
+  }
+}
+
 #ifdef _WIN32
 
 /** Return a smartlist containing lines outputted from
  *  <b>handle</b>. Return NULL on error, and set
  *  <b>stream_status_out</b> appropriately. */
-smartlist_t *
-tor_get_lines_from_handle(HANDLE *handle,
-                          enum stream_status *stream_status_out)
+MOCK_IMPL(smartlist_t *,
+tor_get_lines_from_handle, (HANDLE *handle,
+                            enum stream_status *stream_status_out))
 {
   int pos;
   char stdout_buf[600] = {0};
@@ -4507,8 +4696,9 @@ log_from_handle(HANDLE *pipe, int severity)
 /** Return a smartlist containing lines outputted from
  *  <b>handle</b>. Return NULL on error, and set
  *  <b>stream_status_out</b> appropriately. */
-smartlist_t *
-tor_get_lines_from_handle(FILE *handle, enum stream_status *stream_status_out)
+MOCK_IMPL(smartlist_t *,
+tor_get_lines_from_handle, (FILE *handle,
+                            enum stream_status *stream_status_out))
 {
   enum stream_status stream_status;
   char stdout_buf[400];
@@ -4558,23 +4748,7 @@ log_from_pipe(FILE *stream, int severity, const char *executable,
 
     /* Check if buf starts with SPAWN_ERROR_MESSAGE */
     if (strcmpstart(buf, SPAWN_ERROR_MESSAGE) == 0) {
-      /* Parse error message */
-      int retval, child_state, saved_errno;
-      retval = tor_sscanf(buf, SPAWN_ERROR_MESSAGE "%x/%x",
-                          &child_state, &saved_errno);
-      if (retval == 2) {
-        log_warn(LD_GENERAL,
-                 "Failed to start child process \"%s\" in state %d: %s",
-                 executable, child_state, strerror(saved_errno));
-        if (child_status)
-          *child_status = 1;
-      } else {
-        /* Failed to parse message from child process, log it as a
-           warning */
-        log_warn(LD_GENERAL,
-                 "Unexpected message from port forwarding helper \"%s\": %s",
-                 executable, buf);
-      }
+      log_portfw_spawn_error_message(buf, executable, child_status);
     } else {
       log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf);
     }
@@ -4652,7 +4826,7 @@ get_string_from_pipe(FILE *stream, char *buf_out, size_t count)
 /** Parse a <b>line</b> from tor-fw-helper and issue an appropriate
  *  log message to our user. */
 static void
-handle_fw_helper_line(const char *line)
+handle_fw_helper_line(const char *executable, const char *line)
 {
   smartlist_t *tokens = smartlist_new();
   char *message = NULL;
@@ -4663,6 +4837,19 @@ handle_fw_helper_line(const char *line)
   int port = 0;
   int success = 0;
 
+  if (strcmpstart(line, SPAWN_ERROR_MESSAGE) == 0) {
+    /* We need to check for SPAWN_ERROR_MESSAGE again here, since it's
+     * possible that it got sent after we tried to read it in log_from_pipe.
+     *
+     * XXX Ideally, we should be using one of stdout/stderr for the real
+     * output, and one for the output of the startup code.  We used to do that
+     * before cd05f35d2c.
+     */
+    int child_status;
+    log_portfw_spawn_error_message(line, executable, &child_status);
+    goto done;
+  }
+
   smartlist_split_string(tokens, line, NULL,
                          SPLIT_SKIP_SPACE|SPLIT_IGNORE_BLANK, -1);
 
@@ -4742,7 +4929,8 @@ handle_fw_helper_line(const char *line)
 /** Read what tor-fw-helper has to say in its stdout and handle it
  *  appropriately */
 static int
-handle_fw_helper_output(process_handle_t *process_handle)
+handle_fw_helper_output(const char *executable,
+                        process_handle_t *process_handle)
 {
   smartlist_t *fw_helper_output = NULL;
   enum stream_status stream_status = 0;
@@ -4757,7 +4945,7 @@ handle_fw_helper_output(process_handle_t *process_handle)
 
   /* Handle the lines we got: */
   SMARTLIST_FOREACH_BEGIN(fw_helper_output, char *, line) {
-    handle_fw_helper_line(line);
+    handle_fw_helper_line(executable, line);
     tor_free(line);
   } SMARTLIST_FOREACH_END(line);
 
@@ -4872,7 +5060,7 @@ tor_check_port_forwarding(const char *filename,
     stderr_status = log_from_pipe(child_handle->stderr_handle,
                                   LOG_INFO, filename, &retval);
 #endif
-    if (handle_fw_helper_output(child_handle) < 0) {
+    if (handle_fw_helper_output(filename, child_handle) < 0) {
       log_warn(LD_GENERAL, "Failed to handle fw helper output.");
       stdout_status = -1;
       retval = -1;