commit 355e474b3d62dceb09699f1be27b5b27b925e7fa Author: Damian Johnson <atagar@torproject.org> Date: Mon Sep 10 09:12:19 2012 -0700 Parsing the params attribute Being a fair bit more anal about field validation, checking the following... * bounds on the value to be an 32 bit int * order of the values (keys should be ascending) * reject values like '+11' (the int() function accepts them) This also includes unit tests for these use cases and better error messaging. --- stem/descriptor/networkstatus.py | 50 ++++++++++++++-- test/unit/descriptor/networkstatus/document.py | 74 +++++++++++++++++++++++- 2 files changed, 117 insertions(+), 7 deletions(-) diff --git a/stem/descriptor/networkstatus.py b/stem/descriptor/networkstatus.py index 1efcf58..d43c1d0 100644 --- a/stem/descriptor/networkstatus.py +++ b/stem/descriptor/networkstatus.py @@ -196,7 +196,7 @@ class NetworkStatusDocument(stem.descriptor.Descriptor): :var list client_versions: list of recommended client tor versions :var list server_versions: list of recommended server tor versions :var list known_flags: **\*** list of known router flags - :var list params: dict of parameter(str) => value(int) mappings + :var list params: **\*** dict of parameter(str) => value(int) mappings :var list directory_authorities: **\*** list of DirectoryAuthority objects that have generated this document :var dict bandwidth_weights: **~** dict of weight(str) => value(int) mappings :var list directory_signatures: **\*** list of signatures this document has @@ -381,6 +381,49 @@ class NetworkStatusDocument(stem.descriptor.Descriptor): # simply fetches the entries, excluding empty strings self.known_flags = [entry for entry in value.split(" ") if entry] + elif keyword == "params": + # "params" [Parameters] + # Parameter ::= Keyword '=' Int32 + # Int32 ::= A decimal integer between -2147483648 and 2147483647. + # Parameters ::= Parameter | Parameters SP Parameter + + # should only appear in consensus-method 7 or later + if validate and not (self.consensus_method >= 7 or filter(lambda x: x >= 7, self.consensus_methods)): + raise ValueError("A network status document's 'params' line should only appear in consensus-method 7 or later") + + # skip if this is a blank line + if value == "": continue + + for entry in value.split(" "): + try: + if not '=' in entry: + raise ValueError("must only have 'key=value' entries") + + entry_key, entry_value = entry.split("=", 1) + + try: + # the int() function accepts things like '+123', but we don't want to + if entry_value.startswith('+'): + raise ValueError() + + entry_value = int(entry_value) + except ValueError: + raise ValueError("'%s' is a non-numeric value" % entry_value) + + if validate: + # check int32 range + if entry_value < -2147483648 or entry_value > 2147483647: + raise ValueError("values must be between -2147483648 and 2147483647") + + # parameters should be in ascending order by their key + for prior_key in self.params: + if prior_key > entry_key: + raise ValueError("parameters must be sorted by their key") + + self.params[entry_key] = entry_value + except ValueError, exc: + if not validate: continue + raise ValueError("Unable to parse network status document's 'params' line (%s): %s'" % (exc, line)) # doing this validation afterward so we know our 'is_consensus' and # 'is_vote' attributes @@ -407,13 +450,10 @@ class NetworkStatusDocument(stem.descriptor.Descriptor): _read_keyword_line("client-versions", content, False, True) _read_keyword_line("server-versions", content, False, True) _read_keyword_line("known-flags", content, False, True) + _read_keyword_line("params", content, False, True) vote = self.is_vote - read_keyword_line("params", True) - if self.params: - self.params = dict([(param.split("=")[0], int(param.split("=")[1])) for param in self.params.split(" ")]) - # authority section while _peek_keyword(content) == "dir-source": dirauth_data = _read_until_keywords(["dir-source", "r", "directory-footer", "directory-signature", "bandwidth-weights"], content, False, True) diff --git a/test/unit/descriptor/networkstatus/document.py b/test/unit/descriptor/networkstatus/document.py index 97e239a..01f08bf 100644 --- a/test/unit/descriptor/networkstatus/document.py +++ b/test/unit/descriptor/networkstatus/document.py @@ -116,7 +116,7 @@ class TestNetworkStatusDocument(unittest.TestCase): self.assertEqual([], document.client_versions) self.assertEqual([], document.server_versions) self.assertEqual(expected_known_flags, document.known_flags) - self.assertEqual(None, document.params) + self.assertEqual({}, document.params) self.assertEqual([], document.directory_authorities) self.assertEqual(None, document.bandwidth_weights) self.assertEqual([sig], document.directory_signatures) @@ -150,7 +150,7 @@ class TestNetworkStatusDocument(unittest.TestCase): self.assertEqual([], document.client_versions) self.assertEqual([], document.server_versions) self.assertEqual(expected_known_flags, document.known_flags) - self.assertEqual(None, document.params) + self.assertEqual({}, document.params) self.assertEqual([], document.directory_authorities) self.assertEqual({}, document.bandwidth_weights) self.assertEqual([sig], document.directory_signatures) @@ -441,4 +441,74 @@ class TestNetworkStatusDocument(unittest.TestCase): content = get_network_status_document({"known-flags": test_value}) document = NetworkStatusDocument(content) self.assertEquals(expected_value, document.known_flags) + + def test_params(self): + """ + General testing for the 'params' line, exercising the happy cases. + """ + + content = get_network_status_document({"params": "CircuitPriorityHalflifeMsec=30000 bwauthpid=1 unrecognized=-122"}) + document = NetworkStatusDocument(content) + self.assertEquals(30000, document.params["CircuitPriorityHalflifeMsec"]) + self.assertEquals(1, document.params["bwauthpid"]) + self.assertEquals(-122, document.params["unrecognized"]) + + # empty params line + content = get_network_status_document({"params": ""}) + document = NetworkStatusDocument(content) + self.assertEquals({}, document.params) + + def test_params_malformed(self): + """ + Parses a 'params' line with malformed content. + """ + + test_values = ( + "foo=", + "foo=abc", + "foo=+123", + "foo=12\tbar=12", + ) + + for test_value in test_values: + content = get_network_status_document({"params": test_value}) + self.assertRaises(ValueError, NetworkStatusDocument, content) + + document = NetworkStatusDocument(content, False) + self.assertEquals({}, document.params) + + def test_params_range(self): + """ + Check both the furthest valid 'params' values and values that are out of + bounds. + """ + + test_values = ( + ("foo=2147483648", {"foo": 2147483648}, False), + ("foo=-2147483649", {"foo": -2147483649}, False), + ("foo=2147483647", {"foo": 2147483647}, True), + ("foo=-2147483648", {"foo": -2147483648}, True), + ) + + for test_value, expected_value, is_ok in test_values: + content = get_network_status_document({"params": test_value}) + + if is_ok: + document = NetworkStatusDocument(content) + else: + self.assertRaises(ValueError, NetworkStatusDocument, content) + document = NetworkStatusDocument(content, False) + + self.assertEquals(expected_value, document.params) + + def test_params_misordered(self): + """ + Check that the 'params' line is rejected if out of order. + """ + + content = get_network_status_document({"params": "unrecognized=-122 bwauthpid=1"}) + self.assertRaises(ValueError, NetworkStatusDocument, content) + + document = NetworkStatusDocument(content, False) + self.assertEquals({"unrecognized": -122, "bwauthpid": 1}, document.params)