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

1. 
   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:

   0src/net.cpp:    // if header incomplete, exit
   

2. 
   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.
3. 
   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:
4. 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
5. 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:

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

6. DrahtBot added the label Tests on Jun 17, 2020
7. 
   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.
8. test: Check that peer is disconnected for bad buffered message faac5ea7af
9. MarcoFalke force-pushed on Jun 17, 2020
10. 
    MarcoFalke commented at 12:42 pm on June 17, 2020: member

    they could be placed next to each other

    Done

11. 
    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.
12. 
    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.
13. 
    troygiorshev commented at 3:05 pm on June 17, 2020: contributor

    #19304 should cover everything discussed here.
14. MarcoFalke closed this on Jun 17, 2020

    ---

15. 
    MarcoFalke commented at 3:45 pm on June 17, 2020: member

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

Contributors
MarcoFalke jonatack jnewbery troygiorshev

Labels
Tests