Factor out and re-write code for splitting lines from a handle

Now handles non-printable characters and will not output a spurious
new-line if given a partial line.

3 files changed, 132 insertions, 23 deletions

diff --git a/src/common/util.c b/src/common/util.c
index 87c6fb5c1..aa344bc9c 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -3597,6 +3597,60 @@ tor_read_all_from_process_stderr(const process_handle_t process_handle,
 #endif
 }
 
+/* Split buf into lines, and add to smartlist. The buffer <b>buf</b> will be
+ * modified. The resulting smartlist will consist of pointers to buf, so there
+ * is no need to free the contents of sl. <b>buf</b> must be a NULL terminated
+ * string. <b>len</b> should be set to the length of the buffer excluding the
+ * NULL. Non-printable characters (including NULL) will be replaced with "." */
+
+int
+tor_split_lines(smartlist_t *sl, char *buf, int len)
+{
+  /* Index in buf of the start of the current line */
+  int start = 0;
+  /* Index in buf of the current character being processed */
+  int cur = 0;
+  /* Are we currently in a line */
+  char in_line = 0;
+
+  /* Loop over string */
+  while (cur < len) {
+    /* Loop until end of line or end of string */
+    for (; cur < len; cur++) {
+      if (in_line) {
+        if ('\r' == buf[cur] || '\n' == buf[cur]) {
+          /* End of line */
+          buf[cur] = '\0';
+          /* Point cur to the next line */
+          cur++;
+          /* Line starts at start and ends with a null */
+          break;
+        } else {
+          if (!TOR_ISPRINT(buf[cur]))
+            buf[cur] = '.';
+        }
+      } else {
+        if ('\r' == buf[cur] || '\n' == buf[cur]) {
+          /* Skip leading vertical space */
+          ;
+        } else {
+          in_line = 1;
+          start = cur;
+          if (!TOR_ISPRINT(buf[cur]))
+            buf[cur] = '.';
+        }
+      }
+    }
+    /* We are at the end of the line or end of string. If in_line is true there
+     * is a line which starts at buf+start and ends at a NULL. cur points to
+     * the character after the NULL. */
+    if (in_line)
+      smartlist_add(sl, (void *)(buf+start));
+    in_line = 0;
+  }
+  return smartlist_len(sl);
+}
+
 #ifdef MS_WINDOWS
 /** Read from stream, and send lines to log at the specified log level.
  * Returns -1 if there is a error reading, and 0 otherwise.
@@ -3606,7 +3660,7 @@ log_from_handle(HANDLE *pipe, int severity)
 {
   char buf[256];
   int pos;
-  int start, cur, next;
+  smartlist_t *lines;
 
   pos = tor_read_all_handle(pipe, buf, sizeof(buf) - 1, NULL);
   if (pos < 0) {
@@ -3626,28 +3680,17 @@ log_from_handle(HANDLE *pipe, int severity)
   buf[pos] = '\0';
   log_debug(LD_GENERAL, "Subprocess had %d bytes to say", pos);
 
-  /* Split buf into lines and log each one */
-  next = 0; // Start of the next line
-  while (next < pos) {
-    start = next; // Look for the end of this line
-    for (cur=start; cur<pos; cur++) {
-      /* On Windows \r means end of line */
-      if ('\r' == buf[cur]) {
-        buf[cur] = '\0';
-        next = cur + 1;
-        /* If \n follows, remove it too */
-        if ((cur + 1) < pos && '\n' == buf[cur+1]) {
-          buf[cur + 1] = '\0';
-          next = cur + 2;
-        }
-        /* Line starts at start and ends with a null (was \r\n) */
-        break;
-      }
-      /* Line starts at start and ends at the end of a string
-         but we already added a null in earlier */
-    }
-    log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", buf+start);
-  }
+  /* Split up the buffer */
+  lines = smartlist_create();
+  tor_split_lines(lines, buf, pos);
+
+  /* Log each line */
+  SMARTLIST_FOREACH(lines, char *, line,
+  {
+    log_fn(severity, LD_GENERAL, "Port forwarding helper says: %s", line);
+  });
+  smartlist_free(lines);
+
   return 0;
 }
 
