April 2014 - tor-commits - lists.torproject.org

[bridgedb/develop] Fix recursion in filtered log call in validation used by log filter.
by isis＠torproject.org 08 Apr '14

08 Apr '14

commit d68f8e2427a5b9623a7d4907c177bf2cc46da9b2 Author: Isis Lovecruft <isis(a)torproject.org> Date: Mon Apr 7 12:32:47 2014 +0000 Fix recursion in filtered log call in validation used by log filter. We can't log an IP address in a IP validation function which is used by a log filter in `bridgedb.safelog`, because the log call made in the validation function `bridgedb.parse.addr._isIPv()` will call the filter, which will call the validation function, which will call the logger, which will call the filter… --- lib/bridgedb/parse/addr.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/bridgedb/parse/addr.py b/lib/bridgedb/parse/addr.py index 927de2f..455b953 100644 --- a/lib/bridgedb/parse/addr.py +++ b/lib/bridgedb/parse/addr.py @@ -181,20 +181,26 @@ def isIPAddress(ip, compressed=True): def _isIPv(version, ip): """Check if **ip** is a certain **version** (IPv4 or IPv6). + .. warning: Do *not* put any calls to the logging module in this function, + or else an infinite recursion will occur when the call is made, due + the the log :class:`~logging.Filter`s in :mod:`~bridgedb.safelog` + using this function to validate matches from the regular expression + for IP addresses. + :param integer version: The IPv[4|6] version to check; must be either - ``4`` or ``6``. + ``4`` or ``6``. Any other value will be silently changed to ``4``. :param ip: The IP address to check. May be an any type which :class:`ipaddr.IPAddress` will accept. :rtype: boolean :returns: ``True``, if the address is an IPv4 address. """ try: - ip = ipaddr.IPAddress(ip, version=version) - except ipaddr.AddressValueError: - logging.debug("Address %s seems not to be IPv%d." % (ip, version)) + ipaddr.IPAddress(ip, version=version) + except (ipaddr.AddressValueError, Exception): return False else: return True + return False def isIPv4(ip): """Check if an address is IPv4.

1 0

[bridgedb/develop] Merge branch 'fix/9199-safelogging' into develop
by isis＠torproject.org 08 Apr '14

08 Apr '14

commit 0275fa56cbd39c6f452cc973ac34d955f0ec6603 Merge: 33499da d68f8e2 Author: Isis Lovecruft <isis(a)torproject.org> Date: Tue Apr 8 16:08:19 2014 +0000 Merge branch 'fix/9199-safelogging' into develop bridgedb.conf | 18 +- doc/sphinx/source/bridgedb.rst | 1 + doc/sphinx/source/bridgedb.safelog.rst | 9 + doc/sphinx/source/conf.py | 1 + lib/bridgedb/Bridges.py | 10 +- lib/bridgedb/Dist.py | 23 ++- lib/bridgedb/EmailServer.py | 24 +-- lib/bridgedb/HTTPServer.py | 15 +- lib/bridgedb/Main.py | 54 +---- lib/bridgedb/parse/addr.py | 14 +- lib/bridgedb/safelog.py | 197 +++++++++++++++++++ lib/bridgedb/test/test_safelog.py | 336 ++++++++++++++++++++++++++++++++ lib/bridgedb/util.py | 201 ++++++++++++++++++- 13 files changed, 806 insertions(+), 97 deletions(-)

1 0

[bridgedb/develop] Add unittests for bridgedb.safelog module.
by isis＠torproject.org 08 Apr '14

08 Apr '14

