From 1729c29dedc64d900a1a1c686a56e16fa5417fa1 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 18 Sep 2017 18:45:51 -0400 Subject: [PATCH 1/4] net: split socket creation out of connection Also, check for the correct error during socket creation --- src/net.cpp | 18 ++++++++++++++++-- src/netbase.cpp | 42 +++++++++++++++++++++++++++--------------- src/netbase.h | 1 + 3 files changed, 44 insertions(+), 17 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 5eaeaab8f64..8f15842150f 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -417,16 +417,30 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo if (addrConnect.IsValid()) { bool proxyConnectionFailed = false; - if (GetProxy(addrConnect.GetNetwork(), proxy)) + if (GetProxy(addrConnect.GetNetwork(), proxy)) { + hSocket = CreateSocket(proxy.proxy); + if (hSocket == INVALID_SOCKET) { + return nullptr; + } connected = ConnectThroughProxy(proxy, addrConnect.ToStringIP(), addrConnect.GetPort(), hSocket, nConnectTimeout, &proxyConnectionFailed); - else // no proxy needed (none set for target network) + } else { + // no proxy needed (none set for target network) + hSocket = CreateSocket(addrConnect); + if (hSocket == INVALID_SOCKET) { + return nullptr; + } connected = ConnectSocketDirectly(addrConnect, hSocket, nConnectTimeout); + } if (!proxyConnectionFailed) { // If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to // the proxy, mark this as an attempt. addrman.Attempt(addrConnect, fCountFailure); } } else if (pszDest && GetNameProxy(proxy)) { + hSocket = CreateSocket(proxy.proxy); + if (hSocket == INVALID_SOCKET) { + return nullptr; + } std::string host; int port = default_port; SplitHostPort(std::string(pszDest), port, host); diff --git a/src/netbase.cpp b/src/netbase.cpp index 82040605c55..9b63d603420 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -452,20 +452,18 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials return true; } -bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRet, int nTimeout) +SOCKET CreateSocket(const CService &addrConnect) { - hSocketRet = INVALID_SOCKET; - struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) { - LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString()); - return false; + LogPrintf("Cannot create socket for %s: unsupported network\n", addrConnect.ToString()); + return INVALID_SOCKET; } SOCKET hSocket = socket(((struct sockaddr*)&sockaddr)->sa_family, SOCK_STREAM, IPPROTO_TCP); if (hSocket == INVALID_SOCKET) - return false; + return INVALID_SOCKET; #ifdef SO_NOSIGPIPE int set = 1; @@ -479,9 +477,24 @@ bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRet, int // Set to non-blocking if (!SetSocketNonBlocking(hSocket, true)) { CloseSocket(hSocket); - return error("ConnectSocketDirectly: Setting socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError())); + LogPrintf("ConnectSocketDirectly: Setting socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError())); } + return hSocket; +} +bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTimeout) +{ + struct sockaddr_storage sockaddr; + socklen_t len = sizeof(sockaddr); + if (hSocket == INVALID_SOCKET) { + LogPrintf("Cannot connect to %s: invalid socket\n", addrConnect.ToString()); + return false; + } + if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) { + LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString()); + CloseSocket(hSocket); + return false; + } if (connect(hSocket, (struct sockaddr*)&sockaddr, len) == SOCKET_ERROR) { int nErr = WSAGetLastError(); @@ -534,8 +547,6 @@ bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRet, int return false; } } - - hSocketRet = hSocket; return true; } @@ -587,9 +598,8 @@ bool IsProxy(const CNetAddr &addr) { return false; } -bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, SOCKET& hSocketRet, int nTimeout, bool *outProxyConnectionFailed) +bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, SOCKET& hSocket, int nTimeout, bool *outProxyConnectionFailed) { - SOCKET hSocket = INVALID_SOCKET; // first connect to proxy server if (!ConnectSocketDirectly(proxy.proxy, hSocket, nTimeout)) { if (outProxyConnectionFailed) @@ -601,14 +611,16 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int ProxyCredentials random_auth; static std::atomic_int counter(0); random_auth.username = random_auth.password = strprintf("%i", counter++); - if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) + if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) { + CloseSocket(hSocket); return false; + } } else { - if (!Socks5(strDest, (unsigned short)port, 0, hSocket)) + if (!Socks5(strDest, (unsigned short)port, 0, hSocket)) { + CloseSocket(hSocket); return false; + } } - - hSocketRet = hSocket; return true; } bool LookupSubNet(const char* pszName, CSubNet& ret) diff --git a/src/netbase.h b/src/netbase.h index e7d7bcb3751..59945ea97d2 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -51,6 +51,7 @@ bool Lookup(const char *pszName, CService& addr, int portDefault, bool fAllowLoo bool Lookup(const char *pszName, std::vector& vAddr, int portDefault, bool fAllowLookup, unsigned int nMaxSolutions); CService LookupNumeric(const char *pszName, int portDefault = 0); bool LookupSubNet(const char *pszName, CSubNet& subnet); +SOCKET CreateSocket(const CService &addrConnect); bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRet, int nTimeout); bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, SOCKET& hSocketRet, int nTimeout, bool *outProxyConnectionFailed); /** Return readable error string for a network error code */ From 9e3b2f576bb368a0857e808dcbd24b2dcb8bef2d Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 2 Oct 2017 14:18:32 -0400 Subject: [PATCH 2/4] net: Move IsSelectableSocket check into socket creation We use select in ConnectSocketDirectly, so this check needs to happen before that. IsSelectableSocket will not be relevant after upcoming changes to remove select. --- src/net.cpp | 27 +++++++++++---------------- src/netbase.cpp | 6 ++++++ 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 8f15842150f..07128a03495 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -446,24 +446,19 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo SplitHostPort(std::string(pszDest), port, host); connected = ConnectThroughProxy(proxy, host, port, hSocket, nConnectTimeout, nullptr); } - if (connected) { - if (!IsSelectableSocket(hSocket)) { - LogPrintf("Cannot create connection: non-selectable socket created (fd >= FD_SETSIZE ?)\n"); - CloseSocket(hSocket); - return nullptr; - } - - // Add node - NodeId id = GetNewNodeId(); - uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); - CAddress addr_bind = GetBindAddress(hSocket); - CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", false); - pnode->AddRef(); - - return pnode; + if (!connected) { + CloseSocket(hSocket); + return nullptr; } - return nullptr; + // Add node + NodeId id = GetNewNodeId(); + uint64_t nonce = GetDeterministicRandomizer(RANDOMIZER_ID_LOCALHOSTNONCE).Write(id).Finalize(); + CAddress addr_bind = GetBindAddress(hSocket); + CNode* pnode = new CNode(id, nLocalServices, GetBestHeight(), hSocket, addrConnect, CalculateKeyedNetGroup(addrConnect), nonce, addr_bind, pszDest ? pszDest : "", false); + pnode->AddRef(); + + return pnode; } void CConnman::DumpBanlist() diff --git a/src/netbase.cpp b/src/netbase.cpp index 9b63d603420..1635957b7f0 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -465,6 +465,12 @@ SOCKET CreateSocket(const CService &addrConnect) if (hSocket == INVALID_SOCKET) return INVALID_SOCKET; + if (!IsSelectableSocket(hSocket)) { + CloseSocket(hSocket); + LogPrintf("Cannot create connection: non-selectable socket created (fd >= FD_SETSIZE ?)\n"); + return INVALID_SOCKET; + } + #ifdef SO_NOSIGPIPE int set = 1; // Different way of disabling SIGPIPE on BSD From df3bcf89e49ec951baa3778a2452c0d1237ec053 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 2 Oct 2017 16:37:36 -0400 Subject: [PATCH 3/4] net: pass socket closing responsibility up to caller for outgoing connections This allows const references to be passed around, making it clear where the socket may and may not be invalidated. --- src/netbase.cpp | 32 ++++---------------------------- src/netbase.h | 4 ++-- 2 files changed, 6 insertions(+), 30 deletions(-) diff --git a/src/netbase.cpp b/src/netbase.cpp index 1635957b7f0..74ea6b19cfa 100644 --- a/src/netbase.cpp +++ b/src/netbase.cpp @@ -317,12 +317,11 @@ std::string Socks5ErrorString(uint8_t err) } /** Connect using SOCKS5 (as described in RFC1928) */ -static bool Socks5(const std::string& strDest, int port, const ProxyCredentials *auth, SOCKET& hSocket) +static bool Socks5(const std::string& strDest, int port, const ProxyCredentials *auth, const SOCKET& hSocket) { IntrRecvError recvr; LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest); if (strDest.size() > 255) { - CloseSocket(hSocket); return error("Hostname too long"); } // Accepted authentication methods @@ -338,17 +337,14 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials } ssize_t ret = send(hSocket, (const char*)vSocks5Init.data(), vSocks5Init.size(), MSG_NOSIGNAL); if (ret != (ssize_t)vSocks5Init.size()) { - CloseSocket(hSocket); return error("Error sending to proxy"); } uint8_t pchRet1[2]; if ((recvr = InterruptibleRecv(pchRet1, 2, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) { - CloseSocket(hSocket); LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port); return false; } if (pchRet1[0] != SOCKSVersion::SOCKS5) { - CloseSocket(hSocket); return error("Proxy failed to initialize"); } if (pchRet1[1] == SOCKS5Method::USER_PASS && auth) { @@ -363,23 +359,19 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials vAuth.insert(vAuth.end(), auth->password.begin(), auth->password.end()); ret = send(hSocket, (const char*)vAuth.data(), vAuth.size(), MSG_NOSIGNAL); if (ret != (ssize_t)vAuth.size()) { - CloseSocket(hSocket); return error("Error sending authentication to proxy"); } LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password); uint8_t pchRetA[2]; if ((recvr = InterruptibleRecv(pchRetA, 2, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) { - CloseSocket(hSocket); return error("Error reading proxy authentication response"); } if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) { - CloseSocket(hSocket); return error("Proxy authentication unsuccessful"); } } else if (pchRet1[1] == SOCKS5Method::NOAUTH) { // Perform no authentication } else { - CloseSocket(hSocket); return error("Proxy requested wrong authentication method %02x", pchRet1[1]); } std::vector vSocks5; @@ -393,12 +385,10 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials vSocks5.push_back((port >> 0) & 0xFF); ret = send(hSocket, (const char*)vSocks5.data(), vSocks5.size(), MSG_NOSIGNAL); if (ret != (ssize_t)vSocks5.size()) { - CloseSocket(hSocket); return error("Error sending to proxy"); } uint8_t pchRet2[4]; if ((recvr = InterruptibleRecv(pchRet2, 4, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) { - CloseSocket(hSocket); if (recvr == IntrRecvError::Timeout) { /* If a timeout happens here, this effectively means we timed out while connecting * to the remote node. This is very common for Tor, so do not print an @@ -409,17 +399,14 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials } } if (pchRet2[0] != SOCKSVersion::SOCKS5) { - CloseSocket(hSocket); return error("Proxy failed to accept request"); } if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) { // Failures to connect to a peer that are not proxy errors - CloseSocket(hSocket); LogPrintf("Socks5() connect to %s:%d failed: %s\n", strDest, port, Socks5ErrorString(pchRet2[1])); return false; } if (pchRet2[2] != 0x00) { // Reserved field must be 0 - CloseSocket(hSocket); return error("Error: malformed proxy response"); } uint8_t pchRet3[256]; @@ -431,21 +418,18 @@ static bool Socks5(const std::string& strDest, int port, const ProxyCredentials { recvr = InterruptibleRecv(pchRet3, 1, SOCKS5_RECV_TIMEOUT, hSocket); if (recvr != IntrRecvError::OK) { - CloseSocket(hSocket); return error("Error reading from proxy"); } int nRecv = pchRet3[0]; recvr = InterruptibleRecv(pchRet3, nRecv, SOCKS5_RECV_TIMEOUT, hSocket); break; } - default: CloseSocket(hSocket); return error("Error: malformed proxy response"); + default: return error("Error: malformed proxy response"); } if (recvr != IntrRecvError::OK) { - CloseSocket(hSocket); return error("Error reading from proxy"); } if ((recvr = InterruptibleRecv(pchRet3, 2, SOCKS5_RECV_TIMEOUT, hSocket)) != IntrRecvError::OK) { - CloseSocket(hSocket); return error("Error reading from proxy"); } LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest); @@ -488,7 +472,7 @@ SOCKET CreateSocket(const CService &addrConnect) return hSocket; } -bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTimeout) +bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocket, int nTimeout) { struct sockaddr_storage sockaddr; socklen_t len = sizeof(sockaddr); @@ -498,7 +482,6 @@ bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTi } if (!addrConnect.GetSockAddr((struct sockaddr*)&sockaddr, &len)) { LogPrintf("Cannot connect to %s: unsupported network\n", addrConnect.ToString()); - CloseSocket(hSocket); return false; } if (connect(hSocket, (struct sockaddr*)&sockaddr, len) == SOCKET_ERROR) @@ -515,13 +498,11 @@ bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTi if (nRet == 0) { LogPrint(BCLog::NET, "connection to %s timeout\n", addrConnect.ToString()); - CloseSocket(hSocket); return false; } if (nRet == SOCKET_ERROR) { LogPrintf("select() for %s failed: %s\n", addrConnect.ToString(), NetworkErrorString(WSAGetLastError())); - CloseSocket(hSocket); return false; } socklen_t nRetSize = sizeof(nRet); @@ -532,13 +513,11 @@ bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTi #endif { LogPrintf("getsockopt() for %s failed: %s\n", addrConnect.ToString(), NetworkErrorString(WSAGetLastError())); - CloseSocket(hSocket); return false; } if (nRet != 0) { LogPrintf("connect() to %s failed after select(): %s\n", addrConnect.ToString(), NetworkErrorString(nRet)); - CloseSocket(hSocket); return false; } } @@ -549,7 +528,6 @@ bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocket, int nTi #endif { LogPrintf("connect() to %s failed: %s\n", addrConnect.ToString(), NetworkErrorString(WSAGetLastError())); - CloseSocket(hSocket); return false; } } @@ -604,7 +582,7 @@ bool IsProxy(const CNetAddr &addr) { return false; } -bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, SOCKET& hSocket, int nTimeout, bool *outProxyConnectionFailed) +bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocket, int nTimeout, bool *outProxyConnectionFailed) { // first connect to proxy server if (!ConnectSocketDirectly(proxy.proxy, hSocket, nTimeout)) { @@ -618,12 +596,10 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int static std::atomic_int counter(0); random_auth.username = random_auth.password = strprintf("%i", counter++); if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) { - CloseSocket(hSocket); return false; } } else { if (!Socks5(strDest, (unsigned short)port, 0, hSocket)) { - CloseSocket(hSocket); return false; } } diff --git a/src/netbase.h b/src/netbase.h index 59945ea97d2..52e920b3d8c 100644 --- a/src/netbase.h +++ b/src/netbase.h @@ -52,8 +52,8 @@ bool Lookup(const char *pszName, std::vector& vAddr, int portDefault, CService LookupNumeric(const char *pszName, int portDefault = 0); bool LookupSubNet(const char *pszName, CSubNet& subnet); SOCKET CreateSocket(const CService &addrConnect); -bool ConnectSocketDirectly(const CService &addrConnect, SOCKET& hSocketRet, int nTimeout); -bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, SOCKET& hSocketRet, int nTimeout, bool *outProxyConnectionFailed); +bool ConnectSocketDirectly(const CService &addrConnect, const SOCKET& hSocketRet, int nTimeout); +bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int port, const SOCKET& hSocketRet, int nTimeout, bool *outProxyConnectionFailed); /** Return readable error string for a network error code */ std::string NetworkErrorString(int err); /** Close socket and set hSocket to INVALID_SOCKET */ From 3830b6e0659106458c941029f5b2e789e3cb38a3 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 2 Oct 2017 16:31:37 -0400 Subject: [PATCH 4/4] net: use CreateSocket for binds --- src/net.cpp | 25 +------------------------ 1 file changed, 1 insertion(+), 24 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 07128a03495..8eb25a8c74e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2078,44 +2078,21 @@ bool CConnman::BindListenPort(const CService &addrBind, std::string& strError, b return false; } - SOCKET hListenSocket = socket(((struct sockaddr*)&sockaddr)->sa_family, SOCK_STREAM, IPPROTO_TCP); + SOCKET hListenSocket = CreateSocket(addrBind); if (hListenSocket == INVALID_SOCKET) { strError = strprintf("Error: Couldn't open socket for incoming connections (socket returned error %s)", NetworkErrorString(WSAGetLastError())); LogPrintf("%s\n", strError); return false; } - if (!IsSelectableSocket(hListenSocket)) - { - strError = "Error: Couldn't create a listenable socket for incoming connections"; - LogPrintf("%s\n", strError); - return false; - } - - #ifndef WIN32 -#ifdef SO_NOSIGPIPE - // Different way of disabling SIGPIPE on BSD - setsockopt(hListenSocket, SOL_SOCKET, SO_NOSIGPIPE, (void*)&nOne, sizeof(int)); -#endif // Allow binding if the port is still in TIME_WAIT state after // the program was closed and restarted. setsockopt(hListenSocket, SOL_SOCKET, SO_REUSEADDR, (void*)&nOne, sizeof(int)); - // Disable Nagle's algorithm - setsockopt(hListenSocket, IPPROTO_TCP, TCP_NODELAY, (void*)&nOne, sizeof(int)); #else setsockopt(hListenSocket, SOL_SOCKET, SO_REUSEADDR, (const char*)&nOne, sizeof(int)); - setsockopt(hListenSocket, IPPROTO_TCP, TCP_NODELAY, (const char*)&nOne, sizeof(int)); #endif - // Set to non-blocking, incoming connections will also inherit this - if (!SetSocketNonBlocking(hListenSocket, true)) { - CloseSocket(hListenSocket); - strError = strprintf("BindListenPort: Setting listening socket to non-blocking failed, error %s\n", NetworkErrorString(WSAGetLastError())); - LogPrintf("%s\n", strError); - return false; - } - // some systems don't have IPV6_V6ONLY but are always v6only; others do have the option // and enable it by default or not. Try to enable it, if possible. if (addrBind.IsIPv6()) {