net: move CreateSock() calls from ConnectNode() to netbase methods

Matthew Zipkin · Matthew Zipkin · commit 3a7d6548effa · 2024-03-01T14:47:28.000-05:00
diff --git a/src/i2p.cpp b/src/i2p.cpp
@@ -326,13 +326,9 @@ Session::Reply Session::SendRequestAndGetReply(const Sock& sock,
 
 std::unique_ptr<Sock> Session::Hello() const
 {
-    auto sock = CreateSock(m_control_host.GetSAFamily());
+    auto sock = ConnectDirectly(m_control_host, true);
 
     if (!sock) {
-        throw std::runtime_error("Cannot create socket");
-    }
-
-    if (!ConnectSocketDirectly(m_control_host, *sock, nConnectTimeout, true)) {
         throw std::runtime_error(strprintf("Cannot connect to %s", m_control_host.ToStringAddrPort()));
     }
 
diff --git a/src/net.cpp b/src/net.cpp
@@ -442,7 +442,6 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
     }
 
     // Connect
-    bool connected = false;
     std::unique_ptr<Sock> sock;
     Proxy proxy;
     CAddress addr_bind;
@@ -455,6 +454,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
 
         if (addrConnect.IsI2P() && use_proxy) {
             i2p::Connection conn;
+            bool connected{false};
 
             if (m_i2p_sam_session) {
                 connected = m_i2p_sam_session->Connect(addrConnect, conn, proxyConnectionFailed);
@@ -483,40 +483,25 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
                 addr_bind = CAddress{conn.me, NODE_NONE};
             }
         } else if (use_proxy) {
-            sock = CreateSock(proxy.proxy.GetSAFamily());
-            if (!sock) {
-                return nullptr;
-            }
             LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Using proxy: %s to connect to %s:%s\n", proxy.proxy.ToStringAddrPort(), addrConnect.ToStringAddr(), addrConnect.GetPort());
-            connected = ConnectThroughProxy(proxy, addrConnect.ToStringAddr(), addrConnect.GetPort(),
-                                            *sock, nConnectTimeout, proxyConnectionFailed);
+            sock = ConnectThroughProxy(proxy, addrConnect.ToStringAddr(), addrConnect.GetPort(), proxyConnectionFailed);
         } else {
             // no proxy needed (none set for target network)
-            sock = CreateSock(addrConnect.GetSAFamily());
-            if (!sock) {
-                return nullptr;
-            }
-            connected = ConnectSocketDirectly(addrConnect, *sock, nConnectTimeout,
-                                              conn_type == ConnectionType::MANUAL);
+            sock = ConnectDirectly(addrConnect, conn_type == ConnectionType::MANUAL);
         }
         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)) {
-        sock = CreateSock(proxy.proxy.GetSAFamily());
-        if (!sock) {
-            return nullptr;
-        }
         std::string host;
         uint16_t port{default_port};
         SplitHostPort(std::string(pszDest), port, host);
         bool proxyConnectionFailed;
-        connected = ConnectThroughProxy(proxy, host, port, *sock, nConnectTimeout,
-                                        proxyConnectionFailed);
+        sock = ConnectThroughProxy(proxy, host, port, proxyConnectionFailed);
     }
-    if (!connected) {
+    if (!sock) {
         return nullptr;
     }
 
diff --git a/src/netbase.cpp b/src/netbase.cpp
@@ -517,18 +517,24 @@ static void LogConnectFailure(bool manual_connection, const char* fmt, const Arg
     }
 }
 
-bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nTimeout, bool manual_connection)
+std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connection)
 {
+    auto sock = CreateSock(dest.GetSAFamily());
+    if (!sock) {
+        LogPrintLevel(BCLog::NET, BCLog::Level::Error, "Cannot create a socket for connecting to %s\n", dest.ToStringAddrPort());
+        return {};
+    }
+
     // Create a sockaddr from the specified service.
     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.ToStringAddrPort());
-        return false;
+    if (!dest.GetSockAddr((struct sockaddr*)&sockaddr, &len)) {
+        LogPrintf("Cannot connect to %s: unsupported network\n", dest.ToStringAddrPort());
+        return {};
     }
 
