commit 05b106bb575f669487fc376a29b7710d2addb2b0 Author: Damian Johnson atagar@torproject.org Date: Wed Oct 12 09:17:59 2011 -0700

Protocol error tests and fix

Expanding the ControlMessage unit tests to cover causes of protocol errors, and fixing an issue that this revealed where it was possable for a malformed line prefix to go undetected (if, say, a dropped character caused a valid line divider to fall into that place). --- stem/types.py | 5 ++ test/unit/message.py | 125 +++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 113 insertions(+), 17 deletions(-)

diff --git a/stem/types.py b/stem/types.py index d37f9a1..730accb 100644 --- a/stem/types.py +++ b/stem/types.py @@ -60,6 +60,9 @@ def read_message(control_file): if len(line) < 4: log.log(log.WARN, "ProtocolError: line too short (%s)" % line) raise ProtocolError("Badly formatted reply line: too short") + elif not re.match(r'^[a-zA-Z0-9]{3}[-+ ]', line): + log.log(log.WARN, "ProtocolError: malformed status code/divider (%s)" % line) + raise ProtocolError("Badly formatted reply line: beginning is malformed") elif not line.endswith("\r\n"): log.log(log.WARN, "ProtocolError: no CRLF linebreak (%s)" % line) raise ProtocolError("All lines should end with CRLF") @@ -105,6 +108,8 @@ def read_message(control_file):

parsed_content.append((status_code, divider, content)) else: + # this should never be reached due to the prefix regex, but might as well + # be safe... log.log(log.WARN, "ProtocolError: unrecognized divider type (%s)" % line) raise ProtocolError("Unrecognized type '%s': %s" % (divider, line))

diff --git a/test/unit/message.py b/test/unit/message.py index dde2c9c..e1fb622 100644 --- a/test/unit/message.py +++ b/test/unit/message.py @@ -6,20 +6,27 @@ import StringIO import unittest import stem.types

-GETINFO_VERSION_REPLY = """250-version=0.2.2.23-alpha (git-b85eb949b528f4d7)\r -250 OK\r -""" +OK_REPLY = "250 OK\r\n"

-GETINFO_INFONAMES_REPLY = """250+info/names=\r -accounting/bytes -- Number of bytes read/written so far in the accounting interval.\r -accounting/bytes-left -- Number of bytes left to write/read so far in the accounting interval.\r -accounting/enabled -- Is accounting currently enabled?\r -accounting/hibernating -- Are we hibernating or awake?\r -stream-status -- List of current streams.\r -version -- The current version of Tor.\r -.\r -250 OK\r -""" +EVENT_BW = "650 BW 32326 2856\r\n" +EVENT_CIRC_TIMEOUT = "650 CIRC 5 FAILED PURPOSE=GENERAL REASON=TIMEOUT\r\n" +EVENT_CIRC_LAUNCHED = "650 CIRC 9 LAUNCHED PURPOSE=GENERAL\r\n" +EVENT_CIRC_EXTENDED = "650 CIRC 5 EXTENDED $A200F527C82C59A25CCA44884B49D3D65B122652=faktor PURPOSE=MEASURE_TIMEOUT\r\n" + +GETINFO_VERSION = """250-version=0.2.2.23-alpha (git-b85eb949b528f4d7) +250 OK +""".replace("\n", "\r\n") + +GETINFO_INFONAMES = """250+info/names= +accounting/bytes -- Number of bytes read/written so far in the accounting interval. +accounting/bytes-left -- Number of bytes left to write/read so far in the accounting interval. +accounting/enabled -- Is accounting currently enabled? +accounting/hibernating -- Are we hibernating or awake? +stream-status -- List of current streams. +version -- The current version of Tor. +. +250 OK +""".replace("\n", "\r\n")

class TestMessageFunctions(unittest.TestCase): """ @@ -27,13 +34,47 @@ class TestMessageFunctions(unittest.TestCase): StringIO to make 'files' to mock socket input. """

