Add code to read all from a handle, but this block forever

See http://stackoverflow.com/questions/3722409/windows-child-process-with-redirected-input-and-output
for a potential solution

src/common/util.c

@@ -3195,6 +3195,7 @@ tor_spawn_background(const char *const filename, const char **argv)
 
     /* Write the error message. GCC requires that we check the return
        value, but there is nothing we can do if it fails */
+    // TODO: Don't use STDOUT, use a pipe set up just for this purpose
     nbytes = write(STDOUT_FILENO, error_message, error_message_length);
     nbytes = write(STDOUT_FILENO, hex_errno, sizeof(hex_errno));
 
@@ -3217,6 +3218,8 @@ tor_spawn_background(const char *const filename, const char **argv)
     return process_handle;
   }
 
+  // TODO: If the child process forked but failed to exec, waitpid it
+
   /* Return read end of the pipes to caller, and close write end */
   process_handle.stdout_pipe = stdout_pipe[0];
   retval = close(stdout_pipe[1]);
@@ -3281,22 +3284,41 @@ tor_get_exit_code(const process_handle_t process_handle)
 #endif // MS_WINDOWS
 }
 
+#ifdef MS_WINDOWS
+/* Windows equivalent of read_all */
+static ssize_t
+read_all_handle(HANDLE h, char *buf, size_t count)
+{
+  size_t numread = 0;
+  BOOL retval;
+  DWORD bytes_read;
+
+  if (count > SIZE_T_CEILING || count > SSIZE_T_MAX)
+    return -1;
+
+  while (numread != count) {
+    retval = ReadFile(h, buf+numread, count-numread, &bytes_read, NULL);
+    if (!retval) {
+      log_warn(LD_GENERAL,
+        "Failed to read from stdin pipe: %s",
+        format_win32_error(GetLastError()));
+      return -1;
+    } else if (0 == bytes_read) {
+      /* End of file */
+      return bytes_read;
+    }
+    numread += bytes_read;
+  }
+  return (ssize_t)numread;
+}
+#endif
+
 ssize_t
 tor_read_all_from_process_stdout(const process_handle_t process_handle,
                                 char *buf, size_t count)
 {
 #ifdef MS_WINDOWS
-  BOOL retval;
+  return read_all_handle(process_handle.stdout_pipe, buf, count);
-  DWORD bytes_read;
-  retval = ReadFile(process_handle.stdout_pipe, buf, count, &bytes_read, NULL);
-  if (!retval) {
-    log_warn(LD_GENERAL,
-      "Failed to read from stdin pipe: %s",
-      format_win32_error(GetLastError()));
-    return -1;
-  } else {
-    return bytes_read;
-  }
 #else
   return read_all(process_handle.stdout_pipe, buf, count, 0);
 #endif
@@ -3307,17 +3329,7 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle,
                                  char *buf, size_t count)
 {
 #ifdef MS_WINDOWS
-  BOOL retval;
+  return read_all_handle(process_handle.stderr_pipe, buf, count);
-  DWORD bytes_read;
-  retval = ReadFile(process_handle.stderr_pipe, buf, count, &bytes_read, NULL);
-  if (!retval) {
-    log_warn(LD_GENERAL,
-      "Failed to read from stderr pipe: %s",
-      format_win32_error(GetLastError()));
-    return -1;
-  } else {
-    return bytes_read;
-  }
 #else
   return read_all(process_handle.stderr_pipe, buf, count, 0);
 #endif

src/test/test-child.c

@@ -1,4 +1,11 @@
 #include <stdio.h>
+#include "orconfig.h"
+#ifdef MS_WINDOWS
+#define WINDOWS_LEAN_AND_MEAN
+#include <windows.h>
+#else
+#include <unistd.h>
+#endif
 
 /** Trivial test program which prints out its command line arguments so we can
  * check if tor_spawn_background() works */
@@ -11,7 +18,18 @@ main(int argc, char **argv)
   fprintf(stderr, "ERR\n");
   for (i = 1; i < argc; i++)
     fprintf(stdout, "%s\n", argv[i]);
+  fprintf(stdout, "SLEEPING\n");
+#ifdef MS_WINDOWS
+  Sleep(1000);
+#else
+  sleep(1);
+#endif
   fprintf(stdout, "DONE\n");
+#ifdef MS_WINDOWS
+  Sleep(1000);
+#else
+  sleep(1);
+#endif
 
   return 0;
 }

src/test/test_util.c

@@ -1379,7 +1379,8 @@ test_util_fgets_eagain(void *ptr)
 /** 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)
+                          const char *expected_err, int expected_exit,
+                          int expected_status)
 {
   int retval;
   ssize_t pos;
@@ -1389,7 +1390,12 @@ run_util_spawn_background(const char *argv[], const char *expected_out,
   /* Start the program */
   process_handle = tor_spawn_background(argv[0], argv);
 
-  tt_int_op(process_handle.status, ==, 0);
+  tt_int_op(process_handle.status, ==, expected_status);
+  
+  /* If the process failed to start, don't bother continuing */
+  if (process_handle.status == -1)
+    return;
+
   tt_int_op(process_handle.stdout_pipe, >, 0);
   tt_int_op(process_handle.stderr_pipe, >, 0);
 
@@ -1435,21 +1441,31 @@ test_util_spawn_background_ok(void *ptr)
 
   (void)ptr;
 
-  run_util_spawn_background(argv, expected_out, expected_err, 0);
+  run_util_spawn_background(argv, expected_out, expected_err, 0, 0);
 }
 
 /** Check that failing to find the executable works as expected */
 static void
 test_util_spawn_background_fail(void *ptr)
 {
+#ifdef MS_WINDOWS
   const char *argv[] = {BUILDDIR "/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 = "";
+  const int expected_status = -1;
+#else
+  const char *argv[] = {BUILDDIR "/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 = "";
+  // TODO: Once we can signal failure to exec, set this to be -1;
+  const int expected_status = 0;
+#endif
 
   (void)ptr;
 
-  run_util_spawn_background(argv, expected_out, expected_err, 255);
+  run_util_spawn_background(argv, expected_out, expected_err, 255, expected_status);
 }
 
 static void