Limit message sizes before receiving. #5843
Conversation
|
Tested by IBDing a node running this against a node running this. |
|
Code looks correct to me, going to test. |
| if (msg->nMessageSize > MAX_PROTOCOL_MESSAGE_LENGTH) { | ||
| LOCK(cs_main); | ||
| Misbehaving(pnode->GetId(), 100); | ||
| LogPrintf("net", "Oversized message from peer=%d (size=%d)\n", pnode->GetId(), msg->nMessageSize); |
There was a problem hiding this comment.
I lobbed a 4MB message at my node and it crashed here:
EXCEPTION: St13runtime_error
tinyformat: Not enough conversion specifiers in format string
bitcoin in net
As a category is passed, this should be LogPrint, not LogPrintf [yes, stupid naming.].
|
Retesting. |
|
Tested ACK |
|
@gmaxwell Please test again? |
|
Hurray, It disconnects now, but as I pointed out-- while it grants misbehaving-- it does not attempt to ban because now it disconnects before it gets a chance to do so. If it's not going to ban, there is no point in setting the misbehaving, and a simpler patch which doesn't use the signals would work the same. (though I suppose we should have the combiner fix for the other signal that uses the return value!). |
|
ACK on the signal combiner. RE: Misbehaving/banning: seems like it should ban, but instead of introducing Yet Another Signal, seems to me a check for state->fShouldBan in main.cpp FinalizeNode (called when net disconnects a peer) might be the right approach. |
|
@sipa : nit: I think the combiner code could be replaced with boost::bind(boost::algorithm::all_of_equal, _1, _2, true) |
|
Moved the combiner logic, and simplified the code here. A more complex signals-based solution can be done later. |
This introduces a fixed limit for the size of p2p messages, and enforces it before download.
|
I was a little surprised that you went with logging at only debug=net level, but it works. ACK. |
ba04c4a Limit message sizes before transfer (Pieter Wuille)
|
I like the simpler solution. ACK. |
|
posthumous ACK for simpler solution |
This introduces a fixed limit for the size of p2p messages, and enforces it before download. Rebased-From: ba04c4a Github-Pull: bitcoin#5843 (cherry picked from commit d5d8998)
There is currently no message over 2 MiB that is acceptable anyway, no need to support them inside our network buffers.
This is done by a dispatch mechanism in net, rather than a direct check, to support future modularity (where message handling is split over different modules). It's also required to deal with DoS management correctly (which is done outside of net).
It also doesn't touch serialize.h's MAX_SIZE, as it's a global for all serialization usage, including non-protocol processing.