- tor-commits - lists.torproject.org

[arm/master] fix: loop from closing conn based on torctl event
by atagar＠torproject.org 14 Jun '11

14 Jun '11

commit e272d05973abcc3b92acecb4e944c8861ed58903 Author: Damian Johnson <atagar(a)torproject.org> Date: Mon Jun 13 20:35:56 2011 -0700 fix: loop from closing conn based on torctl event Attempting to close our control connection when TorCtl logged a message saying that it was done caused us to attempt to join our own thread within torctl (which causes an exception). I'm not really recalling why I ever did this, but fixes for concurrency issues in TorCtl are triggering this so removing the check. --- src/util/torTools.py | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-) diff --git a/src/util/torTools.py b/src/util/torTools.py index f17ed0b..a4c0b5d 100644 --- a/src/util/torTools.py +++ b/src/util/torTools.py @@ -67,7 +67,6 @@ CACHE_GETINFO_PREFIX_ARGS = ("ip-to-country/", ) # so erring on the side of inclusiveness by using the notice event instead. ADDR_CHANGED_MSG_PREFIX = ("Our IP Address has changed from", "Guessed our IP address as") -TOR_CTL_CLOSE_MSG = "Tor closed control connection. Exiting event thread." UNKNOWN = "UNKNOWN" # value used by cached information if undefined CONFIG = {"torrc.map": {}, "features.pathPrefix": "", @@ -1605,9 +1604,6 @@ class Controller(TorCtl.PostEventListener): # notifies listeners of TorCtl events for callback in self.torctlListeners: callback(TORCTL_RUNLEVELS[level], msg) - # checks if TorCtl is providing a notice that control port is closed - if TOR_CTL_CLOSE_MSG in msg: self.close() - # if the message is informing us of our ip address changing then clear # its cached value for prefix in ADDR_CHANGED_MSG_PREFIX:

1 0

[arm/master] Dropping join on torctl's internal thread
by atagar＠torproject.org 14 Jun '11

14 Jun '11