commit 0dc4e5b040e7dbe9714ea5b563caa32182a07cc7 Author: Isis Lovecruft <isis(a)torproject.org> Date: Mon Apr 7 12:28:07 2014 +0000 Add unittests for bridgedb.safelog module. * FIXES #9875 Write tests for BridgeDB's logger --- lib/bridgedb/test/test_safelog.py | 336 +++++++++++++++++++++++++++++++++++++ 1 file changed, 336 insertions(+) diff --git a/lib/bridgedb/test/test_safelog.py b/lib/bridgedb/test/test_safelog.py new file mode 100644 index 0000000..bc92305 --- /dev/null +++ b/lib/bridgedb/test/test_safelog.py @@ -0,0 +1,336 @@ +# -*- coding: utf-8 -*- + +"""Unittests for :mod:`bridgedb.safelog`.""" + +import re + +from twisted.internet import defer +from twisted.test.proto_helpers import StringTransport +from twisted.trial import unittest + +from bridgedb import safelog + + +class SafelogTests(unittest.TestCase): + """Tests for functions and attributes in :mod:`bridgedb.safelog`.""" + + def setUp(self): + """Create a logger at debug level and add the filter to be tested.""" + self.logfile = StringTransport() + self.handler = safelog.logging.StreamHandler(self.logfile) + self.logger = safelog.logging.getLogger(self.__class__.__name__) + self.logger.setLevel(10) + self.logger.addHandler(self.handler) + self.sensitiveData = 'Nicholas Bourbaki' + + def tearDown(self): + """Rewind and truncate the logfile so that we have an empty one.""" + self.logfile.clear() + + def test_setSafeLogging_off(self): + """Calls to ``logSafely()`` should return the original data when + ``safe_logging`` is disabled. + """ + safelog.setSafeLogging(False) + self.logger.warn("Got a connection from %s..." + % safelog.logSafely(self.sensitiveData)) + contents = self.logfile.value() + self.assertIsNotNone(contents) + #self.assertSubstring("Got a connection from", contents) + #self.assertSubstring(self.sensitiveData, contents) + #self.failIfSubstring("[scrubbed]", contents) + + def test_setSafeLogging_on(self): + """Calls to ``logSafely()`` should return ``"[scrubbed]"`` for any + arbitrary data when ``safe_logging`` is enabled. + """ + safelog.setSafeLogging(True) + self.logger.warn("Got a connection from %s..." + % safelog.logSafely(self.sensitiveData)) + contents = self.logfile.value() + self.assertIsNotNone(contents) + #self.assertSubstring("Got a connection from", contents) + #self.failIfSubstring(self.sensitiveData, contents) + #self.assertSubstring("[scrubbed]", contents) + + +class BaseSafelogFilterTests(unittest.TestCase): + """Unittests for :class:`bridgedb.safelog.BaseSafelogFilter`.""" + + def setUp(self): + safelog.setSafeLogging(True) + self.logfile = StringTransport() + self.handler = safelog.logging.StreamHandler(self.logfile) + self.logger = safelog.logging.getLogger(self.__class__.__name__) + self.logger.setLevel(10) + self.logger.addHandler(self.handler) + self.filter = safelog.BaseSafelogFilter() + self.logger.addFilter(self.filter) + + self.logMessage = "testing 1 2 3" + self.record = safelog.logging.LogRecord('name', 10, __file__, 1337, + self.logMessage, {}, None) + + def test_doubleCheck(self): + """BaseSafelogFilter.doubleCheck() should always return True.""" + checked = self.filter.doubleCheck(self.logMessage) + self.assertTrue(checked) + + def test_filter(self): + """Test filtering a log record with no ``easyFind`` nor ``pattern``. + + The ``LogRecord.message`` shouldn't change. + """ + filtered = self.filter.filter(self.record) + self.assertEqual(filtered.getMessage(), self.logMessage) + + def test_filter_withEasyFind(self): + """Test filtering a log record with ``easyFind``, but no ``pattern``. + + The ``LogRecord.message`` shouldn't change. + """ + self.filter.easyFind = "2" + filtered = self.filter.filter(self.record) + self.assertEqual(filtered.getMessage(), self.logMessage) + + def test_filter_withPattern(self): + """Test filtering a log record with ``easyFind`` and ``pattern``.""" + self.filter.easyFind = "2" + self.filter.pattern = re.compile("1 2 3") + filtered = self.filter.filter(self.record) + self.assertEqual(filtered.msg, "testing [scrubbed]") + + +class SafelogEmailFilterTests(unittest.TestCase): + """Unittests for :class:`bridgedb.safelog.SafelogEmailFilter`.""" + + def setUp(self): + """Create a logger at debug level and add the filter to be tested.""" + self.logfile = StringTransport() + self.handler = safelog.logging.StreamHandler(self.logfile) + self.filter = safelog.SafelogEmailFilter() + self.logger = safelog.logging.getLogger(self.__class__.__name__) + self.logger.setLevel(10) + self.logger.addHandler(self.handler) + self.logger.addFilter(self.filter) + self.s1 = "Here is an email address: " + self.s2 = "blackhole(a)torproject.org" + + def test_filter_withPattern(self): + """Test filtering a log record with ``easyFind`` and ``pattern``.""" + record = safelog.logging.LogRecord('name', 10, __file__, 1337, + "testing blackhole(a)torproject.org", + {}, None) + filtered = self.filter.filter(record) + self.assertEqual(filtered.msg, "testing [scrubbed]") + + def test_debugLevel(self): + self.logger.debug("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + # XXX We should test the following assertions for each test_*Level + # method, however, twisted.trial doesn't give us an easy way to wait + # for the logging module to complete it's IO operations. + #self.assertSubstring(self.s1, contents) + #self.failIfSubstring(self.s2, contents) + + def test_infoLevel(self): + self.logger.info("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_warnLevel(self): + self.logger.warn("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_errorLevel(self): + self.logger.error("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_exceptionLevel(self): + try: + raise Exception("%s %s" % (self.s1, self.s2)) + except Exception as error: + self.logger.exception(error) + + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + #self.assertSubstring(self.s1, contents) + # If an email address is within an Exception message, it doesn't get + # sanitised. + #self.assertSubstring(self.s2, contents) + + def test_withSafeLoggingDisabled(self): + """The filter should be disabled if ``safe_logging`` is disabled.""" + safelog.setSafeLogging(False) + self.logger.info("%s %s" % (self.s1, self.s2)) + self.logfile.io.flush() + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + #self.assertSubstring(self.s1, contents) + #self.assertSubstring(self.s2, contents) + + +class SafelogIPv4FilterTests(unittest.TestCase): + """Unittests for :class:`bridgedb.safelog.SafelogIPv4Filter`.""" + + def setUp(self): + """Create a logger at debug level and add the filter to be tested.""" + self.logfile = StringTransport() + self.handler = safelog.logging.StreamHandler(self.logfile) + self.filter = safelog.SafelogIPv4Filter() + self.logger = safelog.logging.getLogger(str(self.__class__.__name__)) + self.logger.addHandler(self.handler) + self.logger.addFilter(self.filter) + self.logger.setLevel(10) + self.s1 = "There's an IPv4 address at the end of this book: " + self.s2 = "1.2.3.4" + + def test_filter_withPattern(self): + """Test filtering a log record with ``easyFind`` and ``pattern``.""" + record = safelog.logging.LogRecord('name', 10, __file__, 1337, + "testing 1.2.3.4", + {}, None) + filtered = self.filter.filter(record) + self.assertIsInstance(filtered, safelog.logging.LogRecord) + + def test_doubleCheck_IPv4(self): + checked = self.filter.doubleCheck("1.2.3.4") + self.assertIs(checked, True) + + def test_doubleCheck_IPv6(self): + checked = self.filter.doubleCheck("2af1:a470:9b36::a1:3:82") + self.assertIsNot(checked, True) + + def test_debugLevel(self): + self.logger.debug("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_infoLevel(self): + self.logger.info("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_warnLevel(self): + self.logger.warn("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_errorLevel(self): + self.logger.error("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_exceptionLevel(self): + try: + raise Exception("%s %s" % (self.s1, self.s2)) + except Exception as error: + self.logger.exception(error) + + self.logfile.io.flush() + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_withSafeLoggingDisabled(self): + """The filter should be disabled if ``safe_logging`` is disabled.""" + safelog.setSafeLogging(False) + self.logger.info("%s %s" % (self.s1, self.s2)) + self.logfile.io.flush() + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + +class SafelogIPv6FilterTests(unittest.TestCase): + """Unittests for :class:`bridgedb.safelog.SafelogIPv6Filter`.""" + + def setUp(self): + """Create a logger at debug level and add the filter to be tested.""" + self.logfile = StringTransport() + self.handler = safelog.logging.StreamHandler(self.logfile) + self.filter = safelog.SafelogIPv6Filter() + self.logger = safelog.logging.getLogger(str(self.__class__.__name__)) + self.logger.addHandler(self.handler) + self.logger.addFilter(self.filter) + self.logger.setLevel(10) + self.s1 = "There's an IPv6 address at the end of this book: " + self.s2 = "2af1:a470:9b36::a1:3:82" + + def test_filter_withPattern(self): + """Test filtering a log record with ``easyFind`` and ``pattern``.""" + record = safelog.logging.LogRecord('name', 10, __file__, 1337, + "2af1:a470:9b36::a1:3:82", + {}, None) + filtered = self.filter.filter(record) + self.assertIsInstance(filtered, safelog.logging.LogRecord) + + def test_doubleCheck_IPv4(self): + checked = self.filter.doubleCheck("1.2.3.4") + self.assertIsNot(checked, True) + + def test_doubleCheck_IPv6(self): + checked = self.filter.doubleCheck("2af1:a470:9b36::a1:3:82") + self.assertIs(checked, True) + + def test_debugLevel(self): + self.logger.debug("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_infoLevel(self): + self.logger.info("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_warnLevel(self): + self.logger.warn("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_errorLevel(self): + self.logger.error("%s %s" % (self.s1, self.s2)) + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + + def test_exceptionLevel(self): + try: + raise Exception("%s %s" % (self.s1, self.s2)) + except Exception as error: + self.logger.exception(error) + + self.logfile.io.flush() + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents) + #self.assertSubstring(self.s1, contents) + # If an IP address is within an Exception message, it doesn't get + # sanitised. + #self.assertSubstring(self.s2, contents) + + def test_withSafeLoggingDisabled(self): + """The filter should be disabled if ``safe_logging`` is disabled.""" + safelog.setSafeLogging(False) + self.logger.info("%s %s" % (self.s1, self.s2)) + + self.logfile.io.flush() + self.logfile.io.seek(0) + contents = self.logfile.value() + self.assertIsNotNone(contents)

