October 2017 - tor-commits - lists.torproject.org

[nyx/master] Use stem's address caching
by atagar＠torproject.org 27 Oct '17

27 Oct '17

commit 5766118620a45d9df9731e1c00f7538a001dd3d4 Author: Damian Johnson <atagar(a)torproject.org> Date: Thu Oct 26 10:55:19 2017 -0700 Use stem's address caching Now that stem can cache our address we can drop our our_address() helper. --- nyx/__init__.py | 25 +++---------------------- nyx/panel/connection.py | 6 +++--- nyx/panel/header.py | 2 +- nyx/tracker.py | 4 ++-- test/panel/connection.py | 8 ++++---- test/panel/header.py | 2 +- 6 files changed, 14 insertions(+), 33 deletions(-) diff --git a/nyx/__init__.py b/nyx/__init__.py index 5947294..7c61651 100644 --- a/nyx/__init__.py +++ b/nyx/__init__.py @@ -9,7 +9,6 @@ Tor curses monitoring application. nyx_interface - nyx interface singleton tor_controller - tor connection singleton cache - provides our application cache - our_address - provides our ip address show_message - shows a message to the user input_prompt - prompts the user for text input @@ -105,9 +104,9 @@ CACHE = None CHROOT = None BASE_DIR = os.path.sep.join(__file__.split(os.path.sep)[:-1]) -CACHED_ADDRESS = None -ADDRESS_FETCHED = None -ADDRESS_FETCH_RATE = 60 +# our address infrequently changes so we can cache it for a while + +stem.control.CACHE_ADDRESS_FOR = 30 # disable trace level messages about cache hits @@ -251,24 +250,6 @@ def cache(): return CACHE -def our_address(default = None): - """ - Provides our ip address. - - :param str default: response if address is unavailable - - :returns: **str** with our address - """ - - global CACHED_ADDRESS, ADDRESS_FETCHED - - if ADDRESS_FETCHED is None or (time.time() - ADDRESS_FETCHED) > ADDRESS_FETCH_RATE: - CACHED_ADDRESS = tor_controller().get_info('address', None) - ADDRESS_FETCHED = time.time() - - return CACHED_ADDRESS if CACHED_ADDRESS is not None else default - - def show_message(message = None, *attr, **kwargs): """ Shows a message in our header. diff --git a/nyx/panel/connection.py b/nyx/panel/connection.py index 1a090ac..fc586b6 100644 --- a/nyx/panel/connection.py +++ b/nyx/panel/connection.py @@ -608,10 +608,10 @@ def _draw_line(subwindow, x, y, line, is_selected, width, current_time): def _draw_address_column(subwindow, x, y, line, attr): + src = tor_controller().get_info('address', line.connection.local_address) + if line.line_type == LineType.CONNECTION: - src = '%s:%s' % (nyx.our_address(line.connection.local_address), line.connection.local_port) - else: - src = nyx.our_address(line.connection.local_address) + src = '%s:%s' % (src, line.connection.local_port) if line.line_type == LineType.CIRCUIT_HEADER and line.circuit.status != 'BUILT': dst = 'Building...' diff --git a/nyx/panel/header.py b/nyx/panel/header.py index 0820a21..b1cbfbf 100644 --- a/nyx/panel/header.py +++ b/nyx/panel/header.py @@ -268,7 +268,7 @@ class Sampling(object): 'version': str(controller.get_version('Unknown')).split()[0], 'version_status': controller.get_info('status/version/current', 'Unknown'), - 'address': or_listeners[0][0] if (or_listeners and or_listeners[0][0] != '0.0.0.0') else nyx.our_address('Unknown'), + 'address': or_listeners[0][0] if (or_listeners and or_listeners[0][0] != '0.0.0.0') else controller.get_info('address', 'Unknown'), 'or_port': or_listeners[0][1] if or_listeners else '', 'dir_port': controller.get_conf('DirPort', '0'), 'control_port': str(control_listeners[0][1]) if control_listeners else None, diff --git a/nyx/tracker.py b/nyx/tracker.py index fb41f9e..fa2381e 100644 --- a/nyx/tracker.py +++ b/nyx/tracker.py @@ -903,7 +903,7 @@ class ConsensusTracker(object): controller = tor_controller() - if nyx.our_address() == address: + if controller.get_info('address', None) == address: fingerprint = controller.get_info('fingerprint', None) ports = controller.get_ports(stem.control.Listener.OR, None) @@ -924,7 +924,7 @@ class ConsensusTracker(object): controller = tor_controller() if fingerprint == controller.get_info('fingerprint', None): - my_address = nyx.our_address() + my_address = controller.get_info('address', None) my_or_ports = controller.get_ports(stem.control.Listener.OR, []) if my_address and len(my_or_ports) == 1: diff --git a/test/panel/connection.py b/test/panel/connection.py index c95f31a..f075f77 100644 --- a/test/panel/connection.py +++ b/test/panel/connection.py @@ -195,9 +195,9 @@ class TestConnectionPanel(unittest.TestCase): @require_curses @patch('nyx.panel.connection.tor_controller') - @patch('nyx.our_address', Mock(return_value = '82.121.9.9')) def test_draw_line(self, tor_controller_mock): tor_controller_mock().is_geoip_unavailable.return_value = False + tor_controller_mock().get_info.return_value = '82.121.9.9' test_data = (( line(), @@ -213,7 +213,7 @@ class TestConnectionPanel(unittest.TestCase): ' | 82.121.9.9 2 / Middle', ), ( line(line_type = LineType.CIRCUIT, fingerprint = 'E0BD57A11F00041A9789577C53A1B784473669E4'), - ' +- 82.121.9.9 3 / Exit', + ' +- 82.121.9.9 3 / End', )) for test_line, expected in test_data: @@ -222,9 +222,9 @@ class TestConnectionPanel(unittest.TestCase): @require_curses @patch('nyx.panel.connection.tor_controller') - @patch('nyx.our_address', Mock(return_value = '82.121.9.9')) def test_draw_address_column(self, tor_controller_mock): tor_controller_mock().is_geoip_unavailable.return_value = False + tor_controller_mock().get_info.return_value = '82.121.9.9' test_data = (( line(), @@ -282,7 +282,7 @@ class TestConnectionPanel(unittest.TestCase): test_data = { '1F43EE37A0670301AD9CB555D94AFEC2C89FDE86': ' 1 / Guard', 'B6D83EC2D9E18B0A7A33428F8CFA9C536769E209': ' 2 / Middle', - 'E0BD57A11F00041A9789577C53A1B784473669E4': ' 3 / Exit', + 'E0BD57A11F00041A9789577C53A1B784473669E4': ' 3 / End', } for fp, expected in test_data.items(): diff --git a/test/panel/header.py b/test/panel/header.py index 73259da..1b07799 100644 --- a/test/panel/header.py +++ b/test/panel/header.py @@ -87,7 +87,6 @@ class TestHeaderPanel(unittest.TestCase): @patch('time.time', Mock(return_value = 1234.5)) @patch('os.times', Mock(return_value = (0.08, 0.03, 0.0, 0.0, 18759021.31))) @patch('os.uname', Mock(return_value = ('Linux', 'odin', '3.5.0-54-generic', '#81~precise1-Ubuntu SMP Tue Jul 15 04:05:58 UTC 2014', 'i686'))) - @patch('nyx.our_address', Mock(return_value = '174.21.17.28')) @patch('stem.util.system.start_time', Mock(return_value = 5678)) @patch('stem.util.proc.file_descriptors_used', Mock(return_value = 89)) def test_sample(self, consensus_tracker_mock, resource_tracker_mock, tor_controller_mock): @@ -100,6 +99,7 @@ class TestHeaderPanel(unittest.TestCase): tor_controller_mock().get_pid.return_value = '123' tor_controller_mock().get_info.side_effect = lambda param, default = None: { + 'address': '174.21.17.28', 'fingerprint': '1A94D1A794FCB2F8B6CBC179EF8FDD4008A98D3B', 'status/version/current': 'recommended', 'process/descriptor-limit': 678,

