- tor-commits - lists.torproject.org

[tor/master] Resolve crash caused by format_helper_exit_status changes in #5557
by nickm＠torproject.org 25 Jun '12

25 Jun '12

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

1 0

[tor/master] Don't assert in get_string_from_pipe() on len==0
by nickm＠torproject.org 25 Jun '12

25 Jun '12

commit ffd7189b3fc015ce47e6ab27ac85d4eab2183a2b Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Jun 23 15:35:43 2012 -0400 Don't assert in get_string_from_pipe() on len==0 We can treat this case as an EAGAIN (probably because of an unexpected internal NUL) rather than a crash-worthy problem. Fixes bug 6225, again. Bug not in any released version of Tor. --- src/common/util.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 8cb013e..e5b51b9 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -4386,7 +4386,10 @@ get_string_from_pipe(FILE *stream, char *buf_out, size_t count) } } else { len = strlen(buf_out); - tor_assert(len>0); + if (len == 0) { + /* this probably means we got a NUL at the start of the string. */ + return IO_STREAM_EAGAIN; + } if (buf_out[len - 1] == '\n') { /* Remove the trailing newline */

1 0

[tor/master] Merge remote-tracking branch 'public/bug6225' into maint-0.2.3
by nickm＠torproject.org 25 Jun '12

25 Jun '12

commit 2703e072a1056edd3b68a16e045175e5bd268301 Merge: a6688c5 ffd7189 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Jun 25 11:51:19 2012 -0400 Merge remote-tracking branch 'public/bug6225' into maint-0.2.3 src/common/util.c | 34 ++++++++++++++++++++++++---------- src/common/util.h | 4 ++-- src/test/test_util.c | 16 +++++++++++----- 3 files changed, 37 insertions(+), 17 deletions(-)

1 0

[tor/master] Merge remote-tracking branch 'origin/maint-0.2.3'
by nickm＠torproject.org 25 Jun '12

25 Jun '12

commit aad71eef1b8cf5e6dd74371abc012b342ec3f011 Merge: 8e5d3ca 2703e07 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Jun 25 11:59:33 2012 -0400 Merge remote-tracking branch 'origin/maint-0.2.3' src/common/util.c | 34 ++++++++++++++++++++++++---------- src/common/util.h | 4 ++-- src/test/test_util.c | 16 +++++++++++----- 3 files changed, 37 insertions(+), 17 deletions(-)

1 0

[tor/maint-0.2.3] Don't assert in get_string_from_pipe() on len==0
by nickm＠torproject.org 25 Jun '12

25 Jun '12

commit ffd7189b3fc015ce47e6ab27ac85d4eab2183a2b Author: Nick Mathewson <nickm(a)torproject.org> Date: Sat Jun 23 15:35:43 2012 -0400 Don't assert in get_string_from_pipe() on len==0 We can treat this case as an EAGAIN (probably because of an unexpected internal NUL) rather than a crash-worthy problem. Fixes bug 6225, again. Bug not in any released version of Tor. --- src/common/util.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 8cb013e..e5b51b9 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -4386,7 +4386,10 @@ get_string_from_pipe(FILE *stream, char *buf_out, size_t count) } } else { len = strlen(buf_out); - tor_assert(len>0); + if (len == 0) { + /* this probably means we got a NUL at the start of the string. */ + return IO_STREAM_EAGAIN; + } if (buf_out[len - 1] == '\n') { /* Remove the trailing newline */

1 0

[tor/maint-0.2.3] Resolve crash caused by format_helper_exit_status changes in #5557
by nickm＠torproject.org 25 Jun '12

25 Jun '12

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

1 0

[tor/maint-0.2.3] Merge remote-tracking branch 'public/bug6225' into maint-0.2.3
by nickm＠torproject.org 25 Jun '12

25 Jun '12