commit 290eaa974ee4d36a99981109bc8a89fbcd392e01 Author: Damian Johnson <atagar(a)torproject.org> Date: Mon Jun 13 20:47:46 2011 -0700 Dropping join on torctl's internal thread A fix for the root cause this addressed has been sent to Mike in... https://trac.torproject.org/projects/tor/ticket/2412 --- src/util/torTools.py | 14 -------------- 1 files changed, 0 insertions(+), 14 deletions(-) diff --git a/src/util/torTools.py b/src/util/torTools.py index a4c0b5d..5a47175 100644 --- a/src/util/torTools.py +++ b/src/util/torTools.py @@ -457,20 +457,6 @@ class Controller(TorCtl.PostEventListener): self.connLock.acquire() if self.conn: self.conn.close() - - # If we're closing due to an event from TorCtl (for instance, tor was - # stopped) then TorCtl is shutting itself down and there's no need to - # join on its thread (actually, this *is* the TorCtl thread in that - # case so joining on it causes deadlock). - # - # This poses a slight possability of shutting down with a live orphaned - # thread if Tor is shut down, then arm shuts down before TorCtl has a - # chance to terminate. However, I've never seen that occure so leaving - # that alone for now. - - if not threading.currentThread() == self.conn._thread: - self.conn._thread.join() - self.conn = None self.connLock.release()

1 0

[arm/master] fix: race condition for multiple status changes
by atagar＠torproject.org 14 Jun '11

14 Jun '11

commit 56d0105a50809f7e272f3d0d1f3d2b25eca2d498 Author: Damian Johnson <atagar(a)torproject.org> Date: Tue Jun 14 09:13:20 2011 -0700 fix: race condition for multiple status changes When an event caused multiple changes in Tor's status we'd run into a race condition and often pick the wrong final state. For instance, if a sighup caused Tor to crash (for instance via a bad torrc) then we'd trigger both a reset and closed event. This enqueues events to keep the order in which they occured, expands locks to include them, and introduces a small delay to coles events if they occure in quick succession like this. --- src/util/torTools.py | 77 +++++++++++++++++++++++++++++++++++++------------ 1 files changed, 58 insertions(+), 19 deletions(-) diff --git a/src/util/torTools.py b/src/util/torTools.py index 5a47175..8c18a9d 100644 --- a/src/util/torTools.py +++ b/src/util/torTools.py @@ -9,6 +9,7 @@ import time import socket import thread import threading +import Queue from TorCtl import TorCtl, TorUtil @@ -373,6 +374,15 @@ class Controller(TorCtl.PostEventListener): self._statusTime = 0 # unix time-stamp for the duration of the status self.lastHeartbeat = 0 # time of the last tor event + # Status signaling for when tor starts, stops, or is reset is done via + # enquing the signal then spawning a handler thread. This is to provide + # safety in race conditions, for instance if we sighup with a torrc that + # causes tor to crash then we'll get both an INIT and CLOSED signal. It's + # important in those cases that listeners get the correct signal last (in + # that case CLOSED) so they aren't confused about what tor's current state + # is. + self._notificationQueue = Queue.Queue() + self._exitPolicyChecker = None self._isExitingAllowed = False self._exitPolicyLookupCache = {} # mappings of ip/port tuples to if they were accepted by the policy or not @@ -440,14 +450,15 @@ class Controller(TorCtl.PostEventListener): # are dropped with a logged warning) self.setControllerEvents(self.controllerEvents) - self.connLock.release() - self._status = State.INIT self._statusTime = time.time() # notifies listeners that a new controller is available if not NO_SPAWN: - thread.start_new_thread(self._notifyStatusListeners, (State.INIT,)) + self._notificationQueue.put(State.INIT) + thread.start_new_thread(self._notifyStatusListeners, ()) + + self.connLock.release() def close(self): """ @@ -458,14 +469,16 @@ class Controller(TorCtl.PostEventListener): if self.conn: self.conn.close() self.conn = None - self.connLock.release() self._status = State.CLOSED self._statusTime = time.time() # notifies listeners that the controller's been shut down if not NO_SPAWN: - thread.start_new_thread(self._notifyStatusListeners, (State.CLOSED,)) + self._notificationQueue.put(State.CLOSED) + thread.start_new_thread(self._notifyStatusListeners, ()) + + self.connLock.release() else: self.connLock.release() def isAlive(self): @@ -1459,13 +1472,19 @@ class Controller(TorCtl.PostEventListener): """ if event.level == "NOTICE" and event.msg.startswith("Received reload signal (hup)"): - self._isReset = True + self.connLock.acquire() - self._status = State.INIT - self._statusTime = time.time() + if self.isAlive(): + self._isReset = True + + self._status = State.INIT + self._statusTime = time.time() + + if not NO_SPAWN: + self._notificationQueue.put(State.INIT) + thread.start_new_thread(self._notifyStatusListeners, ()) - if not NO_SPAWN: - thread.start_new_thread(self._notifyStatusListeners, (State.INIT,)) + self.connLock.release() def ns_event(self, event): self._updateHeartbeat() @@ -2006,7 +2025,7 @@ class Controller(TorCtl.PostEventListener): if result == None or result == UNKNOWN: return default else: return result - def _notifyStatusListeners(self, eventType): + def _notifyStatusListeners(self): """ Sends a notice to all current listeners that a given change in tor's controller status has occurred. @@ -2015,16 +2034,36 @@ class Controller(TorCtl.PostEventListener): eventType - enum representing tor's new status """ - # resets cached GETINFO and GETCONF parameters - self._cachedParam = {} - self._cachedConf = {} + # If there's a quick race state (for instance a sighup causing both an init + # and close event) then give them a moment to enqueue. This way we can + # coles the events and discard the inaccurate one. + + time.sleep(0.2) + + self.connLock.acquire() - # gives a notice that the control port has closed - if eventType == State.CLOSED: - log.log(CONFIG["log.torCtlPortClosed"], "Tor control port closed") + try: + eventType = self._notificationQueue.get(timeout=0) + + # checks that the notice is accurate for our current state + if self.isAlive() != (eventType == State.INIT): + eventType = None + except Queue.Empty: + eventType = None + + if eventType: + # resets cached GETINFO and GETCONF parameters + self._cachedParam = {} + self._cachedConf = {} + + # gives a notice that the control port has closed + if eventType == State.CLOSED: + log.log(CONFIG["log.torCtlPortClosed"], "Tor control port closed") + + for callback in self.statusListeners: + callback(self, eventType) - for callback in self.statusListeners: - callback(self, eventType) + self.connLock.release() class ExitPolicy: """

1 0

[tor/master] Merge remote-tracking branch 'origin/maint-0.2.2'
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit 8839b86085dbb9ccf26165b6eae4d09462bc88b6 Merge: 910dadd 54d7d31 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 14 12:25:33 2011 -0400 Merge remote-tracking branch 'origin/maint-0.2.2' changes/coverity_maint | 9 +++++++++ src/common/util.c | 33 ++++++++++++++++++++++++++------- src/common/util.h | 3 ++- src/or/circuitlist.c | 4 ++-- src/or/config.c | 6 ++++-- src/or/connection.c | 2 +- src/or/geoip.c | 6 +++--- src/or/policies.c | 2 +- src/or/rendservice.c | 2 +- src/or/rephist.c | 13 +++++++------ src/or/router.c | 4 ++-- src/or/routerparse.c | 6 +++--- src/test/test_addr.c | 3 +++ 13 files changed, 64 insertions(+), 29 deletions(-)

1 0

[tor/master] Fix another check_private_dir instance in master
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit 22efe2030901e7ef878c8ec358e819bbdb1239f6 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Jun 14 12:28:16 2011 -0400 Fix another check_private_dir instance in master --- src/or/rephist.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/or/rephist.c b/src/or/rephist.c index 3cc6185..b4dbba7 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -2758,7 +2758,7 @@ rep_hist_conn_stats_write(time_t now) /* Try to write to disk. */ statsdir = get_datadir_fname("stats"); - if (check_private_dir(statsdir, CPD_CREATE) < 0) { + if (check_private_dir(statsdir, CPD_CREATE, get_options()->User) < 0) { log_warn(LD_HIST, "Unable to create stats/ directory!"); goto done; }

1 0

[tor/master] remove some dead code, found by coverity
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit db7dd3ee7a07d9bc121d1c8f353e206327bb72c5 Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Wed Jun 8 20:02:16 2011 +0200 remove some dead code, found by coverity --- changes/coverity_maint | 3 +++ src/or/rephist.c | 3 +-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/changes/coverity_maint b/changes/coverity_maint new file mode 100644 index 0000000..ec25d09 --- /dev/null +++ b/changes/coverity_maint @@ -0,0 +1,3 @@ + o Code simplifications and refactoring: + - Remove some dead code as indicated by coverity. + diff --git a/src/or/rephist.c b/src/or/rephist.c index 6be8484..242fe81 100644 --- a/src/or/rephist.c +++ b/src/or/rephist.c @@ -2401,8 +2401,7 @@ rep_hist_buffer_stats_add_circ(circuit_t *circ, time_t end_of_interval) stat = tor_malloc_zero(sizeof(circ_buffer_stats_t)); stat->processed_cells = orcirc->processed_cells; /* 1000.0 for s -> ms; 2.0 because of app-ward and exit-ward queues */ - stat->mean_num_cells_in_queue = interval_length == 0 ? 0.0 : - (double) orcirc->total_cell_waiting_time / + stat->mean_num_cells_in_queue = (double) orcirc->total_cell_waiting_time / (double) interval_length / 1000.0 / 2.0; stat->mean_time_cells_in_queue = (double) orcirc->total_cell_waiting_time /

1 0

[tor/master] Check some more return values in unit tests
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit ff75e8b02dbf35c2ecc82ffd4d2b2112a144322e Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Wed Jun 8 21:06:01 2011 +0200 Check some more return values in unit tests --- changes/coverity_maint | 3 +++ src/test/test_addr.c | 3 +++ 2 files changed, 6 insertions(+), 0 deletions(-) diff --git a/changes/coverity_maint b/changes/coverity_maint index ec25d09..1bd1c41 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -1,3 +1,6 @@ o Code simplifications and refactoring: - Remove some dead code as indicated by coverity. + o Minor bugfixes: + - Add some forgotten return value checks during unit tests. Found + by coverity. diff --git a/src/test/test_addr.c b/src/test/test_addr.c index 6db4ee2..1dab0e0 100644 --- a/src/test/test_addr.c +++ b/src/test/test_addr.c @@ -436,13 +436,16 @@ test_addr_ip6_helpers(void) /* test tor_addr_parse_mask_ports */ test_addr_mask_ports_parse("[::f]/17:47-95", AF_INET6, 0, 0, 0, 0x0000000f, 17, 47, 95); + test_streq(p1, "::f"); //test_addr_parse("[::fefe:4.1.1.7/120]:999-1000"); //test_addr_parse_check("::fefe:401:107", 120, 999, 1000); test_addr_mask_ports_parse("[::ffff:4.1.1.7]/120:443", AF_INET6, 0, 0, 0x0000ffff, 0x04010107, 120, 443, 443); + test_streq(p1, "::ffff:4.1.1.7"); test_addr_mask_ports_parse("[abcd:2::44a:0]:2-65000", AF_INET6, 0xabcd0002, 0, 0, 0x044a0000, 128, 2, 65000); + test_streq(p1, "abcd:2::44a:0"); r=tor_addr_parse_mask_ports("[fefef::]/112", &t1, NULL, NULL, NULL); test_assert(r == -1); r=tor_addr_parse_mask_ports("efef::/112", &t1, NULL, NULL, NULL);

1 0

[tor/master] Remove a few dead assignments during router parsing
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit e6fff7235e46b794cd77af516a1ecfab9c87de7f Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Wed Jun 8 21:16:11 2011 +0200 Remove a few dead assignments during router parsing --- changes/coverity_maint | 1 + src/or/routerparse.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/changes/coverity_maint b/changes/coverity_maint index 1bd1c41..fd8c440 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -1,5 +1,6 @@ o Code simplifications and refactoring: - Remove some dead code as indicated by coverity. + - Remove a few dead assignments during router parsing. Found by coverity. o Minor bugfixes: - Add some forgotten return value checks during unit tests. Found by coverity. diff --git a/src/or/routerparse.c b/src/or/routerparse.c index f855f9d..42dbcac 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -1544,10 +1544,10 @@ router_parse_entry_from_string(const char *s, const char *end, } } - if ((tok = find_opt_by_keyword(tokens, K_CACHES_EXTRA_INFO))) + if (find_opt_by_keyword(tokens, K_CACHES_EXTRA_INFO)) router->caches_extra_info = 1; - if ((tok = find_opt_by_keyword(tokens, K_ALLOW_SINGLE_HOP_EXITS))) + if (find_opt_by_keyword(tokens, K_ALLOW_SINGLE_HOP_EXITS)) router->allow_single_hop_exits = 1; if ((tok = find_opt_by_keyword(tokens, K_EXTRA_INFO_DIGEST))) { @@ -1560,7 +1560,7 @@ router_parse_entry_from_string(const char *s, const char *end, } } - if ((tok = find_opt_by_keyword(tokens, K_HIDDEN_SERVICE_DIR))) { + if (find_opt_by_keyword(tokens, K_HIDDEN_SERVICE_DIR)) { router->wants_to_be_hs_dir = 1; }

1 0

[tor/master] Comment out some obviously dead code.
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit 9e56ac27da80969fadaad7eaf64a8384e68a778e Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Wed Jun 8 21:23:05 2011 +0200 Comment out some obviously dead code. Coverity warned about it, it's harmless to comment out. --- src/or/circuitlist.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 4ad244d..8534c38 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -775,8 +775,8 @@ circuit_get_by_circid_orconn_impl(circid_t circ_id, or_connection_t *conn) return found->circuit; return NULL; - /* The rest of this checks for bugs. Disabled by default. */ + /* We comment it out because coverity complains otherwise. { circuit_t *circ; for (circ=global_circuitlist;circ;circ = circ->next) { @@ -795,7 +795,7 @@ circuit_get_by_circid_orconn_impl(circid_t circ_id, or_connection_t *conn) } } return NULL; - } + } */ } /** Return a circ such that:

1 0

[tor/master] Don't use signed 1-bit bitfields
by nickm＠torproject.org 14 Jun '11

14 Jun '11

commit 680646e0de29454f92d57bc3a4895d75c95e158c Author: Sebastian Hahn <sebastian(a)torproject.org> Date: Wed Jun 8 21:27:32 2011 +0200 Don't use signed 1-bit bitfields This was harmless, we never compared it to anything but itself or 0. But Coverity complained, and it had a point. --- changes/coverity_maint | 1 + src/or/policies.c | 2 +- 2 files changed, 2 insertions(+), 1 deletions(-) diff --git a/changes/coverity_maint b/changes/coverity_maint index fd8c440..6d60355 100644 --- a/changes/coverity_maint +++ b/changes/coverity_maint @@ -4,4 +4,5 @@ o Minor bugfixes: - Add some forgotten return value checks during unit tests. Found by coverity. + - Don't use 1-bit wide signed bit fields. Found by coverity. diff --git a/src/or/policies.c b/src/or/policies.c index e48f420..c870360 100644 --- a/src/or/policies.c +++ b/src/or/policies.c @@ -45,7 +45,7 @@ typedef struct policy_summary_item_t { uint16_t prt_max; /**< Highest port number to accept/reject. */ uint64_t reject_count; /**< Number of IP-Addresses that are rejected to this port range. */ - int accepted:1; /** Has this port already been accepted */ + unsigned int accepted:1; /** Has this port already been accepted */ } policy_summary_item_t; /** Private networks. This list is used in two places, once to expand the

1 0