1 0

[stem/master] Temporarily cache 'GETINFO address' responses
by atagar＠torproject.org 27 Oct '17

27 Oct '17

commit d0922386bf73ab81e3db9f993adf9842c18f4d2a Author: Damian Johnson <atagar(a)torproject.org> Date: Wed Oct 25 13:33:34 2017 -0700 Temporarily cache 'GETINFO address' responses This is a mutable parameter, but it's both reasonably static and highly requested. Caching it for a second by default. --- stem/control.py | 23 +++++++++++++++++++++++ test/unit/control/controller.py | 29 +++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/stem/control.py b/stem/control.py index 6746441c..eebc24b6 100644 --- a/stem/control.py +++ b/stem/control.py @@ -372,6 +372,7 @@ MAPPED_CONFIG_KEYS = { # unchangeable GETINFO parameters CACHEABLE_GETINFO_PARAMS = ( + 'address', 'version', 'config-file', 'exit-policy/default', @@ -389,6 +390,12 @@ CACHEABLE_GETINFO_PARAMS_UNTIL_SETCONF = ( 'accounting/enabled', ) +# 'GETINFO address' isn't technically cachable, but it's also both highly +# requested and highly static. As such fetching it at a fixed rate. If +# you'd like you can set this global to zero to disable caching. + +CACHE_ADDRESS_FOR = 1.0 + # GETCONF parameters we shouldn't cache. This includes hidden service # perameters due to the funky way they're set and retrieved (for instance, # 'SETCONF HiddenServiceDir' effects 'GETCONF HiddenServiceOptions'). @@ -1048,6 +1055,9 @@ class Controller(BaseController): self._event_listeners_lock = threading.RLock() self._enabled_features = [] self._is_geoip_unavailable = None + + self._address_cached_at = 0 + self._last_address_exc = None self._last_fingerprint_exc = None super(Controller, self).__init__(control_socket, is_authenticated) @@ -1145,6 +1155,8 @@ class Controller(BaseController): for param in params: if param.startswith('ip-to-country/') and param != 'ip-to-country/0.0.0.0' and self.is_geoip_unavailable(): raise stem.ProtocolError('Tor geoip database is unavailable') + elif param == 'address' and self._last_address_exc and (time.time() - self._address_cached_at) < CACHE_ADDRESS_FOR: + raise self._last_address_exc # we already know we can't resolve an address elif param == 'fingerprint' and self._last_fingerprint_exc and self.get_conf('ORPort', None) is None: raise self._last_fingerprint_exc # we already know we're not a relay @@ -1153,6 +1165,9 @@ class Controller(BaseController): cached_results = self._get_cache_map(map(str.lower, params), 'getinfo') for key in cached_results: + if key == 'address' and (time.time() - self._address_cached_at) > CACHE_ADDRESS_FOR: + continue # cached address is too old + user_expected_key = _case_insensitive_lookup(params, key) reply[user_expected_key] = cached_results[key] params.remove(user_expected_key) @@ -1192,6 +1207,10 @@ class Controller(BaseController): self._set_cache(to_cache, 'getinfo') + if 'address' in params: + self._address_cached_at = time.time() + self._last_address_exc = None + if 'fingerprint' in params: self._last_fingerprint_exc = None @@ -1202,6 +1221,10 @@ class Controller(BaseController): else: return list(reply.values())[0] except stem.ControllerError as exc: + if 'address' in params: + self._address_cached_at = time.time() + self._last_address_exc = exc + if 'fingerprint' in params: self._last_fingerprint_exc = exc diff --git a/test/unit/control/controller.py b/test/unit/control/controller.py index c7e412c5..a9a7e73d 100644 --- a/test/unit/control/controller.py +++ b/test/unit/control/controller.py @@ -5,6 +5,7 @@ integ tests, but a few bits lend themselves to unit testing. import datetime import io +import time import unittest import stem.descriptor.router_status_entry @@ -46,13 +47,37 @@ class TestControl(unittest.TestCase): self.assertTrue(stem.control.event_description(event) is not None) @patch('stem.control.Controller.msg') - def test_get_get_info(self, msg_mock): + def test_get_info(self, msg_mock): msg_mock.return_value = ControlMessage.from_str('250-hello=hi right back!\r\n250 OK\r\n', 'GETINFO') self.assertEqual('hi right back!', self.controller.get_info('hello')) @patch('stem.control.Controller.msg') + def test_get_info_address_caching(self, msg_mock): + test_start = time.time() + msg_mock.return_value = ControlMessage.from_str('551 Address unknown\r\n') + + self.assertEqual(0, self.controller._address_cached_at) + self.assertEqual(None, self.controller._last_address_exc) + self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') + self.assertTrue(test_start <= self.controller._address_cached_at <= time.time()) + self.assertEqual("GETINFO response didn't have an OK status:\nAddress unknown", str(self.controller._last_address_exc)) + self.assertEqual(1, msg_mock.call_count) + + # now that we have a cached failure we should provide that back + + self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') + self.assertEqual(1, msg_mock.call_count) + + # pretend it's a minute later and try again, now with an address + + msg_mock.return_value = ControlMessage.from_str('250-address=17.2.89.80\r\n250 OK\r\n', 'GETINFO') + self.assertRaisesRegexp(stem.ProtocolError, 'Address unknown', self.controller.get_info, 'address') + self.controller._address_cached_at -= 60 + self.assertEqual('17.2.89.80', self.controller.get_info('address')) + + @patch('stem.control.Controller.msg') @patch('stem.control.Controller.get_conf') - def test_get_get_info_without_fingerprint(self, get_conf_mock, msg_mock): + def test_get_info_without_fingerprint(self, get_conf_mock, msg_mock): msg_mock.return_value = ControlMessage.from_str('551 Not running in server mode\r\n') get_conf_mock.return_value = None