diff --git a/src/common/util.h b/src/common/util.h
index 9cdd8cb39..853ac8db5 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -385,6 +385,7 @@ int tor_spawn_background(const char *const filename, const char **argv,
 #define PROCESS_EXIT_ERROR -1
 int tor_get_exit_code(const process_handle_t process_handle,
                       int block, int *exit_code);
+int tor_split_lines(struct smartlist_t *sl, char *buf, int len);
 #ifdef MS_WINDOWS
 ssize_t tor_read_all_handle(HANDLE h, char *buf, size_t count,
                             HANDLE hProcess);
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 9df7bc675..a3bc003b2 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1554,6 +1554,9 @@ test_util_spawn_background_partial_read(void *ptr)
   ;
 }
 
+/**
+ * Test that we can properly format q Windows command line
+ */
 static void
 test_util_join_cmdline(void *ptr)
 {
@@ -1602,6 +1605,67 @@ test_util_join_cmdline(void *ptr)
   ;
 }
 
+#define MAX_SPLIT_LINE_COUNT 3
+struct split_lines_test_t {
+  const char *orig_line; // Line to be split (may contain \0's)
+  int orig_length; // Length of orig_line
+  const char *split_line[MAX_SPLIT_LINE_COUNT]; // Split lines
+};
+
+/**
+ * Test that we properly split a buffer into lines
+ */
+static void
+test_util_split_lines(void *ptr)
+{
+  /* Test cases. orig_line of last test case must be NULL.
+   * The last element of split_line[i] must be NULL. */
+  struct split_lines_test_t tests[] = {
+    {"", 0, {NULL}},
+    {"foo", 3, {"foo", NULL}},
+    {"\n\rfoo\n\rbar\r\n", 12, {"foo", "bar", NULL}},
+    {"fo o\r\nb\tar", 10, {"fo o", "b.ar", NULL}},
+    {"\x0f""f\0o\0\n\x01""b\0r\0\r", 12, {".f.o.", ".b.r.", NULL}},
+    {NULL, 0, {}}
+  };
+
+  int i, j;
+  char *orig_line;
+  smartlist_t *sl;
+
+  (void)ptr;
+
+  for (i=0; tests[i].orig_line; i++) {
+    sl = smartlist_create();
+    orig_line = tor_malloc(tests[i].orig_length);
+    memcpy(orig_line, tests[i].orig_line, tests[i].orig_length + 1);
+    tor_split_lines(sl, orig_line, tests[i].orig_length);
+
+    j = 0;
+    log_info(LD_GENERAL, "Splitting test %d of length %d",
+             i, tests[i].orig_length);
+    SMARTLIST_FOREACH(sl, const char *, line,
+    {
+      /* Check we have not got too many lines */
+      tt_int_op(j, <, MAX_SPLIT_LINE_COUNT);
+      /* Check that there actually should be a line here */
+      tt_assert(tests[i].split_line[j] != NULL);
+      log_info(LD_GENERAL, "Line %d of test %d, should be <%s>",
+               j, i, tests[i].split_line[j]);
+      /* Check that the line is as expected */
+      tt_str_op(tests[i].split_line[j], ==, line);
+      j++;
+    });
+    /* Check that we didn't miss some lines */
+    tt_assert(tests[i].split_line[j] == NULL);
+    tor_free(orig_line);
+    smartlist_free(sl);
+  }
+
+ done:
+  ;
+}
+
 static void
 test_util_di_ops(void)
 {
@@ -1691,6 +1755,7 @@ struct testcase_t util_tests[] = {
   UTIL_TEST(spawn_background_fail, 0),
   UTIL_TEST(spawn_background_partial_read, 0),
   UTIL_TEST(join_cmdline, 0),
+  UTIL_TEST(split_lines, 0),
   END_OF_TESTCASES
 };