Merge remote-tracking branch 'origin/maint-0.2.3'

changes/bug5557

@@ -0,0 +1,3 @@
+  o Minor bugfixes
+    - Make format_helper_exit_status() avoid unnecessary space padding and
+      stop confusing log_from_pipe().  Fixes ticket 5557.

src/common/util.c

@@ -3191,6 +3191,68 @@ tor_join_win_cmdline(const char *argv[])
   return joined_argv;
 }
 
+/**
+ * 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.
+ *
+ * This function DOES NOT add a terminating NUL character to its output: be
+ * careful!
+ *
+ * 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
+ * does not guarantee that an int is wider than a char (an int must be at
+ * least 16 bits but it is permitted for a char to be that wide as well), we
+ * can't assume a signed int is sufficient to accomodate an unsigned char.
+ * Thus, format_helper_exit_status() will still need to emit any require '-'
+ * on its own.
+ *
+ * For most purposes, you'd want to use tor_snprintf("%x") instead of this
+ * function; it's designed to be used in code paths where you can't call
+ * arbitrary C functions.
+ */
+int
+format_hex_number_for_helper_exit_status(unsigned int x, char *buf,
+                                         int max_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 len */
+  return len;
+}
+
 /** 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.
@@ -3203,12 +3265,12 @@ tor_join_win_cmdline(const char *argv[])
  * 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)
 {
   unsigned int unsigned_errno;
+  int written, left;
   char *cur;
   size_t i;
 
@@ -3225,35 +3287,73 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
     unsigned_errno = (unsigned int) saved_errno;
   }
 
-  /* Convert errno to hex (start before \n) */
+  /*
-  cur = hex_errno + HEX_ERRNO_SIZE - 2;
+   * Count how many chars of space we have left, and keep a pointer into the
+   * current point in the buffer.
+   */
+  left = HEX_ERRNO_SIZE;
+  cur = hex_errno;
 
-  /* Check for overflow on first iteration of the loop */
+  /* Emit child_state */
-  if (cur < hex_errno)
+  written = format_hex_number_for_helper_exit_status(child_state,
-    return;
+                                                     cur, left);
+  if (written <= 0)
+    goto err;
 
-  do {
+  /* Adjust left and cur */
-    *cur-- = "0123456789ABCDEF"[unsigned_errno % 16];
+  left -= written;
-    unsigned_errno /= 16;
+  cur += written;
-  } while (unsigned_errno != 0 && cur >= hex_errno);
+  if (left <= 0)
+    goto err;
 
-  /* Prepend the minus sign if errno was negative */
+  /* Now the '/' */
-  if (saved_errno < 0 && cur >= hex_errno)
+  *cur = '/';
-    *cur-- = '-';
 
-  /* Leave a gap */
+  /* Adjust left and cur */
-  if (cur >= hex_errno)
+  ++cur;
-    *cur-- = '/';
+  --left;
+  if (left <= 0)
+    goto err;
 
-  /* Check for overflow on first iteration of the loop */
+  /* Need minus? */
-  if (cur < hex_errno)
+  if (saved_errno < 0) {
-    return;
+    *cur = '-';
+    ++cur;
+    --left;
+    if (left <= 0)
+      goto err;
+  }
 
-  /* Convert child_state to hex */
+  /* Emit unsigned_errno */
-  do {
+  written = format_hex_number_for_helper_exit_status(unsigned_errno,
-    *cur-- = "0123456789ABCDEF"[child_state % 16];
+                                                     cur, left);
-    child_state /= 16;
+
-  } while (child_state != 0 && cur >= hex_errno);
+  if (written <= 0)
+    goto err;
+
+  /* Adjust left and cur */
+  left -= written;
+  cur += written;
+
+  /* Check that we have enough space left for a newline */
+  if (left <= 0)
+    goto err;
+
+  /* Emit the newline and NUL */
+  *cur++ = '\n';
+  *cur++ = '\0';
+
+  goto done;
+
+ err:
+  /*
+   * In error exit, just write a '\0' in the first char so whatever called
+   * this at least won't fall off the end.
+   */
+  *hex_errno = '\0';
+
+ done:
+  return;
 }
 
 /* Maximum number of file descriptors, if we cannot get it via sysconf() */

