Merge #15654: net: Remove unused unsanitized user agent string CNode::strSubVer

fa8548c5d1 net: Remove unused unsanitized user agent string CNode::strSubVer (MarcoFalke)

Pull request description:

  I fail to see a use case for this unsanitized byte array. In fact this can easily be confused with `cleanSubVer` and be displayed to the user (or logged) by a simple typo that is hard to find in review.

  Further reading: https://btcinformation.org/en/developer-reference#version

ACKs for commit fa8548:
  promag:
    utACK fa8548c, good catch.
  practicalswift:
    utACK fa8548c5d13957f57f9b1e20e03002600962f7f0
  sipa:
    utACK fa8548c5d13957f57f9b1e20e03002600962f7f0

Tree-SHA512: 3c3ff1504d1583ad099df9a6aa761458a82ec48a58ef7aaa9b5679a5281dd1b59036ba2932ed708488951a565b669a3083ef70be5a58472ff8677b971162ae2f
This commit is contained in:
MarcoFalke 2019-04-04 16:45:12 -04:00 committed by Pasta
parent 7f012a777b
commit 3a75bef4af
No known key found for this signature in database
GPG Key ID: 52527BEDABE87984
3 changed files with 9 additions and 11 deletions

View File

@ -3899,7 +3899,6 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
nVersion = 0; nVersion = 0;
nNumWarningsSkipped = 0; nNumWarningsSkipped = 0;
nLastWarningTime = 0; nLastWarningTime = 0;
strSubVer = "";
fWhitelisted = false; fWhitelisted = false;
fOneShot = false; fOneShot = false;
m_manual_connection = false; m_manual_connection = false;

View File

@ -62,7 +62,7 @@ static const unsigned int MAX_LOCATOR_SZ = 101;
static const unsigned int MAX_ADDR_TO_SEND = 1000; static const unsigned int MAX_ADDR_TO_SEND = 1000;
/** Maximum length of incoming protocol messages (no message over 3 MiB is currently acceptable). */ /** Maximum length of incoming protocol messages (no message over 3 MiB is currently acceptable). */
static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 3 * 1024 * 1024; static const unsigned int MAX_PROTOCOL_MESSAGE_LENGTH = 3 * 1024 * 1024;
/** Maximum length of strSubVer in `version` message */ /** Maximum length of the user agent string in `version` message */
static const unsigned int MAX_SUBVERSION_LENGTH = 256; static const unsigned int MAX_SUBVERSION_LENGTH = 256;
/** Maximum number of automatic outgoing nodes */ /** Maximum number of automatic outgoing nodes */
static const int MAX_OUTBOUND_CONNECTIONS = 8; static const int MAX_OUTBOUND_CONNECTIONS = 8;
@ -872,11 +872,11 @@ public:
const CAddress addrBind; const CAddress addrBind;
std::atomic<int> nNumWarningsSkipped; std::atomic<int> nNumWarningsSkipped;
std::atomic<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 * cleanSubVer is a sanitized string of the user agent byte array we read
// store the sanitized version in cleanSubVer. The original should be used when dealing with * from the wire. This cleaned string can safely be logged or displayed.
// the network or wire types and the cleaned string used when displayed or logged. */
std::string strSubVer GUARDED_BY(cs_SubVer), cleanSubVer GUARDED_BY(cs_SubVer); std::string cleanSubVer GUARDED_BY(cs_SubVer){};
CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer CCriticalSection cs_SubVer; // used for both cleanSubVer and strSubVer
bool fWhitelisted; // This peer can bypass DoS banning. bool fWhitelisted; // This peer can bypass DoS banning.
bool fFeeler; // If true this node is being used as a short lived feeler. bool fFeeler; // If true this node is being used as a short lived feeler.

View File

@ -2156,7 +2156,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
ServiceFlags nServices; ServiceFlags nServices;
int nVersion; int nVersion;
int nSendVersion; int nSendVersion;
std::string strSubVer;
std::string cleanSubVer; std::string cleanSubVer;
int nStartingHeight = -1; int nStartingHeight = -1;
bool fRelay = true; bool fRelay = true;
@ -2193,6 +2192,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
if (!vRecv.empty()) if (!vRecv.empty())
vRecv >> addrFrom >> nNonce; vRecv >> addrFrom >> nNonce;
if (!vRecv.empty()) { if (!vRecv.empty()) {
std::string strSubVer;
vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH); vRecv >> LIMITED_STRING(strSubVer, MAX_SUBVERSION_LENGTH);
cleanSubVer = SanitizeString(strSubVer); cleanSubVer = SanitizeString(strSubVer);
} }
@ -2238,9 +2238,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
PushNodeVersion(pfrom, connman, GetAdjustedTime()); PushNodeVersion(pfrom, connman, GetAdjustedTime());
if (Params().NetworkIDString() == CBaseChainParams::DEVNET) { if (Params().NetworkIDString() == CBaseChainParams::DEVNET) {
if (strSubVer.find(strprintf("devnet=%s", gArgs.GetDevNetName())) == std::string::npos) { if (cleanSubVer.find(strprintf("devnet=%s", gArgs.GetDevNetName())) == std::string::npos) {
LOCK(cs_main); LOCK(cs_main);
LogPrintf("connected to wrong devnet. Reported version is %s, expected devnet name is %s\n", strSubVer, gArgs.GetDevNetName()); LogPrintf("connected to wrong devnet. Reported version is %s, expected devnet name is %s\n", cleanSubVer, gArgs.GetDevNetName());
if (!pfrom->fInbound) if (!pfrom->fInbound)
Misbehaving(pfrom->GetId(), 100); // don't try to connect again Misbehaving(pfrom->GetId(), 100); // don't try to connect again
else else
@ -2257,7 +2257,6 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
pfrom->SetAddrLocal(addrMe); pfrom->SetAddrLocal(addrMe);
{ {
LOCK(pfrom->cs_SubVer); LOCK(pfrom->cs_SubVer);
pfrom->strSubVer = strSubVer;
pfrom->cleanSubVer = cleanSubVer; pfrom->cleanSubVer = cleanSubVer;
} }
pfrom->nStartingHeight = nStartingHeight; pfrom->nStartingHeight = nStartingHeight;