test: Check that peer is disconnected for bad buffered message #19302

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2006-badBuffer changing 1 files +18 −3

MarcoFalke commented at 11:24 AM on June 17, 2020: member
Currently test coverage is non-deterministic, which makes it hard to compare coverage between commits.

Fix one instance of non-determinism in net by adding an explicit test for this case:
```
src/net.cpp:    // if header incomplete, exit
```
MarcoFalke commented at 11:26 AM on June 17, 2020: member

Can be tested by adding an assert(false) to the line that I claim to add coverage to and observing a deterministic crash when running the new test and no crash when running the previous test script.
MarcoFalke commented at 11:26 AM on June 17, 2020: member

@troygiorshev @jnewbery based on your recent work in this area, you seem qualified to review this :pray:

in test/functional/p2p_invalid_messages.py:111 in 777714a2b2 outdated

 106 | @@ -106,6 +107,20 @@ def test_large_inv(self):
 107 |              conn.send_and_ping(msg)
 108 |          self.nodes[0].disconnect_p2ps()
 109 |  
 110 | +    def test_buffer(self):
 111 | +        self.log.debug('Check that peer is disconnected for bad message even if some of the bytes are delayed')

jonatack commented at 11:50 AM on June 17, 2020:

I think this log can be info.

MarcoFalke commented at 12:41 PM on June 17, 2020:

Changed

in test/functional/p2p_invalid_messages.py:122 in 777714a2b2 outdated

 117 | +            msg = b'\xff' * 4 + msg[4:]
 118 | +            conn.send_raw_message(msg[:22])
 119 | +            conn_ping.sync_with_ping()
 120 | +            conn.send_raw_message(msg[22:])
 121 | +            conn.wait_for_disconnect(timeout=1)
 122 | +        self.nodes[0].disconnect_p2ps()

jonatack commented at 11:51 AM on June 17, 2020:

I think this line can be removed.

MarcoFalke commented at 12:37 PM on June 17, 2020:

It is required to disconnect conn_ping

jonatack commented at 12:54 PM on June 17, 2020:

Interesting. I need to look at it more, but this seems to hit the conditional:

--- a/src/net.cpp
+++ b/src/net.cpp
@@ -652,9 +652,10 @@ int V1TransportDeserializer::readHeader(const char *pch, unsigned int nBytes)
     nHdrPos += nCopy;
 
     // if header incomplete, exit
-    if (nHdrPos < CMessageHeader::HEADER_SIZE)
+    if (nHdrPos < CMessageHeader::HEADER_SIZE) {
+        LogPrintf("Incomplete header");
         return nCopy;
-
+    }
--- a/test/functional/p2p_invalid_messages.py
+++ b/test/functional/p2p_invalid_messages.py
     def test_buffer(self):
         self.log.info('Check that peer is disconnected for bad message even if some of the bytes are delayed')
         conn = self.nodes[0].add_p2p_connection(P2PDataStore())
-        conn_ping = self.nodes[0].add_p2p_connection(P2PDataStore())
-        with self.nodes[0].assert_debug_log(['PROCESSMESSAGE: INVALID MESSAGESTART badmsg']):
+        with self.nodes[0].assert_debug_log(['Incomplete header', 'PROCESSMESSAGE: INVALID MESSAGESTART badmsg']):
             msg = conn.build_message(msg_unrecognized(str_data="d"))
             # modify magic bytes
             msg = b'\xff' * 4 + msg[4:]
             conn.send_raw_message(msg[:22])
-            conn_ping.sync_with_ping()
             conn.send_raw_message(msg[22:])
             conn.wait_for_disconnect(timeout=1)
-        self.nodes[0].disconnect_p2ps()

DrahtBot added the label Tests on Jun 17, 2020
jonatack commented at 12:33 PM on June 17, 2020: member

Tested concept ACK on verifying the behavior of incomplete headers, both if the message is bad like in this test, and perhaps also verify behavior when the message is good, e.g. magic bytes not modified. This test is very similar to test_magic_bytes; they could be placed next to each other.
test: Check that peer is disconnected for bad buffered message faac5ea7af
MarcoFalke force-pushed on Jun 17, 2020
MarcoFalke commented at 12:42 PM on June 17, 2020: member

they could be placed next to each other

Done
jnewbery commented at 12:57 PM on June 17, 2020: member

@troygiorshev already has a test that verifies that we're able to deserialize a message that is split across two buffer reads. I think that's preferable - there's no obvious reason that split messages and invalid messages need to be linked.
MarcoFalke commented at 1:12 PM on June 17, 2020: member

Is there a pull request for the test? I'd be happy to review and close this one instead.
troygiorshev commented at 3:05 PM on June 17, 2020: contributor

#19304 should cover everything discussed here.
MarcoFalke closed this on Jun 17, 2020
MarcoFalke commented at 3:45 PM on June 17, 2020: member

Thx
MarcoFalke deleted the branch on Jun 17, 2020
MarcoFalke referenced this in commit 343c0bfbf1 on Jun 18, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19304 test: Check that message sends successfully when header is split across two buffers