commit b1ad1a1d0266a20bb0dac15e65abe7b65a74e8a0 Author: Nick Mathewson nickm@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: ;