From 57890abf2c7919eddfec36178b1136cd44ffe883 Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Thu, 4 Jun 2020 11:49:09 -0400 Subject: Remove two unneeded tests Test 1 is a duplicate of test_size() later in the file. Inexplicably, this test does not work on macOS, whereas test_size() does. Test 2 is problematic for two reasons. First, it always fails with an invalid checksum, which is probably not what was intended. Second, it's not defined at this layer what the behavior should be. Hypothetically, if this test was fixed so that it gave messages with valid checksums, then the message would pass successfully thought the network layer and fail only in the processing layer. A priori the network layer has no idea what the size of a message "actually" is. The "Why does behavior change at 78 bytes" is because of the following: print(len(node.p2p.build_message(msg))) # 125 => Payload size = 125 - 24 = 101 If we take 77 bytes, then there are 101 - 77 = 24 left That's exactly the size of a header So, bitcoind deserializes the header and rejects it for some other reason (Almost always an invalid size (too large)) But, if we take 78 bytes, then there are 101 - 78 = 23 left That's not enough to fill a header, so the socket stays open waiting for more data. That's why we sometimes have to push additional data in order for the peer to disconnect. Additionally, both of these tests use the "conn" variable. For fun, go look at where it's declared. (Hint: test_large_inv(). Don't we all love python's idea of scope?) --- test/functional/p2p_invalid_messages.py | 86 --------------------------------- 1 file changed, 86 deletions(-) (limited to 'test') diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 81302374c9..244a5a136d 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -5,7 +5,6 @@ """Test node responses to invalid network messages.""" import asyncio import struct -import sys from test_framework.messages import ( CBlockHeader, @@ -50,11 +49,6 @@ class InvalidMessagesTest(BitcoinTestFramework): . Test msg header 0. Send a bunch of large (4MB) messages of an unrecognized type. Check to see that it isn't an effective DoS against the node. - - 1. Send an oversized (4MB+) message and check that we're disconnected. - - 2. Send a few messages with an incorrect data size in the header, ensure the - messages are ignored. """ self.test_magic_bytes() self.test_checksum() @@ -95,66 +89,6 @@ class InvalidMessagesTest(BitcoinTestFramework): node.p2p.sync_with_ping(timeout=400) assert node.p2p.is_connected - # - # 1. - # - # Send an oversized message, ensure we're disconnected. - # - # Under macOS this test is skipped due to an unexpected error code - # returned from the closing socket which python/asyncio does not - # yet know how to handle. - # - if sys.platform != 'darwin': - msg_over_size = msg_unrecognized(str_data="b" * (valid_data_limit + 1)) - assert len(msg_over_size.serialize()) == (msg_limit + 1) - - # An unknown message type (or *any* message type) over - # MAX_PROTOCOL_MESSAGE_LENGTH should result in a disconnect. - node.p2p.send_message(msg_over_size) - node.p2p.wait_for_disconnect(timeout=4) - - node.disconnect_p2ps() - conn = node.add_p2p_connection(P2PDataStore()) - conn.wait_for_verack() - else: - self.log.info("Skipping test p2p_invalid_messages/1 (oversized message) under macOS") - - # - # 2. - # - # Send messages with an incorrect data size in the header. - # - actual_size = 100 - msg = msg_unrecognized(str_data="b" * actual_size) - - # TODO: handle larger-than cases. I haven't been able to pin down what behavior to expect. - for wrong_size in (2, 77, 78, 79): - self.log.info("Sending a message with incorrect size of {}".format(wrong_size)) - - # Unmodified message should submit okay. - node.p2p.send_and_ping(msg) - - # A message lying about its data size results in a disconnect when the incorrect - # data size is less than the actual size. - # - # TODO: why does behavior change at 78 bytes? - # - node.p2p.send_raw_message(self._tweak_msg_data_size(msg, wrong_size)) - - # For some reason unknown to me, we sometimes have to push additional data to the - # peer in order for it to realize a disconnect. - try: - node.p2p.send_message(msg_ping(nonce=123123)) - except IOError: - pass - - node.p2p.wait_for_disconnect(timeout=10) - node.disconnect_p2ps() - node.add_p2p_connection(P2PDataStore()) - - # Node is still up. - conn = node.add_p2p_connection(P2PDataStore()) - def test_magic_bytes(self): conn = self.nodes[0].add_p2p_connection(P2PDataStore()) @@ -225,26 +159,6 @@ class InvalidMessagesTest(BitcoinTestFramework): conn.send_and_ping(msg) self.nodes[0].disconnect_p2ps() - def _tweak_msg_data_size(self, message, wrong_size): - """ - Return a raw message based on another message but with an incorrect data size in - the message header. - """ - raw_msg = self.node.p2p.build_message(message) - - bad_size_bytes = struct.pack(" Date: Thu, 4 Jun 2020 11:52:23 -0400 Subject: Move size limits to module-global As well, this renames those variables to match PEP8 and this clears up the comment relating to VALID_DATA_LIMIT. Admittedly, this commit is mainly to make the following ones cleaner. --- test/functional/p2p_invalid_messages.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'test') diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 244a5a136d..38e823df47 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -23,6 +23,8 @@ from test_framework.mininode import ( ) from test_framework.test_framework import BitcoinTestFramework +MSG_LIMIT = 4 * 1000 * 1000 # 4MB, per MAX_PROTOCOL_MESSAGE_LENGTH +VALID_DATA_LIMIT = MSG_LIMIT - 5 # Account for the 5-byte length prefix class msg_unrecognized: """Nonsensical message. Modeled after similar types in test_framework.messages.""" @@ -61,8 +63,6 @@ class InvalidMessagesTest(BitcoinTestFramework): node.add_p2p_connection(P2PDataStore()) conn2 = node.add_p2p_connection(P2PDataStore()) - msg_limit = 4 * 1000 * 1000 # 4MB, per MAX_PROTOCOL_MESSAGE_LENGTH - valid_data_limit = msg_limit - 5 # Account for the 4-byte length prefix # # 0. @@ -70,8 +70,8 @@ class InvalidMessagesTest(BitcoinTestFramework): # Send as large a message as is valid, ensure we aren't disconnected but # also can't exhaust resources. # - msg_at_size = msg_unrecognized(str_data="b" * valid_data_limit) - assert len(msg_at_size.serialize()) == msg_limit + msg_at_size = msg_unrecognized(str_data="b" * VALID_DATA_LIMIT) + assert len(msg_at_size.serialize()) == MSG_LIMIT self.log.info("Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...") -- cgit v1.2.3 From 5c4648d17ba18e4194959963994cc6b37053f127 Mon Sep 17 00:00:00 2001 From: Troy Giorshev Date: Thu, 4 Jun 2020 13:06:17 -0400 Subject: Fix "invalid message size" test This test originally made a message with an invalid stated length, and an invalid checksum. This was because only the header was changed, but the checksum stayed the same. This was fine for now because we check the header first to see if it has a valid stated size, and we disconnect if it does not, so we never end up checking for the checksum. If this behavior was to change, this test would become a problem. (Indeed I discovered this when playing around with this behavior). By instead creating a message with an oversized payload from the start, we create a message with an invalid stated length but a valid checksum, as intended. Additionally, this takes advantage to the newly module-global VALID_DATA_LIMIT as opposed to the magic 0x02000000. Yes, 4MB < 32MiB, but at the moment when receiving a message we check both, so this makes the test tighter. --- test/functional/p2p_invalid_messages.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) (limited to 'test') diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 38e823df47..b15d2ffd2e 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -4,7 +4,6 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test node responses to invalid network messages.""" import asyncio -import struct from test_framework.messages import ( CBlockHeader, @@ -123,13 +122,9 @@ class InvalidMessagesTest(BitcoinTestFramework): def test_size(self): conn = self.nodes[0].add_p2p_connection(P2PDataStore()) with self.nodes[0].assert_debug_log(['']): - msg = conn.build_message(msg_unrecognized(str_data="d")) - cut_len = ( - 4 + # magic - 12 # msgtype - ) - # modify len to MAX_SIZE + 1 - msg = msg[:cut_len] + struct.pack(" Date: Thu, 4 Jun 2020 13:06:29 -0400 Subject: Refactor resource exhaustion test This is a simple refactor of the specified test. It is now brought in line with the rest of the tests in the module. This should make things easier to debug, as all of the tests are now grouped together at the top. --- test/functional/p2p_invalid_messages.py | 60 +++++++++++++-------------------- 1 file changed, 24 insertions(+), 36 deletions(-) (limited to 'test') diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index b15d2ffd2e..771fac08ca 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -46,47 +46,12 @@ class InvalidMessagesTest(BitcoinTestFramework): self.setup_clean_chain = True def run_test(self): - """ - . Test msg header - 0. Send a bunch of large (4MB) messages of an unrecognized type. Check to see - that it isn't an effective DoS against the node. - """ self.test_magic_bytes() self.test_checksum() self.test_size() self.test_msgtype() self.test_large_inv() - - node = self.nodes[0] - self.node = node - node.add_p2p_connection(P2PDataStore()) - conn2 = node.add_p2p_connection(P2PDataStore()) - - - # - # 0. - # - # Send as large a message as is valid, ensure we aren't disconnected but - # also can't exhaust resources. - # - msg_at_size = msg_unrecognized(str_data="b" * VALID_DATA_LIMIT) - assert len(msg_at_size.serialize()) == MSG_LIMIT - - self.log.info("Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...") - - # Run a bunch of times to test for memory exhaustion. - for _ in range(80): - node.p2p.send_message(msg_at_size) - - # Check that, even though the node is being hammered by nonsense from one - # connection, it can still service other peers in a timely way. - for _ in range(20): - conn2.sync_with_ping(timeout=2) - - # Peer 1, despite serving up a bunch of nonsense, should still be connected. - self.log.info("Waiting for node to drop junk messages.") - node.p2p.sync_with_ping(timeout=400) - assert node.p2p.is_connected + self.test_resource_exhaustion() def test_magic_bytes(self): conn = self.nodes[0].add_p2p_connection(P2PDataStore()) @@ -154,6 +119,29 @@ class InvalidMessagesTest(BitcoinTestFramework): conn.send_and_ping(msg) self.nodes[0].disconnect_p2ps() + def test_resource_exhaustion(self): + conn = self.nodes[0].add_p2p_connection(P2PDataStore()) + conn2 = self.nodes[0].add_p2p_connection(P2PDataStore()) + msg_at_size = msg_unrecognized(str_data="b" * VALID_DATA_LIMIT) + assert len(msg_at_size.serialize()) == MSG_LIMIT + + self.log.info("Sending a bunch of large, junk messages to test memory exhaustion. May take a bit...") + + # Run a bunch of times to test for memory exhaustion. + for _ in range(80): + conn.send_message(msg_at_size) + + # Check that, even though the node is being hammered by nonsense from one + # connection, it can still service other peers in a timely way. + for _ in range(20): + conn2.sync_with_ping(timeout=2) + + # Peer 1, despite being served up a bunch of nonsense, should still be connected. + self.log.info("Waiting for node to drop junk messages.") + conn.sync_with_ping(timeout=400) + assert conn.is_connected + self.nodes[0].disconnect_p2ps() + if __name__ == '__main__': InvalidMessagesTest().main() -- cgit v1.2.3