commit 2703e072a1056edd3b68a16e045175e5bd268301 Merge: a6688c5 ffd7189 Author: Nick Mathewson <nickm(a)torproject.org> Date: Mon Jun 25 11:51:19 2012 -0400 Merge remote-tracking branch 'public/bug6225' into maint-0.2.3 src/common/util.c | 34 ++++++++++++++++++++++++---------- src/common/util.h | 4 ++-- src/test/test_util.c | 16 +++++++++++----- 3 files changed, 37 insertions(+), 17 deletions(-)

1 0

[stem/master] Subprocess lacks a kill() method on python 2.5
by atagar＠torproject.org 25 Jun '12

25 Jun '12

commit 9d54db62a7b918c3bc6a852275adaf70dfdfef44 Author: Damian Johnson <atagar(a)torproject.org> Date: Fri Jun 22 09:14:20 2012 -0700 Subprocess lacks a kill() method on python 2.5 The Popen.kill() ad Popen.terminate() methods were first added in python 2.6... http://docs.python.org/library/subprocess.html#subprocess.Popen.kill We can work around this with os.kill() but that only works on *nix... http://stackoverflow.com/questions/552423/use-python-2-6-subprocess-module-… --- stem/process.py | 18 ++++++++++++++++-- test/integ/process.py | 18 +++++++++++++++++- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/stem/process.py b/stem/process.py index 36ef7b9..e9d453e 100644 --- a/stem/process.py +++ b/stem/process.py @@ -23,6 +23,7 @@ import signal import tempfile import subprocess +import stem.prereq import stem.util.system NO_TORRC = "<no torrc>" @@ -80,7 +81,15 @@ def launch_tor(tor_cmd = "tor", args = None, torrc_path = None, completion_perce try: os.remove(temp_file) except: pass - tor_process.kill() + # We can't kill the subprocess on python 2.5 running Windows without the + # win32process module... + # http://stackoverflow.com/questions/552423/use-python-2-6-subprocess-module-… + + if stem.prereq.is_python_26(): + tor_process.kill() + elif stem.util.system.is_windows(): + os.kill(tor_process.pid, signal.SIGTERM) + raise OSError("reached a %i second timeout without success" % timeout) signal.signal(signal.SIGALRM, timeout_handler) @@ -95,7 +104,12 @@ def launch_tor(tor_cmd = "tor", args = None, torrc_path = None, completion_perce # this will provide empty results if the process is terminated if not init_line: - tor_process.kill() # ... but best make sure + # ... but best make sure + if stem.prereq.is_python_26(): + tor_process.kill() + elif stem.util.system.is_windows(): + os.kill(tor_process.pid, signal.SIGTERM) + raise OSError("Process terminated: %s" % last_problem) # provide the caller with the initialization message if they want it diff --git a/test/integ/process.py b/test/integ/process.py index e3dd277..2037d93 100644 --- a/test/integ/process.py +++ b/test/integ/process.py @@ -2,12 +2,16 @@ Tests the stem.process functions with various use cases. """ +import os import time +import signal import unittest +import stem.prereq import stem.socket import stem.process import test.runner +import stem.util.system class TestProcess(unittest.TestCase): def test_launch_tor_with_config(self): @@ -15,6 +19,10 @@ class TestProcess(unittest.TestCase): Exercises launch_tor_with_config. """ + if not stem.prereq.is_python_26() and stem.util.system.is_windows(): + test.runner.skip("(unable to kill subprocesses)") + return + if test.runner.only_run_once(self, "test_launch_tor_with_config"): return # Launch tor without a torrc, but with a control port. Confirms that this @@ -37,13 +45,21 @@ class TestProcess(unittest.TestCase): self.assertEquals("ControlPort=2778", str(getconf_response)) finally: if control_socket: control_socket.close() - tor_process.kill() + + if stem.prereq.is_python_26(): + tor_process.kill() + elif stem.util.system.is_windows(): + os.kill(tor_process.pid, signal.SIGTERM) def test_launch_tor_with_timeout(self): """ Runs launch_tor where it times out before completing. """ + if not stem.prereq.is_python_26() and stem.util.system.is_windows(): + test.runner.skip("(unable to kill subprocesses)") + return + if test.runner.only_run_once(self, "test_launch_tor_with_timeout"): return start_time = time.time()