1 0

[tor/master] Fix to 9841 fix: setting a token to NULL should clear it
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit 09dbcf3b82c6db76024a5345b98f34028b40c30c Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 4 12:01:49 2014 -0400 Fix to 9841 fix: setting a token to NULL should clear it Found by testing with chutney. The old behavior was "fail an assertion", which obviously isn't optimal. Bugfix on 8b9a2cb68b290e550695124d7ef0511225b451d5; bug not in any released version. --- src/or/circuitlist.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 8ab673c..be71fdd 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1233,6 +1233,11 @@ circuit_set_rend_token(or_circuit_t *circ, int is_rend_circ, if (circ->rendinfo) circuit_clear_rend_token(circ); + if (token == NULL) { + /* We were only trying to remove this token, not set a new one. */ + return; + } + found_circ = digestmap_get(map, (const char *)token); if (found_circ) { tor_assert(found_circ != circ);

1 0

[tor/master] Tests for rend_token maps
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit fb595922b12c9ab89f51527def68e9b71386caf7 Author: Nick Mathewson <nickm(a)torproject.org> Date: Thu Feb 13 15:24:15 2014 -0500 Tests for rend_token maps This gets coverage of everything except for the cases which should be impossible. --- src/test/test_circuitlist.c | 85 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/src/test/test_circuitlist.c b/src/test/test_circuitlist.c index 720b407..ee62cd2 100644 --- a/src/test/test_circuitlist.c +++ b/src/test/test_circuitlist.c @@ -161,8 +161,93 @@ test_clist_maps(void *arg) UNMOCK(circuitmux_detach_circuit); } +static void +test_rend_token_maps(void *arg) +{ + or_circuit_t *c1, *c2, *c3, *c4; + const uint8_t tok1[REND_TOKEN_LEN] = "The cat can't tell y"; + const uint8_t tok2[REND_TOKEN_LEN] = "ou its name, and it "; + const uint8_t tok3[REND_TOKEN_LEN] = "doesn't really care."; + /* -- Adapted from a quote by Fredrik Lundh. */ + + (void)arg; + (void)tok1; //xxxx + c1 = or_circuit_new(0, NULL); + c2 = or_circuit_new(0, NULL); + c3 = or_circuit_new(0, NULL); + c4 = or_circuit_new(0, NULL); + + tt_int_op(strlen((char*)tok1), ==, REND_TOKEN_LEN); + + /* No maps; nothing there. */ + tt_ptr_op(NULL, ==, circuit_get_rendezvous(tok1)); + tt_ptr_op(NULL, ==, circuit_get_intro_point(tok1)); + + circuit_set_rendezvous_cookie(c1, tok1); + circuit_set_intro_point_digest(c2, tok2); + + tt_ptr_op(NULL, ==, circuit_get_rendezvous(tok3)); + tt_ptr_op(NULL, ==, circuit_get_intro_point(tok3)); + tt_ptr_op(NULL, ==, circuit_get_rendezvous(tok2)); + tt_ptr_op(NULL, ==, circuit_get_intro_point(tok1)); + + /* Without purpose set, we don't get the circuits */ + tt_ptr_op(NULL, ==, circuit_get_rendezvous(tok1)); + tt_ptr_op(NULL, ==, circuit_get_intro_point(tok2)); + + c1->base_.purpose = CIRCUIT_PURPOSE_REND_POINT_WAITING; + c2->base_.purpose = CIRCUIT_PURPOSE_INTRO_POINT; + + /* Okay, make sure they show up now. */ + tt_ptr_op(c1, ==, circuit_get_rendezvous(tok1)); + tt_ptr_op(c2, ==, circuit_get_intro_point(tok2)); + + /* Two items at the same place with the same token. */ + c3->base_.purpose = CIRCUIT_PURPOSE_REND_POINT_WAITING; + circuit_set_rendezvous_cookie(c3, tok2); + tt_ptr_op(c2, ==, circuit_get_intro_point(tok2)); + tt_ptr_op(c3, ==, circuit_get_rendezvous(tok2)); + + /* Marking a circuit makes it not get returned any more */ + circuit_mark_for_close(TO_CIRCUIT(c1), END_CIRC_REASON_FINISHED); + tt_ptr_op(NULL, ==, circuit_get_rendezvous(tok1)); + circuit_free(TO_CIRCUIT(c1)); + c1 = NULL; + + /* Freeing a circuit makes it not get returned any more. */ + circuit_free(TO_CIRCUIT(c2)); + c2 = NULL; + tt_ptr_op(NULL, ==, circuit_get_intro_point(tok2)); + + /* c3 -- are you still there? */ + tt_ptr_op(c3, ==, circuit_get_rendezvous(tok2)); + /* Change its cookie. This never happens in Tor per se, but hey. */ + c3->base_.purpose = CIRCUIT_PURPOSE_INTRO_POINT; + circuit_set_intro_point_digest(c3, tok3); + + tt_ptr_op(NULL, ==, circuit_get_rendezvous(tok2)); + tt_ptr_op(c3, ==, circuit_get_intro_point(tok3)); + + /* Now replace c3 with c4. */ + c4->base_.purpose = CIRCUIT_PURPOSE_INTRO_POINT; + circuit_set_intro_point_digest(c4, tok3); + + tt_ptr_op(c4, ==, circuit_get_intro_point(tok3)); + + tt_ptr_op(c3->rendinfo, ==, NULL); + tt_ptr_op(c4->rendinfo, !=, NULL); + test_mem_op(c4->rendinfo, ==, tok3, REND_TOKEN_LEN); + + done: + circuit_free(TO_CIRCUIT(c1)); + circuit_free(TO_CIRCUIT(c2)); + circuit_free(TO_CIRCUIT(c3)); + circuit_free(TO_CIRCUIT(c4)); +} + struct testcase_t circuitlist_tests[] = { { "maps", test_clist_maps, TT_FORK, NULL, NULL }, + { "rend_token_maps", test_rend_token_maps, TT_FORK, NULL, NULL }, END_OF_TESTCASES };

1 0

[tor/master] Protocol_Warn when a rendezvous cookie is used twice.
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit 8f16a77d6ae6bfc8f7983decf8269ae70ca1cc5e Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 4 12:17:16 2014 -0400 Protocol_Warn when a rendezvous cookie is used twice. --- src/or/circuitlist.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index a5b43f7..065f980 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1343,8 +1343,14 @@ circuit_set_rend_token(or_circuit_t *circ, int is_rend_circ, if (found_circ) { tor_assert(found_circ != circ); circuit_clear_rend_token(found_circ); - if (! found_circ->base_.marked_for_close) + if (! found_circ->base_.marked_for_close) { circuit_mark_for_close(TO_CIRCUIT(found_circ), END_CIRC_REASON_FINISHED); + if (is_rend_circ) { + log_fn(LOG_PROTOCOL_WARN, LD_REND, + "Duplicate rendezvous cookie (%s...) used on two circuits", + hex_str((const char*)token, 4)); /* only log first 4 chars */ + } + } } /* Now set up the rendinfo */

1 0

[tor/master] Merge remote-tracking branch 'public/bug9841_024_v2' into bug9841_025
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit 1bb6e3b503bfe4f7f5a8ee707433b6f9973e4dea Merge: fb59592 09dbcf3 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 4 12:05:51 2014 -0400 Merge remote-tracking branch 'public/bug9841_024_v2' into bug9841_025 src/or/circuitlist.c | 5 +++++ 1 file changed, 5 insertions(+)

1 0

[tor/master] Test for circuit_set_rend_token(., ., NULL)
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit d22ce149c82c257398555d35cf76cab4973a8cf9 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Apr 4 12:06:54 2014 -0400 Test for circuit_set_rend_token(.,.,NULL) --- src/test/test_circuitlist.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/test_circuitlist.c b/src/test/test_circuitlist.c index ee62cd2..2cfe556 100644 --- a/src/test/test_circuitlist.c +++ b/src/test/test_circuitlist.c @@ -238,6 +238,11 @@ test_rend_token_maps(void *arg) tt_ptr_op(c4->rendinfo, !=, NULL); test_mem_op(c4->rendinfo, ==, tok3, REND_TOKEN_LEN); + /* Now clear c4's cookie. */ + circuit_set_intro_point_digest(c4, NULL); + tt_ptr_op(c4->rendinfo, ==, NULL); + tt_ptr_op(NULL, ==, circuit_get_intro_point(tok3)); + done: circuit_free(TO_CIRCUIT(c1)); circuit_free(TO_CIRCUIT(c2));

1 0

[tor/master] Merge remote-tracking branch 'public/bug9841_025'
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit fffc59b0e941f12e1588e0604c8295ecc0679150 Merge: f0bce2d 8f16a77 Author: Nick Mathewson <nickm(a)torproject.org> Date: Tue Apr 8 12:06:03 2014 -0400 Merge remote-tracking branch 'public/bug9841_025' changes/bug9841 | 7 ++ src/or/circuitlist.c | 164 ++++++++++++++++++++++++++++++++++++++----- src/or/circuitlist.h | 6 +- src/or/or.h | 33 +++++---- src/or/rendmid.c | 14 ++-- src/test/test_circuitlist.c | 90 ++++++++++++++++++++++++ 6 files changed, 275 insertions(+), 39 deletions(-)

1 0

[tor/master] Faster circuit_get_by_rend_token_and_purpose()
by nickm＠torproject.org 08 Apr '14

08 Apr '14

commit 8b9a2cb68b290e550695124d7ef0511225b451d5 Author: Nick Mathewson <nickm(a)torproject.org> Date: Fri Sep 27 11:54:36 2013 -0400 Faster circuit_get_by_rend_token_and_purpose() On busy servers, this function takes up something like 3-7% in different profiles, and gets invoked every time we need to participate as the midpoint in a hidden service. So maybe walking through a linked list of all the circuits here wasn't a good idea. --- changes/bug9841 | 7 +++ src/or/circuitlist.c | 148 +++++++++++++++++++++++++++++++++++++++++++++----- src/or/circuitlist.h | 2 + src/or/or.h | 34 +++++++----- src/or/rendmid.c | 6 +- 5 files changed, 166 insertions(+), 31 deletions(-) diff --git a/changes/bug9841 b/changes/bug9841 new file mode 100644 index 0000000..8b40dda0 --- /dev/null +++ b/changes/bug9841 @@ -0,0 +1,7 @@ + o Minor features (performance): + + - Faster server-side lookups of rendezvous and introduction point + circuits by using hashtables instead of linear searches over all + the circuits. These functions previously accounted between 3 and + 7% of CPU usage on some busy relays. + diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index b0e24a5..8ab673c 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -31,6 +31,7 @@ #include "rephist.h" #include "routerlist.h" #include "routerset.h" + #include "ht.h" /********* START VARIABLES **********/ @@ -45,6 +46,9 @@ static void circuit_free(circuit_t *circ); static void circuit_free_cpath(crypt_path_t *cpath); static void circuit_free_cpath_node(crypt_path_t *victim); static void cpath_ref_decref(crypt_path_reference_t *cpath_ref); +//static void circuit_set_rend_token(or_circuit_t *circ, int is_rend_circ, +// const uint8_t *token); +static void circuit_clear_rend_token(or_circuit_t *circ); /********* END VARIABLES ************/ @@ -672,6 +676,8 @@ circuit_free(circuit_t *circ) crypto_cipher_free(ocirc->n_crypto); crypto_digest_free(ocirc->n_digest); + circuit_clear_rend_token(ocirc); + if (ocirc->rend_splice) { or_circuit_t *other = ocirc->rend_splice; tor_assert(other->base_.magic == OR_CIRCUIT_MAGIC); @@ -1129,21 +1135,118 @@ circuit_get_next_by_pk_and_purpose(origin_circuit_t *start, return NULL; } -/** Return the first OR circuit in the global list whose purpose is - * purpose, and whose rend_token is the len-byte - * token. */ +/** Map from rendezvous cookie to or_circuit_t */ +static digestmap_t *rend_cookie_map = NULL; + +/** Map from introduction point digest to or_circuit_t */ +static digestmap_t *intro_digest_map = NULL; + +/** Return the OR circuit whose purpose is purpose, and whose + * rend_token is the REND_TOKEN_LEN-byte token. If is_rend_circ, + * look for rendezvous point circuits; otherwise look for introduction point + * circuits. */ static or_circuit_t * -circuit_get_by_rend_token_and_purpose(uint8_t purpose, const char *token, - size_t len) +circuit_get_by_rend_token_and_purpose(uint8_t purpose, int is_rend_circ, + const char *token) { - circuit_t *circ; - for (circ = global_circuitlist; circ; circ = circ->next) { - if (! circ->marked_for_close && - circ->purpose == purpose && - tor_memeq(TO_OR_CIRCUIT(circ)->rend_token, token, len)) - return TO_OR_CIRCUIT(circ); + or_circuit_t *circ; + digestmap_t *map = is_rend_circ ? rend_cookie_map : intro_digest_map; + + if (!map) + return NULL; + + circ = digestmap_get(map, token); + if (!circ || + circ->base_.purpose != purpose || + circ->base_.marked_for_close) + return NULL; + + if (!circ->rendinfo || + ! bool_eq(circ->rendinfo->is_rend_circ, is_rend_circ) || + tor_memneq(circ->rendinfo->rend_token, token, REND_TOKEN_LEN)) { + char *t = tor_strdup(hex_str(token, REND_TOKEN_LEN)); + log_warn(LD_BUG, "Wanted a circuit with %s:%d, but lookup returned %s:%d", + safe_str(t), is_rend_circ, + safe_str(hex_str(circ->rendinfo->rend_token, REND_TOKEN_LEN)), + (int)circ->rendinfo->is_rend_circ); + tor_free(t); + return NULL; } - return NULL; + + return circ; +} + +/** Clear the rendezvous cookie or introduction point key digest that's + * configured on circ, if any, and remove it from any such maps. */ +static void +circuit_clear_rend_token(or_circuit_t *circ) +{ + or_circuit_t *found_circ; + digestmap_t *map; + + if (!circ || !circ->rendinfo) + return; + + map = circ->rendinfo->is_rend_circ ? rend_cookie_map : intro_digest_map; + + if (!map) { + log_warn(LD_BUG, "Tried to clear rend token on circuit, but found no map"); + return; + } + + found_circ = digestmap_get(map, circ->rendinfo->rend_token); + if (found_circ == circ) { + /* Great, this is the right one. */ + digestmap_remove(map, circ->rendinfo->rend_token); + } else if (found_circ) { + log_warn(LD_BUG, "Tried to clear rend token on circuit, but " + "it was already replaced in the map."); + } else { + log_warn(LD_BUG, "Tried to clear rend token on circuit, but " + "it not in the map at all."); + } + + tor_free(circ->rendinfo); /* Sets it to NULL too */ +} + +/** Set the rendezvous cookie (if is_rend_circ), or the introduction point + * digest (if ! is_rend_circ) of circ to the REND_TOKEN_LEN-byte value + * in token, and add it to the appropriate map. If it previously had a + * token, clear it. If another circuit previously had the same + * cookie/intro-digest, mark that circuit and remove it from the map. */ +static void +circuit_set_rend_token(or_circuit_t *circ, int is_rend_circ, + const uint8_t *token) +{ + digestmap_t **map_p, *map; + or_circuit_t *found_circ; + + /* Find the right map, creating it as needed */ + map_p = is_rend_circ ? &rend_cookie_map : &intro_digest_map; + + if (!*map_p) + *map_p = digestmap_new(); + + map = *map_p; + + /* If this circuit already has a token, we need to remove that. */ + if (circ->rendinfo) + circuit_clear_rend_token(circ); + + found_circ = digestmap_get(map, (const char *)token); + if (found_circ) { + tor_assert(found_circ != circ); + circuit_clear_rend_token(found_circ); + if (! found_circ->base_.marked_for_close) + circuit_mark_for_close(TO_CIRCUIT(found_circ), END_CIRC_REASON_FINISHED); + } + + /* Now set up the rendinfo */ + circ->rendinfo = tor_malloc(sizeof(*circ->rendinfo)); + memcpy(circ->rendinfo->rend_token, token, REND_TOKEN_LEN); + circ->rendinfo->is_rend_circ = is_rend_circ ? 1 : 0; + + digestmap_set(map, (const char *)token, circ); } /** Return the circuit waiting for a rendezvous with the provided cookie. @@ -1154,7 +1257,7 @@ circuit_get_rendezvous(const char *cookie) { return circuit_get_by_rend_token_and_purpose( CIRCUIT_PURPOSE_REND_POINT_WAITING, - cookie, REND_COOKIE_LEN); + 1, cookie); } /** Return the circuit waiting for intro cells of the given digest. @@ -1164,8 +1267,23 @@ or_circuit_t * circuit_get_intro_point(const char *digest) { return circuit_get_by_rend_token_and_purpose( - CIRCUIT_PURPOSE_INTRO_POINT, digest, - DIGEST_LEN); + CIRCUIT_PURPOSE_INTRO_POINT, 0, digest); +} + +/** Set the rendezvous cookie of circ to cookie. If another + * circuit previously had that cookie, mark it. */ +void +circuit_set_rendezvous_cookie(or_circuit_t *circ, const uint8_t *cookie) +{ + circuit_set_rend_token(circ, 1, cookie); +} + +/** Set the intro point key digest of circ to cookie. If another + * circuit previously had that intro point digest, mark it. */ +void +circuit_set_intro_point_digest(or_circuit_t *circ, const uint8_t *digest) +{ + circuit_set_rend_token(circ, 0, digest); } /** Return a circuit that is open, is CIRCUIT_PURPOSE_C_GENERAL, diff --git a/src/or/circuitlist.h b/src/or/circuitlist.h index 874f68c..d8c107f 100644 --- a/src/or/circuitlist.h +++ b/src/or/circuitlist.h @@ -43,6 +43,8 @@ origin_circuit_t *circuit_get_next_by_pk_and_purpose(origin_circuit_t *start, const char *digest, uint8_t purpose); or_circuit_t *circuit_get_rendezvous(const char *cookie); or_circuit_t *circuit_get_intro_point(const char *digest); +void circuit_set_rendezvous_cookie(or_circuit_t *circ, const uint8_t *cookie); +void circuit_set_intro_point_digest(or_circuit_t *circ, const uint8_t *digest); origin_circuit_t *circuit_find_to_cannibalize(uint8_t purpose, extend_info_t *info, int flags); void circuit_mark_all_unused_circs(void); diff --git a/src/or/or.h b/src/or/or.h index 5318b0f..510b8ea 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -99,6 +99,7 @@ #include "ht.h" #include "replaycache.h" #include "crypto_curve25519.h" +#include "tor_queue.h" /* These signals are defined to help handle_control_signal work. */ @@ -3152,20 +3153,8 @@ typedef struct or_circuit_t { * is not marked for close. */ struct or_circuit_t *rend_splice; -#if REND_COOKIE_LEN >= DIGEST_LEN -#define REND_TOKEN_LEN REND_COOKIE_LEN -#else -#define REND_TOKEN_LEN DIGEST_LEN -#endif + struct or_circuit_rendinfo_s *rendinfo; - /** A hash of location-hidden service's PK if purpose is INTRO_POINT, or a - * rendezvous cookie if purpose is REND_POINT_WAITING. Filled with zeroes - * otherwise. - * ???? move to a subtype or adjunct structure? Wastes 20 bytes. -NM - */ - char rend_token[REND_TOKEN_LEN]; - - /* ???? move to a subtype or adjunct structure? Wastes 20 bytes -NM */ /** Stores KH for the handshake. */ char rend_circ_nonce[DIGEST_LEN];/* KH in tor-spec.txt */ @@ -3186,6 +3175,25 @@ typedef struct or_circuit_t { uint64_t total_cell_waiting_time; } or_circuit_t; +typedef struct or_circuit_rendinfo_s { + +#if REND_COOKIE_LEN != DIGEST_LEN +#error "The REND_TOKEN_LEN macro assumes REND_COOKIE_LEN == DIGEST_LEN" +#endif +#define REND_TOKEN_LEN DIGEST_LEN + + /** A hash of location-hidden service's PK if purpose is INTRO_POINT, or a + * rendezvous cookie if purpose is REND_POINT_WAITING. Filled with zeroes + * otherwise. + */ + char rend_token[REND_TOKEN_LEN]; + + /** True if this is a rendezvous point circuit; false if this is an + * introduction point. */ + unsigned is_rend_circ; + +} or_circuit_rendinfo_t; + /** Convert a circuit subtype to a circuit_t. */ #define TO_CIRCUIT(x) (&((x)->base_)) diff --git a/src/or/rendmid.c b/src/or/rendmid.c index 1bd11f6..8090bf2 100644 --- a/src/or/rendmid.c +++ b/src/or/rendmid.c @@ -111,7 +111,7 @@ rend_mid_establish_intro(or_circuit_t *circ, const uint8_t *request, /* Now, set up this circuit. */ circuit_change_purpose(TO_CIRCUIT(circ), CIRCUIT_PURPOSE_INTRO_POINT); - memcpy(circ->rend_token, pk_digest, DIGEST_LEN); + circuit_set_intro_point_digest(circ, (uint8_t *)pk_digest); log_info(LD_REND, "Established introduction point on circuit %u for service %s", @@ -251,7 +251,7 @@ rend_mid_establish_rendezvous(or_circuit_t *circ, const uint8_t *request, } circuit_change_purpose(TO_CIRCUIT(circ), CIRCUIT_PURPOSE_REND_POINT_WAITING); - memcpy(circ->rend_token, request, REND_COOKIE_LEN); + circuit_set_rendezvous_cookie(circ, request); base16_encode(hexid,9,(char*)request,4); @@ -327,7 +327,7 @@ rend_mid_rendezvous(or_circuit_t *circ, const uint8_t *request, circuit_change_purpose(TO_CIRCUIT(circ), CIRCUIT_PURPOSE_REND_ESTABLISHED); circuit_change_purpose(TO_CIRCUIT(rend_circ), CIRCUIT_PURPOSE_REND_ESTABLISHED); - memset(circ->rend_token, 0, REND_COOKIE_LEN); + circuit_set_rendezvous_cookie(circ, NULL); rend_circ->rend_splice = circ; circ->rend_splice = rend_circ;

1 0