-    // Connect to the addrConnect service on the hSocket socket.
-    if (sock.Connect(reinterpret_cast<struct sockaddr*>(&sockaddr), len) == SOCKET_ERROR) {
+    // Connect to the dest service on the hSocket socket.
+    if (sock->Connect(reinterpret_cast<struct sockaddr*>(&sockaddr), len) == SOCKET_ERROR) {
         int nErr = WSAGetLastError();
         // WSAEINVAL is here because some legacy version of winsock uses it
         if (nErr == WSAEINPROGRESS || nErr == WSAEWOULDBLOCK || nErr == WSAEINVAL)
@@ -538,14 +544,14 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
             // synchronously to check for successful connection with a timeout.
             const Sock::Event requested = Sock::RECV | Sock::SEND;
             Sock::Event occurred;
-            if (!sock.Wait(std::chrono::milliseconds{nTimeout}, requested, &occurred)) {
+            if (!sock->Wait(std::chrono::milliseconds{nConnectTimeout}, requested, &occurred)) {
                 LogPrintf("wait for connect to %s failed: %s\n",
-                          addrConnect.ToStringAddrPort(),
+                          dest.ToStringAddrPort(),
                           NetworkErrorString(WSAGetLastError()));
-                return false;
+                return {};
             } else if (occurred == 0) {
-                LogPrint(BCLog::NET, "connection attempt to %s timed out\n", addrConnect.ToStringAddrPort());
-                return false;
+                LogPrint(BCLog::NET, "connection attempt to %s timed out\n", dest.ToStringAddrPort());
+                return {};
             }
 
             // Even if the wait was successful, the connect might not
@@ -554,17 +560,17 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
             // sockerr here.
             int sockerr;
             socklen_t sockerr_len = sizeof(sockerr);
-            if (sock.GetSockOpt(SOL_SOCKET, SO_ERROR, (sockopt_arg_type)&sockerr, &sockerr_len) ==
+            if (sock->GetSockOpt(SOL_SOCKET, SO_ERROR, (sockopt_arg_type)&sockerr, &sockerr_len) ==
                 SOCKET_ERROR) {
-                LogPrintf("getsockopt() for %s failed: %s\n", addrConnect.ToStringAddrPort(), NetworkErrorString(WSAGetLastError()));
-                return false;
+                LogPrintf("getsockopt() for %s failed: %s\n", dest.ToStringAddrPort(), NetworkErrorString(WSAGetLastError()));
+                return {};
             }
             if (sockerr != 0) {
                 LogConnectFailure(manual_connection,
                                   "connect() to %s failed after wait: %s",
-                                  addrConnect.ToStringAddrPort(),
+                                  dest.ToStringAddrPort(),
                                   NetworkErrorString(sockerr));
-                return false;
+                return {};
             }
         }
 #ifdef WIN32
@@ -573,11 +579,11 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
         else
 #endif
         {
-            LogConnectFailure(manual_connection, "connect() to %s failed: %s", addrConnect.ToStringAddrPort(), NetworkErrorString(WSAGetLastError()));
-            return false;
+            LogConnectFailure(manual_connection, "connect() to %s failed: %s", dest.ToStringAddrPort(), NetworkErrorString(WSAGetLastError()));
+            return {};
         }
     }
-    return true;
+    return sock;
 }
 
 bool SetProxy(enum Network net, const Proxy &addrProxy) {
@@ -628,27 +634,32 @@ bool IsProxy(const CNetAddr &addr) {
     return false;
 }
 
-bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_t port, const Sock& sock, int nTimeout, bool& outProxyConnectionFailed)
+std::unique_ptr<Sock> ConnectThroughProxy(const Proxy& proxy,
+                                          const std::string& dest,
+                                          uint16_t port,
+                                          bool& proxy_connection_failed)
 {
     // first connect to proxy server
-    if (!ConnectSocketDirectly(proxy.proxy, sock, nTimeout, true)) {
-        outProxyConnectionFailed = true;
-        return false;
+    auto sock = ConnectDirectly(proxy.proxy, /*manual_connection=*/true);
+    if (!sock) {
+        proxy_connection_failed = true;
+        return {};
     }
+
     // do socks negotiation
     if (proxy.randomize_credentials) {
         ProxyCredentials random_auth;
         static std::atomic_int counter(0);
         random_auth.username = random_auth.password = strprintf("%i", counter++);
-        if (!Socks5(strDest, port, &random_auth, sock)) {
-            return false;
+        if (!Socks5(dest, port, &random_auth, *sock)) {
+            return {};
         }
     } else {
-        if (!Socks5(strDest, port, nullptr, sock)) {
-            return false;
+        if (!Socks5(dest, port, nullptr, *sock)) {
+            return {};
         }
     }
-    return true;
+    return sock;
 }
 
 CSubNet LookupSubNet(const std::string& subnet_str)
diff --git a/src/netbase.h b/src/netbase.h
@@ -241,35 +241,30 @@ std::unique_ptr<Sock> CreateSockOS(sa_family_t address_family);
 extern std::function<std::unique_ptr<Sock>(const sa_family_t&)> CreateSock;
 
 /**
- * Try to connect to the specified service on the specified socket.
+ * Create a socket and try to connect to the specified service.
  *
- * @param addrConnect The service to which to connect.
- * @param sock The socket on which to connect.
- * @param nTimeout Wait this many milliseconds for the connection to be
- *                 established.
- * @param manual_connection Whether or not the connection was manually requested
- *                          (e.g. through the addnode RPC)
+ * @param[in] dest The service to which to connect.
+ * @param[in] manual_connection Whether or not the connection was manually requested (e.g. through the addnode RPC)
  *
- * @returns Whether or not a connection was successfully made.
+ * @returns the connected socket if the operation succeeded, empty unique_ptr otherwise
  */
-bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nTimeout, bool manual_connection);
+std::unique_ptr<Sock> ConnectDirectly(const CService& dest, bool manual_connection);
 
 /**
  * Connect to a specified destination service through a SOCKS5 proxy by first
  * connecting to the SOCKS5 proxy.
  *
- * @param proxy The SOCKS5 proxy.
- * @param strDest The destination service to which to connect.
- * @param port The destination port.
- * @param sock The socket on which to connect to the SOCKS5 proxy.
- * @param nTimeout Wait this many milliseconds for the connection to the SOCKS5
- *                 proxy to be established.
- * @param[out] outProxyConnectionFailed Whether or not the connection to the
- *                                      SOCKS5 proxy failed.
+ * @param[in] proxy The SOCKS5 proxy.
+ * @param[in] dest The destination service to which to connect.
+ * @param[in] port The destination port.
+ * @param[out] proxy_connection_failed Whether or not the connection to the SOCKS5 proxy failed.
  *
- * @returns Whether or not the operation succeeded.
+ * @returns the connected socket if the operation succeeded. Otherwise an empty unique_ptr.
  */
-bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_t port, const Sock& sock, int nTimeout, bool& outProxyConnectionFailed);
+std::unique_ptr<Sock> ConnectThroughProxy(const Proxy& proxy,
+                                          const std::string& dest,
+                                          uint16_t port,
+                                          bool& proxy_connection_failed);
 
 /**
  * Interrupt SOCKS5 reads or writes.

Original file line number	Diff line number	Diff line change
`@@ -326,13 +326,9 @@ Session::Reply Session::SendRequestAndGetReply(const Sock& sock,`
`326`	`326`
`327`	`327`	`std::unique_ptr<Sock> Session::Hello() const`
`328`	`328`	`{`
`329`		`- auto sock = CreateSock(m_control_host.GetSAFamily());`
	`329`	`+ auto sock = ConnectDirectly(m_control_host, true);`
`330`	`330`
`331`	`331`	`if (!sock) {`
`332`		`- throw std::runtime_error("Cannot create socket");`
`333`		`- }`
`334`		`-`
`335`		`- if (!ConnectSocketDirectly(m_control_host, *sock, nConnectTimeout, true)) {`
`336`	`332`	`throw std::runtime_error(strprintf("Cannot connect to %s", m_control_host.ToStringAddrPort()));`
`337`	`333`	`}`
`338`	`334`