1 0

[tor/release-0.3.2] changelog fix; thanks dgoulet
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit 023d756bfc04c244403ca7fb13e3e077dbe16408 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 27 11:20:44 2017 -0400 changelog fix; thanks dgoulet --- ChangeLog | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5cb6469f2..87517a2a5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -59,10 +59,10 @@ Changes in version 0.3.2.3-alpha - 2017-10-27 bugfix on 0.3.2.1-alpha. o Minor bugfixes (hidden service, v2): - - When reloading a configured with hidden service(s), copy all - information from the old service object. Previously, some data was - omitted, causing delays in descriptor upload, and other bugs. - Fixes bug 23790; bugfix on 0.2.1.9-alpha. + - When reloading configured hidden services, copy all information + from the old service object. Previously, some data was omitted, + causing delays in descriptor upload, and other bugs. Fixes bug + 23790; bugfix on 0.2.1.9-alpha. o Minor bugfixes (memory safety, defensive programming): - Clear the target address when node_get_prim_orport() returns

1 0

[tor/master] Rename "tell_event_loop_to_finish" to "...run_external_code"
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit 78cbced45cd244c8aab84e6fb8087b852769f5bc Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 09:59:48 2017 -0400 Rename "tell_event_loop_to_finish" to "...run_external_code" This function was never about 'finishing' the event loop, but rather about making sure that the code outside the event loop would be run at least once. --- src/or/connection_edge.c | 2 +- src/or/main.c | 4 ++-- src/or/main.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c index 77dac62b0..7d6667dbc 100644 --- a/src/or/connection_edge.c +++ b/src/or/connection_edge.c @@ -999,7 +999,7 @@ connection_ap_mark_as_pending_circuit_(entry_connection_t *entry_conn, * So the fix is to tell it right now that it ought to finish its loop at * its next available opportunity. */ - tell_event_loop_to_finish(); + tell_event_loop_to_run_external_code(); } /** Mark entry_conn as no longer waiting for a circuit. */ diff --git a/src/or/main.c b/src/or/main.c index be6162834..e3a73ef03 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -639,7 +639,7 @@ connection_should_read_from_linked_conn(connection_t *conn) * runs out of events, now we've changed our mind: tell it we want it to * finish. */ void -tell_event_loop_to_finish(void) +tell_event_loop_to_run_external_code(void) { if (!called_loop_once) { struct timeval tv = { 0, 0 }; @@ -663,7 +663,7 @@ connection_start_reading_from_linked_conn(connection_t *conn) /* make sure that the event_base_loop() function exits at * the end of its run through the current connections, so we can * activate read events for linked connections. */ - tell_event_loop_to_finish(); + tell_event_loop_to_run_external_code(); } else { tor_assert(smartlist_contains(active_linked_connection_lst, conn)); } diff --git a/src/or/main.h b/src/or/main.h index 132ab12bb..808aa1414 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -45,7 +45,7 @@ int connection_is_writing(connection_t *conn); MOCK_DECL(void,connection_stop_writing,(connection_t *conn)); MOCK_DECL(void,connection_start_writing,(connection_t *conn)); -void tell_event_loop_to_finish(void); +void tell_event_loop_to_run_external_code(void); void connection_stop_reading_from_linked_conn(connection_t *conn);

