test: Check that peer is disconnected for bad buffered message #19302
Conversation
|
Can be tested by adding an |
|
@troygiorshev @jnewbery based on your recent work in this area, you seem qualified to review this 🙏 |
There was a problem hiding this comment.
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.
| conn_ping.sync_with_ping() | ||
| conn.send_raw_message(msg[22:]) | ||
| conn.wait_for_disconnect(timeout=1) | ||
| self.nodes[0].disconnect_p2ps() |
There was a problem hiding this comment.
I think this line can be removed.
There was a problem hiding this comment.
It is required to disconnect conn_ping
There was a problem hiding this comment.
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()
Done |
|
@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. |
|
Is there a pull request for the test? I'd be happy to review and close this one instead. |
|
#19304 should cover everything discussed here. |
|
Thx |
… is split across two buffers 80d4423 Test buffered valid message (Troy Giorshev) Pull request description: This PR is a tweak of #19302. This sends a valid message. Additionally, this test includes logging in the same vein as #19272. ACKs for top commit: MarcoFalke: tested ACK 80d4423 (added an assert(false) to observe deterministic coverage) 🌦 gzhao408: ACK 80d4423 👊 Tree-SHA512: 3b1aa5ec480a1661917354788923d64595e2886448c9697ec0606a81293e8b4a4642b2b3cc9afb2206ce6f74e5c6d687308c5ad19cb73c5b354d3071ad8496f8
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: