Fix issues in nickm's review of format_helper_exit_status for bug #1903

- Responsibility of clearing hex_errno is no longer with caller
- More conservative bounds checking
- Length requirement of hex_errno documented
- Output format documented

2 files changed, 35 insertions, 14 deletions

diff --git a/src/common/util.c b/src/common/util.c
index b5a3ade2bd..6c7794d1e6 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -2879,17 +2879,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;
+
+  /* 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';
 
-  /* If errno is negative, negate it */
+  /* Convert errno to be unsigned for hex conversion */
   if (saved_errno < 0) {
     unsigned_errno = (unsigned int) -saved_errno;
   } else {
@@ -2899,17 +2916,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 {
@@ -2970,10 +2996,6 @@ 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 */
diff --git a/src/test/test_util.c b/src/test/test_util.c
index 68a0ca2984..208c5c4f59 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -1212,14 +1212,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;