1 0

[stem/master] Avoiding the 'with' keyword for tarfile
by atagar＠torproject.org 25 Jun '12

25 Jun '12

commit e51fea598dced5e3c4a05c4cd0ad509beed530fe Author: Damian Johnson <atagar(a)torproject.org> Date: Thu Jun 21 09:45:14 2012 -0700 Avoiding the 'with' keyword for tarfile The tarfile's __exit__() method was added sometime after python 2.5, causing it to produce a stacktrace if used via the 'with' keyword. Pity, yet another hack to accomidate a six-year-old version of python... --- stem/descriptor/reader.py | 31 ++++++++++++++++++++----------- 1 files changed, 20 insertions(+), 11 deletions(-) diff --git a/stem/descriptor/reader.py b/stem/descriptor/reader.py index 7a7e565..716b58c 100644 --- a/stem/descriptor/reader.py +++ b/stem/descriptor/reader.py @@ -443,22 +443,31 @@ class DescriptorReader: self._notify_skip_listeners(target, ReadFailed(exc)) def _handle_archive(self, target): + # TODO: This would be nicer via the 'with' keyword, but tarfile's __exit__ + # method was added sometime after python 2.5. We should change this when + # we drop python 2.5 support. + + tar_file = None + try: - with tarfile.open(target) as tar_file: - for tar_entry in tar_file: - if tar_entry.isfile(): - entry = tar_file.extractfile(tar_entry) - - for desc in stem.descriptor.parse_file(target, entry): - if self._is_stopped.isSet(): return - self._unreturned_descriptors.put(desc) - self._iter_notice.set() - - entry.close() + tar_file = tarfile.open(target) + + for tar_entry in tar_file: + if tar_entry.isfile(): + entry = tar_file.extractfile(tar_entry) + + for desc in stem.descriptor.parse_file(target, entry): + if self._is_stopped.isSet(): return + self._unreturned_descriptors.put(desc) + self._iter_notice.set() + + entry.close() except TypeError, exc: self._notify_skip_listeners(target, ParsingFailure(exc)) except IOError, exc: self._notify_skip_listeners(target, ReadFailed(exc)) + finally: + if tar_file: tar_file.close() def _notify_skip_listeners(self, path, exception): for listener in self._skip_listeners:

1 0

[stem/master] Missed another tarfile 'with' declaration
by atagar＠torproject.org 25 Jun '12

25 Jun '12

commit ac34b8151c5b46369578922ba4f90fdce15128b3 Author: Damian Johnson <atagar(a)torproject.org> Date: Fri Jun 22 09:18:39 2012 -0700 Missed another tarfile 'with' declaration Missed that we were using the 'with' keyword for a tarfile in the integ tests. Using a try/catch instead so this'll work on python 2.5. --- test/integ/descriptor/reader.py | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/test/integ/descriptor/reader.py b/test/integ/descriptor/reader.py index 27d096f..25952de 100644 --- a/test/integ/descriptor/reader.py +++ b/test/integ/descriptor/reader.py @@ -50,13 +50,20 @@ def _get_raw_tar_descriptors(): test_path = os.path.join(DESCRIPTOR_TEST_DATA, "descriptor_archive.tar") raw_descriptors = [] - with tarfile.open(test_path) as tar_file: + # TODO: revert to using the 'with' keyword for this when dropping python 2.5 support + tar_file = None + + try: + tar_file = tarfile.open(test_path) + for tar_entry in tar_file: if tar_entry.isfile(): entry = tar_file.extractfile(tar_entry) entry.readline() # strip header raw_descriptors.append(entry.read()) entry.close() + finally: + if tar_file: tar_file.close() TAR_DESCRIPTORS = raw_descriptors

1 0