Fix a few cases where messages were sent after requested disconnect #8862
Conversation
…n disconnection 75ead758 turned these into crashes in the event of a handshake failure, most notably when a peer does not offer the expected services. There are likely other cases that these assertions will find as well.
|
Looks harmless at worst. utACK. |
|
utACK. Seems like it would be easy to introduce more of these issues in the future, this fix seems fine though we might want to consider something more comprehensive for later. |
|
@gmaxwell Agreed. As mentioned above, #8708 adds an assert to catch a specific form of this case (sending a post-handshake message before the handshake is complete), so a more comprehensive fix will be required for sure, and is coming as part of the disconnection logic cleanup. These two were just added because they're low-hanging fruit and are easily backported, and because they were the only ones actually observed while testing. |
|
Why not put this logic at the start of SendMessages - where it checks if Version is non zero? |
|
@rebroad I believe that may inhibit the send of some valid reject messages. Generally speaking, I don't like the idea of relying on a stateful difference in SendMessages, as I think the goal should be generally async responses as opposed to the current model of "check all connections for all eligible broadcasts all the time". It's a good makeshift suggestion though, I'll have a look. |
|
utACK
Ouch. I hope we can avoid this in a consistent way. To avoid DoSes, in network code it's always preferable to continue and (if its state is messed up) just boot a peer to crashing. Even in case of bugs. |
… disconnect 905bc68 net: fix a few cases where messages were sent rather than dropped upon disconnection (Cory Fields)
…n disconnection 75ead758 turned these into crashes in the event of a handshake failure, most notably when a peer does not offer the expected services. There are likely other cases that these assertions will find as well. Github-Pull: bitcoin#8862 Rebased-From: 905bc68
…quested disconnect 905bc68 net: fix a few cases where messages were sent rather than dropped upon disconnection (Cory Fields)
…n disconnection 75ead758 turned these into crashes in the event of a handshake failure, most notably when a peer does not offer the expected services. There are likely other cases that these assertions will find as well. Github-Pull: bitcoin#8862 Rebased-From: 905bc68
…quested disconnect 905bc68 net: fix a few cases where messages were sent rather than dropped upon disconnection (Cory Fields)
b954796 include missing atomic to make CMake linux happy. (furszy) c452676 Make the cs_sendProcessing a LOCK instead of a TRY_LOCK (Matt Corallo) ec23964 Split CNode::cs_vSend: message processing and message sending (Matt Corallo) f9c458a Remove double brackets in addrman (Matt Corallo) e48c0d3 Fix AddrMan locking (Matt Corallo) 2d71f05 Make fDisconnect an std::atomic (Matt Corallo) 0544cc6 net: fix a few cases where messages were sent rather than dropped upon disconnection (furszy) Pull request description: Running 4.2 branch with `enable-debug`. Fixing: ``` POTENTIAL DEADLOCK DETECTED Previous lock order was: pnode->cs_vRecvMsg net.cpp:1830 (TRY) (in thread ) (1) cs_main main.cpp:5275 (in thread ) (2) cs_vSend net.cpp:2465 (in thread ) Current lock order is: (2) pnode->cs_vSend net.cpp:1846 (TRY) (in thread ) (1) cs_main main.cpp:6018 (TRY) (in thread ) ``` Plus, added few more minor possible races fixes too. Back porting: bitcoin#8862 bitcoin#9225 bitcoin#9535 ACKs for top commit: Fuzzbawls: ACK b954796 random-zebra: ACK b954796 Tree-SHA512: e15bd81e51282771ca4cb40689229d54787d72d7ed0ebc5f6f0058d3e252cc6691d8ebfc4670207bcd835d32b7898863651fd2ac74704d6cb4fdd30554d8a83a
Noticed these in #8708, which turns them into crashes. PRing separately here as I believe it needs a 0.13.1 backport.