Merge remote branch 'sjmurdoch/bug1903'

5 files changed, 251 insertions, 38 deletions

diff --git a/src/common/util.c b/src/common/util.c
index fd6956c3a4..e207ede887 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -2883,17 +2883,34 @@ load_windows_system_library(const TCHAR *library_name)
 }
 #endif
 
-/** Format child_state and saved_errno as a hex string placed in hex_errno.
- * Called between fork and _exit, so must be signal-handler safe */
+/** 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.
+ *
+ * <b>hex_errno</b> must have at least HEX_ERRNO_SIZE bytes available.
+ *
+ * The format of <b>hex_errno</b> is: "CHILD_STATE/ERRNO\n", left-padded
+ * with spaces. Note that there is no trailing \0. CHILD_STATE indicates where
+ * in the processs of starting the child process did the failure occur (see
+ * CHILD_STATE_* macros for definition), and SAVED_ERRNO is the value of
+ * errno when the failure occurred.
+ */
+
 void
 format_helper_exit_status(unsigned char child_state, int saved_errno,
                           char *hex_errno)
 {
-  /* Convert errno to be unsigned for hex conversion */
   unsigned int unsigned_errno;
   char *cur;
+  size_t i;
 
-  /* If errno is negative, negate it */
+  /* Fill hex_errno with spaces, and a trailing newline (memset may
+     not be signal handler safe, so we can't use it) */
+  for (i = 0; i < (HEX_ERRNO_SIZE - 1); i++)
+    hex_errno[i] = ' ';
+  hex_errno[HEX_ERRNO_SIZE - 1] = '\n';
+
+  /* Convert errno to be unsigned for hex conversion */
   if (saved_errno < 0) {
     unsigned_errno = (unsigned int) -saved_errno;
   } else {
@@ -2903,17 +2920,26 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
   /* Convert errno to hex (start before \n) */
   cur = hex_errno + HEX_ERRNO_SIZE - 2;
 
+  /* Check for overflow on first iteration of the loop */
+  if (cur < hex_errno)
+    return;
+
   do {
     *cur-- = "0123456789ABCDEF"[unsigned_errno % 16];
     unsigned_errno /= 16;
   } while (unsigned_errno != 0 && cur >= hex_errno);
 
-  /* Add on the minus side if errno was negative */
-  if (saved_errno < 0)
+  /* Prepend the minus sign if errno was negative */
+  if (saved_errno < 0 && cur >= hex_errno)
     *cur-- = '-';
 
   /* Leave a gap */
-  *cur-- = '/';
+  if (cur >= hex_errno)
+    *cur-- = '/';
+
+  /* Check for overflow on first iteration of the loop */
+  if (cur < hex_errno)
+    return;
 
   /* Convert child_state to hex */
   do {
@@ -2938,13 +2964,20 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
 
 #define SPAWN_ERROR_MESSAGE "ERR: Failed to spawn background process - code "
 
-/** Start a program in the background. If <b>filename</b> contains a '/', then
- * it will be treated as an absolute or relative path.  Otherwise the system
- * path will be searched for <b>filename</b>. Returns pid on success, otherwise
- * returns -1.
- * Some parts of this code are based on the POSIX subprocess module from Python
+/** Start a program in the background. If <b>filename</b> contains a '/',
+ * then it will be treated as an absolute or relative path.  Otherwise the
+ * system path will be searched for <b>filename</b>. The strings in
+ * <b>argv</b> will be passed as the command line arguments of the child
+ * program (following convention, argv[0] should normally be the filename of
+ * the executable). The last element of argv must be NULL. If the child
+ * program is launched, the PID will be returned and <b>stdout_read</b> and
+ * <b>stdout_err</b> will be set to file descriptors from which the stdout
+ * and stderr, respectively, output of the child program can be read, and the
+ * stdin of the child process shall be set to /dev/null.  Otherwise returns
+ * -1.  Some parts of this code are based on the POSIX subprocess module from
+ * Python.
  */
-static int
+int
 tor_spawn_background(const char *const filename, int *stdout_read,
                      int *stderr_read, const char **argv)
 {
@@ -2974,16 +3007,12 @@ tor_spawn_background(const char *const filename, int *stdout_read,
      and we are not allowed to use unsafe functions between fork and exec */
   error_message_length = strlen(error_message);
 
-  /* Fill hex_errno with spaces, and a trailing newline */
-  memset(hex_errno, ' ', sizeof(hex_errno) - 1);
-  hex_errno[sizeof(hex_errno) - 1] = '\n';
-
   child_state = CHILD_STATE_PIPE;
 
   /* Set up pipe for redirecting stdout and stderr of child */
   retval = pipe(stdout_pipe);
   if (-1 == retval) {
-    log_err(LD_GENERAL,
+    log_warn(LD_GENERAL,
       "Failed to set up pipe for stdout communication with child process: %s",
        strerror(errno));
     return -1;
@@ -2991,7 +3020,7 @@ tor_spawn_background(const char *const filename, int *stdout_read,
 
   retval = pipe(stderr_pipe);
   if (-1 == retval) {
-    log_err(LD_GENERAL,
+    log_warn(LD_GENERAL,
       "Failed to set up pipe for stderr communication with child process: %s",
       strerror(errno));
     return -1;
@@ -3043,7 +3072,8 @@ tor_spawn_background(const char *const filename, int *stdout_read,
     child_state = CHILD_STATE_CLOSEFD;
 
     /* Close all other fds, including the read end of the pipe */
-    /* TODO: use closefrom if available */
+    /* XXX: use closefrom if available, or better still set FD_CLOEXEC
+       on all of Tor's open files */
     for (fd = STDERR_FILENO + 1; fd < max_fd; fd++)
       close(fd);
 
@@ -3059,7 +3089,7 @@ tor_spawn_background(const char *const filename, int *stdout_read,
     child_state = CHILD_STATE_FAILEXEC;
 
   error:
-    /* TODO: are we leaking fds from the pipe? */
+    /* XXX: are we leaking fds from the pipe? */
 
     format_helper_exit_status(child_state, errno, hex_errno);
 
@@ -3075,7 +3105,7 @@ tor_spawn_background(const char *const filename, int *stdout_read,
   /* In parent */
 
   if (-1 == pid) {
-    log_err(LD_GENERAL, "Failed to fork child process: %s", strerror(errno));
+    log_warn(LD_GENERAL, "Failed to fork child process: %s", strerror(errno));
     close(stdout_pipe[0]);
     close(stdout_pipe[1]);
     close(stderr_pipe[0]);
@@ -3088,7 +3118,7 @@ tor_spawn_background(const char *const filename, int *stdout_read,
   retval = close(stdout_pipe[1]);
 
   if (-1 == retval) {
-    log_err(LD_GENERAL,
+    log_warn(LD_GENERAL,
             "Failed to close write end of stdout pipe in parent process: %s",
             strerror(errno));
     /* Do not return -1, because the child is running, so the parent
@@ -3099,7 +3129,7 @@ tor_spawn_background(const char *const filename, int *stdout_read,
   retval = close(stderr_pipe[1]);
 
   if (-1 == retval) {
-    log_err(LD_GENERAL,
+    log_warn(LD_GENERAL,
             "Failed to close write end of stderr pipe in parent process: %s",
             strerror(errno));
     /* Do not return -1, because the child is running, so the parent
@@ -3152,26 +3182,27 @@ log_from_pipe(FILE *stream, int severity, const char *executable,
       } else {
         /* No newline; check whether we overflowed the buffer */
         if (!feof(stream))
-          log_err(LD_GENERAL,
+          log_warn(LD_GENERAL,
                   "Line from port forwarding helper was truncated: %s", buf);
           /* TODO: What to do with this error? */
       }
 
       /* Check if buf starts with SPAWN_ERROR_MESSAGE */
-      if (strstr(buf, SPAWN_ERROR_MESSAGE) == buf) {
+      if (strcmpstart(buf, SPAWN_ERROR_MESSAGE) == 0) {
           /* Parse error message */
           int retval, child_state, saved_errno;
-          retval = sscanf(buf, SPAWN_ERROR_MESSAGE "%d/%d",
-                          &child_state, &saved_errno);
+          retval = tor_sscanf(buf, SPAWN_ERROR_MESSAGE "%x/%x",
+                              &child_state, &saved_errno);
           if (retval == 2) {
-              log_err(LD_GENERAL,
+              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 error */
-              log_err(LD_GENERAL,
+              /* 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);
           }
@@ -3237,7 +3268,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
 
     child_pid = tor_spawn_background(filename, &fd_out, &fd_err, argv);
     if (child_pid < 0) {
-      log_err(LD_GENERAL, "Failed to start port forwarding helper %s",
+      log_warn(LD_GENERAL, "Failed to start port forwarding helper %s",
               filename);
       child_pid = -1;
       return;
@@ -3258,7 +3289,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
     /* Read from stdout/stderr and log result */
     retval = 0;
     stdout_status = log_from_pipe(stdout_read, LOG_INFO, filename, &retval);
-    stderr_status = log_from_pipe(stderr_read, LOG_ERR, filename, &retval);
+    stderr_status = log_from_pipe(stderr_read, LOG_WARN, filename, &retval);
     if (retval) {
       /* There was a problem in the child process */
       time_to_run_helper = now + TIME_TO_EXEC_FWHELPER_FAIL;
@@ -3280,7 +3311,7 @@ tor_check_port_forwarding(const char *filename, int dir_port, int or_port,
       if (1 == retval) {
         log_info(LD_GENERAL, "Port forwarding helper terminated");
       } else {
-        log_err(LD_GENERAL, "Failed to read from port forwarding helper");
+        log_warn(LD_GENERAL, "Failed to read from port forwarding helper");
       }
 
       /* TODO: The child might not actually be finished (maybe it failed or
diff --git a/src/common/util.h b/src/common/util.h
index 86555eeb19..8c2a9be325 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -350,6 +350,8 @@ HANDLE load_windows_system_library(const TCHAR *library_name);
 
 #ifdef UTIL_PRIVATE
 /* Prototypes for private functions only used by util.c (and unit tests) */
+int tor_spawn_background(const char *const filename, int *stdout_read,
+                         int *stderr_read, const char **argv);
 void format_helper_exit_status(unsigned char child_state,
                                int saved_errno, char *hex_errno);
 
diff --git a/src/test/Makefile.am b/src/test/Makefile.am
index 594f615036..16ea66583b 100644
--- a/src/test/Makefile.am
+++ b/src/test/Makefile.am
@@ -1,6 +1,6 @@
 TESTS = test
 
-noinst_PROGRAMS = test
+noinst_PROGRAMS = test test-child
 
 AM_CPPFLAGS = -DSHARE_DATADIR="\"$(datadir)\"" \
         -DLOCALSTATEDIR="\"$(localstatedir)\"" \
diff --git a/src/test/test-child.c b/src/test/test-child.c
new file mode 100644
index 0000000000..100e8c0f32
--- /dev/null
+++ b/src/test/test-child.c
@@ -0,0 +1,18 @@
+#include <stdio.h>
+
+/** Trivial test program which prints out its command line arguments so we can
+ * check if tor_spawn_background() works */
+int
+main(int argc, char **argv)
+{
+  int i;
+
+  fprintf(stdout, "OUT\n");
+  fprintf(stderr, "ERR\n");
+  for (i = 0; i < argc; i++)
+    fprintf(stdout, "%s\n", argv[i]);
+  fprintf(stdout, "DONE\n");
+
+  return 0;
+}
+
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 5701f12857..e33a6df88f 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1224,14 +1224,13 @@ test_util_load_win_lib(void *ptr)
 static void
 clear_hex_errno(char *hex_errno)
 {
-  memset(hex_errno, ' ', HEX_ERRNO_SIZE - 2);
-  hex_errno[HEX_ERRNO_SIZE - 1] = '\n';
-  hex_errno[HEX_ERRNO_SIZE] = '\0';
+  memset(hex_errno, '\0', HEX_ERRNO_SIZE + 1);
 }
 
 static void
 test_util_exit_status(void *ptr)
 {
+  /* Leave an extra byte for a \0 so we can do string comparison */
   char hex_errno[HEX_ERRNO_SIZE + 1];
 
   (void)ptr;
@@ -1260,6 +1259,164 @@ test_util_exit_status(void *ptr)
   ;
 }
 
+#ifndef MS_WINDOWS
+/** Check that fgets waits until a full line, and not return a partial line, on
+ * a EAGAIN with a non-blocking pipe */
+static void
+test_util_fgets_eagain(void *ptr)
+{
+  int test_pipe[2] = {-1, -1};
+  int retval;
+  ssize_t retlen;
+  char *retptr;
+  FILE *test_stream = NULL;
+  char buf[10];
+
+  (void)ptr;
+
+  /* Set up a pipe to test on */
+  retval = pipe(test_pipe);
+  tt_int_op(retval, >=, 0);
+
+  /* Set up the read-end to be non-blocking */
+  retval = fcntl(test_pipe[0], F_SETFL, O_NONBLOCK);
+  tt_int_op(retval, >=, 0);
+
+  /* Open it as a stdio stream */
+  test_stream = fdopen(test_pipe[0], "r");
+  tt_ptr_op(test_stream, !=, NULL);
+
+  /* Send in a partial line */
+  retlen = write(test_pipe[1], "A", 1);
+  tt_int_op(retlen, ==, 1);
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_want(retptr == NULL);
+  tt_int_op(errno, ==, EAGAIN);
+
+  /* Send in the rest */
+  retlen = write(test_pipe[1], "B\n", 2);
+  tt_int_op(retlen, ==, 2);
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_ptr_op(retptr, ==, buf);
+  tt_str_op(buf, ==, "AB\n");
+
+  /* Send in a full line */
+  retlen = write(test_pipe[1], "CD\n", 3);
+  tt_int_op(retlen, ==, 3);
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_ptr_op(retptr, ==, buf);
+  tt_str_op(buf, ==, "CD\n");
+
+  /* Send in a partial line */
+  retlen = write(test_pipe[1], "E", 1);
+  tt_int_op(retlen, ==, 1);
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_ptr_op(retptr, ==, NULL);
+  tt_int_op(errno, ==, EAGAIN);
+
+  /* Send in the rest */
+  retlen = write(test_pipe[1], "F\n", 2);
+  tt_int_op(retlen, ==, 2);
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_ptr_op(retptr, ==, buf);
+  tt_str_op(buf, ==, "EF\n");
+
+  /* Send in a full line and close */
+  retlen = write(test_pipe[1], "GH", 2);
+  tt_int_op(retlen, ==, 2);
+  retval = close(test_pipe[1]);
+  test_pipe[1] = -1;
+  tt_int_op(retval, ==, 0);
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_ptr_op(retptr, ==, buf);
+  tt_str_op(buf, ==, "GH");
+
+  /* Check for EOF */
+  retptr = fgets(buf, sizeof(buf), test_stream);
+  tt_ptr_op(retptr, ==, NULL);
+  tt_int_op(feof(test_stream), >, 0);
+
+ done:
+  if (test_stream != NULL)
+    fclose(test_stream);
+  if (test_pipe[0] != -1)
+    close(test_pipe[0]);
+  if (test_pipe[1] != -1)
+    close(test_pipe[1]);
+}
+#endif
+
+#ifndef MS_WINDOWS
+/** Helper function for testing tor_spawn_background */
+static void
+run_util_spawn_background(const char *argv[], const char *expected_out,
+                          const char *expected_err, int expected_exit)
+{
+  int stdout_pipe=-1, stderr_pipe=-1;
+  int retval, stat_loc;
+  pid_t pid;
+  ssize_t pos;
+  char stdout_buf[100], stderr_buf[100];
+
+  /* Start the program */
+  retval = tor_spawn_background(argv[0], &stdout_pipe, &stderr_pipe, argv);
+  tt_int_op(retval, >, 0);
+  tt_int_op(stdout_pipe, >, 0);
+  tt_int_op(stderr_pipe, >, 0);
+  pid = retval;
+
+  /* Check stdout */
+  pos = read(stdout_pipe, stdout_buf, sizeof(stdout_buf) - 1);
+  stdout_buf[pos] = '\0';
+  tt_int_op(pos, ==, strlen(expected_out));
+  tt_str_op(stdout_buf, ==, expected_out);
+
+  /* Check it terminated correctly */
+  retval = waitpid(pid, &stat_loc, 0);
+  tt_int_op(retval, ==, pid);
+  tt_assert(WIFEXITED(stat_loc));
+  tt_int_op(WEXITSTATUS(stat_loc), ==, expected_exit);
+  tt_assert(!WIFSIGNALED(stat_loc));
+  tt_assert(!WIFSTOPPED(stat_loc));
+
+  /* Check stderr */
+  pos = read(stderr_pipe, stderr_buf, sizeof(stderr_buf) - 1);
+  stderr_buf[pos] = '\0';
+  tt_int_op(pos, ==, strlen(expected_err));
+  tt_str_op(stderr_buf, ==, expected_err);
+
+ done:
+  ;
+}
+
+/** Check that we can launch a process and read the output */
+static void
+test_util_spawn_background_ok(void *ptr)
+{
+  const char *argv[] = {"src/test/test-child", "--test", NULL};
+  const char *expected_out = "OUT\nsrc/test/test-child\n--test\nDONE\n";
+  const char *expected_err = "ERR\n";
+
+  (void)ptr;
+
+  run_util_spawn_background(argv, expected_out, expected_err, 0);
+}
+
+/** Check that failing to find the executable works as expected */
+static void
+test_util_spawn_background_fail(void *ptr)
+{
+  const char *argv[] = {"src/test/no-such-file", "--test", NULL};
+  const char *expected_out = "ERR: Failed to spawn background process "
+                             "- code          9/2\n";
+  const char *expected_err = "";
+
+  (void)ptr;
+
+  run_util_spawn_background(argv, expected_out, expected_err, 255);
+}
+#endif
+
 #define UTIL_LEGACY(name)                                               \
   { #name, legacy_test_helper, 0, &legacy_setup, test_util_ ## name }
 
@@ -1287,6 +1444,11 @@ struct testcase_t util_tests[] = {
   UTIL_TEST(load_win_lib, 0),
 #endif
   UTIL_TEST(exit_status, 0),
+#ifndef MS_WINDOWS
+  UTIL_TEST(fgets_eagain, TT_SKIP),
+  UTIL_TEST(spawn_background_ok, 0),
+  UTIL_TEST(spawn_background_fail, 0),
+#endif
   END_OF_TESTCASES
 };