- def test_getinfo_results(self): + def test_ok_response(self): + """ + Checks the basic 'OK' response that we get for most commands. + """ + + message = self.assert_message_parses(OK_REPLY) + self.assertEquals("OK", str(message)) + + contents = message.content() + self.assertEquals(1, len(contents)) + self.assertEquals(("250", " ", "OK"), contents[0]) + + def test_event_response(self): + """ + Checks parsing of actual events. + """ + + # BW event + message = self.assert_message_parses(EVENT_BW) + self.assertEquals("BW 32326 2856", str(message)) + + contents = message.content() + self.assertEquals(1, len(contents)) + self.assertEquals(("650", " ", "BW 32326 2856"), contents[0]) + + # few types of CIRC events + for circ_content in (EVENT_CIRC_TIMEOUT, EVENT_CIRC_LAUNCHED, EVENT_CIRC_EXTENDED): + message = self.assert_message_parses(circ_content) + self.assertEquals(circ_content[4:-2], str(message)) + + contents = message.content() + self.assertEquals(1, len(contents)) + self.assertEquals(("650", " ", str(message)), contents[0]) + + def test_getinfo_response(self): """ - Checks parsing against some actual GETINFO responses. + Checks parsing of actual GETINFO responses. """

# GETINFO version (basic single-line results) - message = self.assert_message_parses(GETINFO_VERSION_REPLY) + message = self.assert_message_parses(GETINFO_VERSION) self.assertEquals(2, len(list(message))) self.assertEquals(2, len(str(message).split("\n")))

@@ -44,7 +85,7 @@ class TestMessageFunctions(unittest.TestCase): self.assertEquals(("250", " ", "OK"), contents[1])

# GETINFO info/names (data entry) - message = self.assert_message_parses(GETINFO_INFONAMES_REPLY) + message = self.assert_message_parses(GETINFO_INFONAMES) self.assertEquals(2, len(list(message))) self.assertEquals(8, len(str(message).split("\n")))

@@ -56,6 +97,56 @@ class TestMessageFunctions(unittest.TestCase): self.assertEquals(("250", "+", "info/names="), first_entry) self.assertEquals(("250", " ", "OK"), contents[1])

+ def test_no_crlf(self): + """ + Checks that we get a ProtocolError when we don't have both a carrage + returna and newline for line endings. This doesn't really check for + newlines (since that's what readline would break on), but not the end of + the world. + """ + + # Replaces each of the CRLF entries with just LF, confirming that this + # causes a parsing error. This should test line endings for both data + # entry parsing and non-data. + + infonames_lines = [line + "\n" for line in GETINFO_INFONAMES.split("\n")[:-1]] + + for i in range(len(infonames_lines)): + # replace the CRLF for the line + infonames_lines[i] = infonames_lines[i].rstrip("\r\n") + "\n" + test_socket_file = StringIO.StringIO("".join(infonames_lines)) + self.assertRaises(stem.types.ProtocolError, stem.types.read_message, test_socket_file) + + # puts the CRLF back + infonames_lines[i] = infonames_lines[i].rstrip("\n") + "\r\n" + + # sanity check the above test isn't broken due to leaving infonames_lines + # with invalid data + + self.assert_message_parses("".join(infonames_lines)) + + def test_malformed_prefix(self): + """ + Checks parsing for responses where the header is missing a digit or divider. + """ + + for i in range(len(EVENT_BW)): + # makes test input with that character missing or replaced + removal_test_input = EVENT_BW[:i] + EVENT_BW[i + 1:] + replacement_test_input = EVENT_BW[:i] + "#" + EVENT_BW[i + 1:] + + if i < 4 or i >= (len(EVENT_BW) - 2): + # dropping the character should cause an error if... + # - this is part of the message prefix + # - this is disrupting the line ending + + self.assertRaises(stem.types.ProtocolError, stem.types.read_message, StringIO.StringIO(removal_test_input)) + self.assertRaises(stem.types.ProtocolError, stem.types.read_message, StringIO.StringIO(replacement_test_input)) + else: + # otherwise the data will be malformed, but this goes undetected + self.assert_message_parses(removal_test_input) + self.assert_message_parses(replacement_test_input) + def assert_message_parses(self, controller_reply): """ Performs some basic sanity checks that a reply mirrors its parsed result.