diff --git a/changes/bug5557 b/changes/bug5557
new file mode 100644
index 0000000000..c73fbe2839
--- /dev/null
+++ b/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.
diff --git a/src/common/util.c b/src/common/util.c
index 28ecff3983..cb8ff85b40 100644
--- a/src/common/util.c
+++ b/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 child_state and saved_errno as a hex string placed in
* hex_errno. 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 */
- if (cur < hex_errno)
- return;
+ /* Emit child_state */
+ written = format_hex_number_for_helper_exit_status(child_state,
+ cur, left);
+ if (written <= 0)
+ goto err;
- do {
- *cur-- = "0123456789ABCDEF"[unsigned_errno % 16];
- unsigned_errno /= 16;
- } while (unsigned_errno != 0 && cur >= hex_errno);
+ /* Adjust left and cur */
+ left -= written;
+ cur += written;
+ if (left <= 0)
+ goto err;
- /* Prepend the minus sign if errno was negative */
- if (saved_errno < 0 && cur >= hex_errno)
- *cur-- = '-';
+ /* Now the '/' */
+ *cur = '/';
- /* Leave a gap */
- if (cur >= hex_errno)
- *cur-- = '/';
+ /* Adjust left and cur */
+ ++cur;
+ --left;
+ if (left <= 0)
+ goto err;
- /* Check for overflow on first iteration of the loop */
- if (cur < hex_errno)
- return;
+ /* Need minus? */
+ if (saved_errno < 0) {
+ *cur = '-';
+ ++cur;
+ --left;
+ if (left <= 0)
+ goto err;
+ }
- /* Convert child_state to hex */
- do {
- *cur-- = "0123456789ABCDEF"[child_state % 16];
- child_state /= 16;
- } while (child_state != 0 && cur >= hex_errno);
+ /* Emit unsigned_errno */
+ written = format_hex_number_for_helper_exit_status(unsigned_errno,
+ cur, left);
+
+ 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() */
diff --git a/src/common/util.h b/src/common/util.h
index a2b196c88b..6b7c6fb623 100644
--- a/src/common/util.h
+++ b/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);
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 7484b9e90f..d71d280fa3 100644
--- a/src/test/test_util.c
+++ b/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),