1 0

[tor/master] Expose a new function to make the event loop exit once and for all.
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit f0c3b6238168e351835fdc64b5c93b84d6528870 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 10:16:00 2017 -0400 Expose a new function to make the event loop exit once and for all. Instead of calling tor_cleanup(), exit(x), we can now call tor_shutdown_event_loop_and_exit. --- src/common/compat_libevent.h | 1 + src/or/main.c | 47 +++++++++++++++++++++++++++++++++++++++++++- src/or/main.h | 2 ++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/common/compat_libevent.h b/src/common/compat_libevent.h index 834354c40..3d8672fc6 100644 --- a/src/common/compat_libevent.h +++ b/src/common/compat_libevent.h @@ -30,6 +30,7 @@ periodic_timer_t *periodic_timer_new(struct event_base *base, void periodic_timer_free(periodic_timer_t *); #define tor_event_base_loopexit event_base_loopexit +#define tor_event_base_loopbreak event_base_loopbreak /** Defines a configuration for using libevent with Tor: passed as an argument * to tor_libevent_initialize() to describe how we want to set up. */ diff --git a/src/or/main.c b/src/or/main.c index e3a73ef03..6af61d513 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -192,6 +192,14 @@ static smartlist_t *active_linked_connection_lst = NULL; * loop_once. If so, there's no need to trigger a loopexit in order * to handle linked connections. */ static int called_loop_once = 0; +/** Flag: if true, it's time to shut down, so the main loop should exit as + * soon as possible. + */ +static int main_loop_should_exit = 0; +/** The return value that the main loop should yield when it exits, if + * main_loop_should_exit is true. + */ +static int main_loop_exit_value = 0; /** We set this to 1 when we've opened a circuit, so we can print a log * entry to inform the user that Tor is working. We set it to 0 when @@ -648,6 +656,30 @@ tell_event_loop_to_run_external_code(void) } } +/** + * After finishing the current callback (if any), shut down the main loop, + * clean up the process, and exit with exitcode. + */ +void +tor_shutdown_event_loop_and_exit(int exitcode) +{ + if (main_loop_should_exit) + return; /* Ignore multiple calls to this function. */ + + main_loop_should_exit = 1; + main_loop_exit_value = exitcode; + + /* Unlike loopexit, loopbreak prevents other callbacks from running. */ + tor_event_base_loopbreak(tor_libevent_get_base()); +} + +/** Return true iff tor_shutdown_event_loop_and_exit() has been called. */ +int +tor_event_loop_shutdown_is_pending(void) +{ + return main_loop_should_exit; +} + /** Helper: Tell the main loop to begin reading bytes into conn from * its linked connection, if it is not doing so already. Called by * connection_start_reading and connection_start_writing as appropriate. */ @@ -2595,6 +2627,9 @@ do_main_loop(void) } #endif /* defined(HAVE_SYSTEMD) */ + main_loop_should_exit = 0; + main_loop_exit_value = 0; + return run_main_loop_until_done(); } @@ -2610,6 +2645,9 @@ run_main_loop_once(void) if (nt_service_is_stopping()) return 0; + if (main_loop_should_exit) + return 0; + #ifndef _WIN32 /* Make it easier to tell whether libevent failure is our fault or not. */ errno = 0; @@ -2656,6 +2694,9 @@ run_main_loop_once(void) } } + if (main_loop_should_exit) + return 0; + /* And here is where we put callbacks that happen "every time the event loop * runs." They must be very fast, or else the whole Tor process will get * slowed down. @@ -2684,7 +2725,11 @@ run_main_loop_until_done(void) do { loop_result = run_main_loop_once(); } while (loop_result == 1); - return loop_result; + + if (main_loop_should_exit) + return main_loop_exit_value; + else + return loop_result; } /** Libevent callback: invoked when we get a signal. diff --git a/src/or/main.h b/src/or/main.h index 808aa1414..5ab77c03e 100644 --- a/src/or/main.h +++ b/src/or/main.h @@ -46,6 +46,8 @@ MOCK_DECL(void,connection_stop_writing,(connection_t *conn)); MOCK_DECL(void,connection_start_writing,(connection_t *conn)); void tell_event_loop_to_run_external_code(void); +void tor_shutdown_event_loop_and_exit(int exitcode); +int tor_event_loop_shutdown_is_pending(void); void connection_stop_reading_from_linked_conn(connection_t *conn);

1 0

[tor/master] Replace most bad exits in main.c, hibernate.c
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit 9bafc3b1efd64b19695cd78fb51070ff8d3939f5 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 10:25:55 2017 -0400 Replace most bad exits in main.c, hibernate.c This should make most of the reasons that we hibernate cleaner. --- src/or/hibernate.c | 7 +++---- src/or/main.c | 11 +++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/or/hibernate.c b/src/or/hibernate.c index be2bc7ce9..8f665a15a 100644 --- a/src/or/hibernate.c +++ b/src/or/hibernate.c @@ -818,8 +818,8 @@ hibernate_begin(hibernate_state_t new_state, time_t now) log_notice(LD_GENERAL,"SIGINT received %s; exiting now.", hibernate_state == HIBERNATE_STATE_EXITING ? "a second time" : "while hibernating"); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); + return; } if (new_state == HIBERNATE_STATE_LOWBANDWIDTH && @@ -980,8 +980,7 @@ consider_hibernation(time_t now) tor_assert(shutdown_time); if (shutdown_time <= now) { log_notice(LD_GENERAL, "Clean shutdown finished. Exiting."); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); } return; /* if exiting soon, don't worry about bandwidth limits */ } diff --git a/src/or/main.c b/src/or/main.c index c3822dd94..4d252faf2 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -2776,14 +2776,13 @@ process_signal(int sig) { case SIGTERM: log_notice(LD_GENERAL,"Catching signal TERM, exiting cleanly."); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); break; case SIGINT: if (!server_mode(get_options())) { /* do it now */ log_notice(LD_GENERAL,"Interrupt: exiting cleanly."); - tor_cleanup(); - exit(0); // XXXX bad exit + tor_shutdown_event_loop_and_exit(0); + return; } #ifdef HAVE_SYSTEMD sd_notify(0, "STOPPING=1"); @@ -2812,8 +2811,8 @@ process_signal(int sig) #endif if (do_hup() < 0) { log_warn(LD_CONFIG,"Restart failed (config error?). Exiting."); - tor_cleanup(); - exit(1); // XXXX bad exit + tor_shutdown_event_loop_and_exit(1); + return; } #ifdef HAVE_SYSTEMD sd_notify(0, "READY=1");

1 0

[tor/master] On locking failure, return -1 instead of exit()ing.
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit c247a2df6b7f6b5d997ef9e5ccc25eaaf3918db8 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 10:27:05 2017 -0400 On locking failure, return -1 instead of exit()ing. This is safe, since the only caller (options_act) will check the return value, and propagate failure. --- src/or/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/main.c b/src/or/main.c index 4d252faf2..8bfea7895 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -3265,7 +3265,7 @@ try_locking(const or_options_t *options, int err_if_locked) r = try_locking(options, 0); if (r<0) { log_err(LD_GENERAL, "No, it's still there. Exiting."); - exit(1); // XXXX bad exit + return -1; } return r; }

1 0

[tor/master] Add a failsafe to kill tor if the new exit code doesn't work.
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit c82cc8acb5aa43b3d96490d927dcc622f2c825e3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 10:22:04 2017 -0400 Add a failsafe to kill tor if the new exit code doesn't work. It _should_ work, and I don't see a reason that it wouldn't, but just in case, add a 10 second timer to make tor die with an assertion failure if it's supposed to exit but it doesn't. --- src/or/main.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/or/main.c b/src/or/main.c index 6af61d513..c3822dd94 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -656,6 +656,22 @@ tell_event_loop_to_run_external_code(void) } } +/** Failsafe measure that should never actually be necessary: If + * tor_shutdown_event_loop_and_exit() somehow doesn't successfully exit the + * event loop, then this callback will kill Tor with an assertion failure + * seconds later + */ +static void +shutdown_did_not_work_callback(evutil_socket_t fd, short event, void *arg) +{ + // LCOV_EXCL_START + (void) fd; + (void) event; + (void) arg; + tor_assert_unreached(); + // LCOV_EXCL_STOP +} + /** * After finishing the current callback (if any), shut down the main loop, * clean up the process, and exit with exitcode. @@ -669,6 +685,13 @@ tor_shutdown_event_loop_and_exit(int exitcode) main_loop_should_exit = 1; main_loop_exit_value = exitcode; + /* Die with an assertion failure in ten seconds, if for some reason we don't + * exit normally. */ + struct timeval ten_seconds = { 10, 0 }; + event_base_once(tor_libevent_get_base(), -1, EV_TIMEOUT, + shutdown_did_not_work_callback, NULL, + &ten_seconds); + /* Unlike loopexit, loopbreak prevents other callbacks from running. */ tor_event_base_loopbreak(tor_libevent_get_base()); }

1 0

[tor/master] Exit more carefully when options_act() fails.
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit 3c99b810a46c2aba2ea4108018e78bc8f6a19013 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 11:34:03 2017 -0400 Exit more carefully when options_act() fails. Also, annotate options_act() with places where we should be pre-validating values. --- src/or/config.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/or/config.c b/src/or/config.c index 3795aeaa3..6f7bf23df 100644 --- a/src/or/config.c +++ b/src/or/config.c @@ -835,9 +835,12 @@ set_options(or_options_t *new_val, char **msg) return -1; } if (options_act(old_options) < 0) { /* acting on the options failed. die. */ - log_err(LD_BUG, - "Acting on config options left us in a broken state. Dying."); - exit(1); // XXXX bad exit + if (! tor_event_loop_shutdown_is_pending()) { + log_err(LD_BUG, + "Acting on config options left us in a broken state. Dying."); + tor_shutdown_event_loop_and_exit(1); + } + return -1; } /* Issues a CONF_CHANGED event to notify controller of the change. If Tor is * just starting up then the old_options will be undefined. */ @@ -1664,8 +1667,11 @@ options_act(const or_options_t *old_options) return -1; } - if (consider_adding_dir_servers(options, old_options) < 0) + if (consider_adding_dir_servers(options, old_options) < 0) { + // XXXX This should get validated earlier, and committed here, to + // XXXX lower opportunities for reaching an error case. return -1; + } if (rend_non_anonymous_mode_enabled(options)) { log_warn(LD_GENERAL, "This copy of Tor was compiled or configured to run " @@ -1674,6 +1680,7 @@ options_act(const or_options_t *old_options) #ifdef ENABLE_TOR2WEB_MODE /* LCOV_EXCL_START */ + // XXXX This should move into options_validate() if (!options->Tor2webMode) { log_err(LD_CONFIG, "This copy of Tor was compiled to run in " "'tor2web mode'. It can only be run with the Tor2webMode torrc " @@ -1682,6 +1689,7 @@ options_act(const or_options_t *old_options) } /* LCOV_EXCL_STOP */ #else /* !(defined(ENABLE_TOR2WEB_MODE)) */ + // XXXX This should move into options_validate() if (options->Tor2webMode) { log_err(LD_CONFIG, "This copy of Tor was not compiled to run in " "'tor2web mode'. It cannot be run with the Tor2webMode torrc " @@ -1850,7 +1858,7 @@ options_act(const or_options_t *old_options) /* Set up accounting */ if (accounting_parse_options(options, 0)<0) { - log_warn(LD_CONFIG,"Error in accounting options"); + log_warn(LD_BUG,"Error in previously validated accounting options"); return -1; } if (accounting_is_enabled(options)) @@ -1874,6 +1882,7 @@ options_act(const or_options_t *old_options) char *http_authenticator; http_authenticator = alloc_http_authenticator(options->BridgePassword); if (!http_authenticator) { + // XXXX This should get validated in options_validate(). log_warn(LD_BUG, "Unable to allocate HTTP authenticator. Not setting " "BridgePassword."); return -1; @@ -1886,7 +1895,8 @@ options_act(const or_options_t *old_options) } if (parse_outbound_addresses(options, 0, &msg) < 0) { - log_warn(LD_BUG, "Failed parsing outbound bind addresses: %s", msg); + log_warn(LD_BUG, "Failed parsing previously validated outbound " + "bind addresses: %s", msg); tor_free(msg); return -1; }

1 0

[tor/master] Changes file for ticket 23848
by nickm＠torproject.org 27 Oct '17

27 Oct '17

commit 90daa28df403472d4a6aa7067ce6ab3264cdbe55 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Oct 20 11:39:11 2017 -0400 Changes file for ticket 23848 --- changes/bug23848 | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/changes/bug23848 b/changes/bug23848 new file mode 100644 index 000000000..e2aec687c --- /dev/null +++ b/changes/bug23848 @@ -0,0 +1,8 @@ + o Minor features (embedding): + - On most errors that would cause Tor to exit, it now tries to return + from the tor_main() function, rather than calling the system exit() + function. Most users won't notice a difference here, but it should + make a significant difference on platforms that try to run Tor inside + a separate thread: they should now be able to survive Tor's exit + conditions rather than having Tor shut down the entire process. + Closes ticket 23848.

1 0