src/common/util.h

@@ -471,6 +471,8 @@ void tor_process_handle_destroy(process_handle_t *process_handle,
 #ifdef UTIL_PRIVATE
 /* Prototypes for private functions only used by util.c (and unit tests) */
 
+int format_hex_number_for_helper_exit_status(unsigned int x, char *buf,
+                                             int max_len);
 void format_helper_exit_status(unsigned char child_state,
                                int saved_errno, char *hex_errno);
 

src/test/test_util.c

@@ -2143,11 +2143,11 @@ test_util_exit_status(void *ptr)
 
   clear_hex_errno(hex_errno);
   format_helper_exit_status(0, 0, hex_errno);
-  test_streq("         0/0\n", hex_errno);
+  test_streq("0/0\n", hex_errno);
 
   clear_hex_errno(hex_errno);
   format_helper_exit_status(0, 0x7FFFFFFF, hex_errno);
-  test_streq("  0/7FFFFFFF\n", hex_errno);
+  test_streq("0/7FFFFFFF\n", hex_errno);
 
   clear_hex_errno(hex_errno);
   format_helper_exit_status(0xFF, -0x80000000, hex_errno);
@@ -2155,11 +2155,11 @@ test_util_exit_status(void *ptr)
 
   clear_hex_errno(hex_errno);
   format_helper_exit_status(0x7F, 0, hex_errno);
-  test_streq("        7F/0\n", hex_errno);
+  test_streq("7F/0\n", hex_errno);
 
   clear_hex_errno(hex_errno);
   format_helper_exit_status(0x08, -0x242, hex_errno);
-  test_streq("      8/-242\n", hex_errno);
+  test_streq("8/-242\n", hex_errno);
 
  done:
   ;
@@ -2357,7 +2357,7 @@ test_util_spawn_background_fail(void *ptr)
   tor_snprintf(code, sizeof(code), "%x/%x",
     9 /* CHILD_STATE_FAILEXEC */ , ENOENT);
   tor_snprintf(expected_out, sizeof(expected_out),
-    "ERR: Failed to spawn background process - code %12s\n", code);
+    "ERR: Failed to spawn background process - code %s\n", code);
 
   run_util_spawn_background(argv, expected_out, expected_err, 255,
                             expected_status);
@@ -2464,6 +2464,44 @@ test_util_spawn_background_partial_read(void *ptr)
   tor_process_handle_destroy(process_handle, 1);
 }
 
+/**
+ * Test for format_hex_number_for_helper_exit_status()
+ */
+
+static void
+test_util_format_hex_number(void *ptr)
+{
+  int i, len;
+  char buf[HEX_ERRNO_SIZE + 1];
+  const struct {
+    const char *str;
+    unsigned int x;
+  } test_data[] = {
+    {"0", 0},
+    {"1", 1},
+    {"273A", 0x273a},
+    {"FFFF", 0xffff},
+#if UINT_MAX >= 0xffffffff
+    {"31BC421D", 0x31bc421d},
+    {"FFFFFFFF", 0xffffffff},
+#endif
+    {NULL, 0}
+  };
+
+  (void)ptr;
+
+  for (i = 0; test_data[i].str != NULL; ++i) {
+    len = format_hex_number_for_helper_exit_status(test_data[i].x,
+        buf, HEX_ERRNO_SIZE);
+    test_neq(len, 0);
+    buf[len] = '\0';
+    test_streq(buf, test_data[i].str);
+  }
+
+ done:
+  return;
+}
+
 /**
  * Test that we can properly format q Windows command line
  */
@@ -3031,6 +3069,7 @@ struct testcase_t util_tests[] = {
   UTIL_TEST(spawn_background_ok, 0),
   UTIL_TEST(spawn_background_fail, 0),
   UTIL_TEST(spawn_background_partial_read, 0),
+  UTIL_TEST(format_hex_number, 0),
   UTIL_TEST(join_win_cmdline, 0),
   UTIL_TEST(split_lines, 0),
   UTIL_TEST(n_bits_set, 0),