Skip to content

Commit

Permalink
net: deserialize the entire version message locally
Browse files Browse the repository at this point in the history 
This avoids having some vars set if the version negotiation fails.

Also copy it all into CNode at the same site. nVersion and
fSuccessfullyConnected are set last, as they are the gates for the other
vars. Make them atomic for that reason.
  • Loading branch information
@Fuzzbawls
Fuzzbawls committed Sep 2, 2020
1 parent 444f599 commit 2be6877
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 21 deletions.
50 changes: 31 additions & 19 deletions src/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5289,14 +5289,20 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
CAddress addrFrom;
uint64_t nNonce = 1;
uint64_t nServiceInt;
ServiceFlags nServices;
int nVersion;
int nSendVersion;
std::string strSubVer;
int nStartingHeight = -1;
bool fRelay = true;
vRecv >> nVersion >> nServiceInt >> nTime >> addrMe;
pfrom->nServices = ServiceFlags(nServiceInt);
nSendVersion = std::min(nVersion, PROTOCOL_VERSION);
nServices = ServiceFlags(nServiceInt);
if (!pfrom->fInbound) {
connman.SetServices(pfrom->addr, pfrom->nServices);
connman.SetServices(pfrom->addr, nServices);
}
if (pfrom->nServicesExpected & ~pfrom->nServices) {
LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom->id, pfrom->nServices, pfrom->nServicesExpected);
if (pfrom->nServicesExpected & ~nServices) {
LogPrint(BCLog::NET, "peer=%d does not offer the expected services (%08x offered, %08x expected); disconnecting\n", pfrom->id, nServices, pfrom->nServicesExpected);
connman.PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::REJECT, strCommand, REJECT_NONSTANDARD,
strprintf("Expected to offer services %08x", pfrom->nServicesExpected)));
pfrom->fDisconnect = true;
Expand All @@ -5311,15 +5317,12 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
if (!vRecv.empty())
vRecv >> addrFrom >> nNonce;
if (!vRecv.empty()) {
vRecv >> LIMITED_STRING(pfrom->strSubVer, MAX_SUBVERSION_LENGTH);
pfrom->cleanSubVer = SanitizeString(pfrom->strSubVer);
vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH);
}
if (!vRecv.empty())
vRecv >> pfrom->nStartingHeight;
vRecv >> nStartingHeight;
if (!vRecv.empty())
vRecv >> pfrom->fRelayTxes; // set to true after we get the first filter* message
else
pfrom->fRelayTxes = true;
vRecv >> fRelay;

// Disconnect if we connected to ourself
if (pfrom->fInbound && !connman.CheckIncomingNonce(nNonce)) {
Expand All @@ -5343,7 +5346,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
fRequestedSporksIDB = true;
}

pfrom->addrLocal = addrMe;
if (pfrom->fInbound && addrMe.IsRoutable()) {
SeenLocal(addrMe);
}
Expand All @@ -5352,18 +5354,29 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
if (pfrom->fInbound)
PushNodeVersion(pfrom, connman, GetAdjustedTime());

pfrom->fClient = !(pfrom->nServices & NODE_NETWORK);
connman.PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));

pfrom->nServices = nServices;
pfrom->addrLocal = addrMe;
pfrom->strSubVer = strSubVer;
pfrom->cleanSubVer = SanitizeString(strSubVer);
pfrom->nStartingHeight = nStartingHeight;
pfrom->fClient = !(nServices & NODE_NETWORK);
{
LOCK(pfrom->cs_filter);
pfrom->fRelayTxes = fRelay; // set to true after we get the first filter* message
}

// Change version
pfrom->SetSendVersion(nSendVersion);
pfrom->nVersion = nVersion;
pfrom->fSuccessfullyConnected = true;

{
LOCK(cs_main);
// Potentially mark this peer as a preferred download peer.
UpdatePreferredDownload(pfrom, State(pfrom->GetId()));
}
// Change version
connman.PushMessage(pfrom, CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));
int nSendVersion = std::min(nVersion, PROTOCOL_VERSION);
pfrom->nVersion = nVersion;
pfrom->SetSendVersion(nSendVersion);

if (!pfrom->fInbound) {
// Advertise our address
Expand Down Expand Up @@ -5401,7 +5414,6 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
pfrom->nTimeOffset = nTimeOffset;
const int nTimeSlotLength = Params().GetConsensus().nTimeSlotLength;
if (abs64(nTimeOffset) < 2 * nTimeSlotLength) {
pfrom->fSuccessfullyConnected = true;
AddTimeData(pfrom->addr, nTimeOffset, nTimeSlotLength);
} else {
LogPrintf("timeOffset (%d seconds) too large. Disconnecting node %s\n",
Expand Down Expand Up @@ -5430,7 +5442,7 @@ bool static ProcessMessage(CNode* pfrom, std::string strCommand, CDataStream& vR
CNetMsgMaker msgMaker(pfrom->GetSendVersion());

if (strCommand == NetMsgType::VERACK) {
pfrom->SetRecvVersion(std::min(pfrom->nVersion, PROTOCOL_VERSION));
pfrom->SetRecvVersion(std::min(pfrom->nVersion.load(), PROTOCOL_VERSION));

// Mark this node as currently connected, so we update its timestamp later.
if (pfrom->fNetworkNode) {
Expand Down
4 changes: 2 additions & 2 deletions src/net.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ class CNode
const CAddress addr;
std::string addrName;
CService addrLocal;
int nVersion;
std::atomic<int> nVersion;
// strSubVer is whatever byte array we read from the wire. However, this field is intended
// to be printed out, displayed to humans in various forms and so on. So we sanitize it and
// store the sanitized version in cleanSubVer. The original should be used when dealing with
Expand All @@ -605,7 +605,7 @@ class CNode
bool fClient;
const bool fInbound;
bool fNetworkNode;
bool fSuccessfullyConnected;
std::atomic_bool fSuccessfullyConnected;
std::atomic_bool fDisconnect;
// We use fRelayTxes for two purposes -
// a) it allows us to not relay tx invs before receiving the peer's version message
Expand Down

0 comments on commit 2be6877

Please sign in to comment.