[tor-commits] [tor/maint-0.2.3] Resolve crash caused by format_helper_exit_status changes in #5557

[nickm at torproject.org]

commit b1ad1a1d0266a20bb0dac15e65abe7b65a74e8a0
Author: Nick Mathewson <nickm at torproject.org>
Date:   Sat Jun 23 15:30:01 2012 -0400

    Resolve crash caused by format_helper_exit_status changes in #5557
    
    Because the string output was no longer equal in length to
    HEX_ERRNO_SIZE, the write() call would add some extra spaces and
    maybe a NUL, and the NUL would trigger an assert in
    get_string_from_pipe.
    
    Fixes bug 6225; bug not in any released version of Tor.
---
 src/common/util.c    |   29 ++++++++++++++++++++---------
 src/common/util.h    |    4 ++--
 src/test/test_util.c |   16 +++++++++++-----
 3 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/src/common/util.c b/src/common/util.c
index cb8ff85..8cb013e 100644
--- a/src/common/util.c
+++ b/src/common/util.c
@@ -3264,8 +3264,11 @@ format_hex_number_for_helper_exit_status(unsigned int x, char *buf,
  * 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.
+ *
+ * On success return the number of characters added to hex_errno, not counting
+ * the terminating NUL; return -1 on error.
  */
-void
+int
 format_helper_exit_status(unsigned char child_state, int saved_errno,
                           char *hex_errno)
 {
@@ -3273,6 +3276,7 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
   int written, left;
   char *cur;
   size_t i;
+  int res = -1;
 
   /* Fill hex_errno with spaces, and a trailing newline (memset may
      not be signal handler safe, so we can't use it) */
@@ -3343,6 +3347,8 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
   *cur++ = '\n';
   *cur++ = '\0';
 
+  res = cur - hex_errno - 1;
+
   goto done;
 
  err:
@@ -3353,7 +3359,7 @@ format_helper_exit_status(unsigned char child_state, int saved_errno,
   *hex_errno = '\0';
 
  done:
-  return;
+  return res;
 }
 
 /* Maximum number of file descriptors, if we cannot get it via sysconf() */
@@ -3695,15 +3701,20 @@ tor_spawn_background(const char *const filename, const char **argv,
     child_state = CHILD_STATE_FAILEXEC;
 
   error:
-    /* XXX: are we leaking fds from the pipe? */
+    {
+      /* XXX: are we leaking fds from the pipe? */
+      int n;
 
-    format_helper_exit_status(child_state, errno, hex_errno);
+      n = format_helper_exit_status(child_state, errno, hex_errno);
 
-    /* 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));
+      if (n >= 0) {
+        /* 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, n);
+      }
+    }
 
     (void) nbytes;
 
diff --git a/src/common/util.h b/src/common/util.h
index 6b7c6fb..a2ab0cc 100644
--- a/src/common/util.h
+++ b/src/common/util.h
@@ -473,8 +473,8 @@ void tor_process_handle_destroy(process_handle_t *process_handle,
 
 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);
+int format_helper_exit_status(unsigned char child_state,
+                              int saved_errno, char *hex_errno);
 
 /* Space for hex values of child state, a slash, saved_errno (with
    leading minus) and newline (no null) */
diff --git a/src/test/test_util.c b/src/test/test_util.c
index d71d280..fab9595 100644
--- a/src/test/test_util.c
+++ b/src/test/test_util.c
@@ -2138,28 +2138,34 @@ 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];
+  int n;
 
   (void)ptr;
 
   clear_hex_errno(hex_errno);
-  format_helper_exit_status(0, 0, hex_errno);
+  n = format_helper_exit_status(0, 0, hex_errno);
   test_streq("0/0\n", hex_errno);
+  test_eq(n, strlen(hex_errno));
 
   clear_hex_errno(hex_errno);
-  format_helper_exit_status(0, 0x7FFFFFFF, hex_errno);
+  n = format_helper_exit_status(0, 0x7FFFFFFF, hex_errno);
   test_streq("0/7FFFFFFF\n", hex_errno);
+  test_eq(n, strlen(hex_errno));
 
   clear_hex_errno(hex_errno);
-  format_helper_exit_status(0xFF, -0x80000000, hex_errno);
+  n = format_helper_exit_status(0xFF, -0x80000000, hex_errno);
   test_streq("FF/-80000000\n", hex_errno);
+  test_eq(n, strlen(hex_errno));
 
   clear_hex_errno(hex_errno);
-  format_helper_exit_status(0x7F, 0, hex_errno);
+  n = format_helper_exit_status(0x7F, 0, hex_errno);
   test_streq("7F/0\n", hex_errno);
+  test_eq(n, strlen(hex_errno));
 
   clear_hex_errno(hex_errno);
-  format_helper_exit_status(0x08, -0x242, hex_errno);
+  n = format_helper_exit_status(0x08, -0x242, hex_errno);
   test_streq("8/-242\n", hex_errno);
+  test_eq(n, strlen(hex_errno));
 
  done:
   ;