netbase: use reliable send() during SOCKS5 handshake

`send(2)` can be interrupted or for another reason it may not fully
complete sending all the bytes. We should be ready to retry the send
with the remaining bytes. This is what `Sock::SendComplete()` does,
thus use it in `Socks5()`.

Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
change also the recv part of `Socks5()` to use `CThreadInterrupt`
instead of a boolean.

Easier reviewed with `git show -b` (ignore white-space changes).

Author: vasild

src/net.cpp

@@ -3260,7 +3260,6 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
     // Start threads
     //
     assert(m_msgproc);
-    InterruptSocks5(false);
     interruptNet.reset();
     flagInterruptMsgProc = false;
 
@@ -3332,7 +3331,7 @@ void CConnman::Interrupt()
     condMsgProc.notify_all();
 
     interruptNet();
-    InterruptSocks5(true);
+    g_socks5_interrupt();
 
     if (semOutbound) {
         for (int i=0; i<m_max_outbound; i++) {

src/netbase.cpp

@@ -30,7 +30,7 @@ bool fNameLookup = DEFAULT_NAME_LOOKUP;
 
 // Need ample time for negotiation for very slow proxies such as Tor
 std::chrono::milliseconds g_socks5_recv_timeout = 20s;
-static std::atomic<bool> interruptSocks5Recv(false);
+CThreadInterrupt g_socks5_interrupt;
 
 std::vector<CNetAddr> WrappedGetAddrInfo(const std::string& name, bool allow_lookup)
 {
@@ -269,7 +269,7 @@ enum class IntrRecvError {
  *          IntrRecvError::OK only if all of the specified number of bytes were
  *          read.
  *
- * @see This function can be interrupted by calling InterruptSocks5(bool).
+ * @see This function can be interrupted by calling g_socks5_interrupt().
  *      Sockets can be made non-blocking with Sock::SetNonBlocking().
  */
 static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, std::chrono::milliseconds timeout, const Sock& sock)
@@ -297,8 +297,9 @@ static IntrRecvError InterruptibleRecv(uint8_t* data, size_t len, std::chrono::m
                 return IntrRecvError::NetworkError;
             }
         }
-        if (interruptSocks5Recv)
+        if (g_socks5_interrupt) {
             return IntrRecvError::Interrupted;
+        }
         curTime = Now<SteadyMilliseconds>();
     }
     return len == 0 ? IntrRecvError::OK : IntrRecvError::Timeout;
@@ -331,103 +332,93 @@ static std::string Socks5ErrorString(uint8_t err)
 
 bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* auth, const Sock& sock)
 {
-    IntrRecvError recvr;
-    LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest);
-    if (strDest.size() > 255) {
-        return error("Hostname too long");
-    }
-    // Construct the version identifier/method selection message
-    std::vector<uint8_t> vSocks5Init;
-    vSocks5Init.push_back(SOCKSVersion::SOCKS5); // We want the SOCK5 protocol
-    if (auth) {
-        vSocks5Init.push_back(0x02); // 2 method identifiers follow...
-        vSocks5Init.push_back(SOCKS5Method::NOAUTH);
-        vSocks5Init.push_back(SOCKS5Method::USER_PASS);
-    } else {
-        vSocks5Init.push_back(0x01); // 1 method identifier follows...
-        vSocks5Init.push_back(SOCKS5Method::NOAUTH);
-    }
-    ssize_t ret = sock.Send(vSocks5Init.data(), vSocks5Init.size(), MSG_NOSIGNAL);
-    if (ret != (ssize_t)vSocks5Init.size()) {
-        return error("Error sending to proxy");
-    }
-    uint8_t pchRet1[2];
-    if (InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
-        LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port);
-        return false;
-    }
-    if (pchRet1[0] != SOCKSVersion::SOCKS5) {
-        return error("Proxy failed to initialize");
-    }
-    if (pchRet1[1] == SOCKS5Method::USER_PASS && auth) {
-        // Perform username/password authentication (as described in RFC1929)
-        std::vector<uint8_t> vAuth;
-        vAuth.push_back(0x01); // Current (and only) version of user/pass subnegotiation
-        if (auth->username.size() > 255 || auth->password.size() > 255)
-            return error("Proxy username or password too long");
-        vAuth.push_back(auth->username.size());
-        vAuth.insert(vAuth.end(), auth->username.begin(), auth->username.end());
-        vAuth.push_back(auth->password.size());
-        vAuth.insert(vAuth.end(), auth->password.begin(), auth->password.end());
-        ret = sock.Send(vAuth.data(), vAuth.size(), MSG_NOSIGNAL);
-        if (ret != (ssize_t)vAuth.size()) {
-            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 (InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
-            return error("Error reading proxy authentication response");
+    try {
+        IntrRecvError recvr;
+        LogPrint(BCLog::NET, "SOCKS5 connecting %s\n", strDest);
+        if (strDest.size() > 255) {
+            return error("Hostname too long");
         }
-        if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) {
-            return error("Proxy authentication unsuccessful");
+        // Construct the version identifier/method selection message
+        std::vector<uint8_t> vSocks5Init;
+        vSocks5Init.push_back(SOCKSVersion::SOCKS5); // We want the SOCK5 protocol
+        if (auth) {
+            vSocks5Init.push_back(0x02); // 2 method identifiers follow...
+            vSocks5Init.push_back(SOCKS5Method::NOAUTH);
+            vSocks5Init.push_back(SOCKS5Method::USER_PASS);
+        } else {
+            vSocks5Init.push_back(0x01); // 1 method identifier follows...
+            vSocks5Init.push_back(SOCKS5Method::NOAUTH);
         }
-    } else if (pchRet1[1] == SOCKS5Method::NOAUTH) {
-        // Perform no authentication
-    } else {
-        return error("Proxy requested wrong authentication method %02x", pchRet1[1]);
-    }
-    std::vector<uint8_t> vSocks5;
-    vSocks5.push_back(SOCKSVersion::SOCKS5); // VER protocol version
-    vSocks5.push_back(SOCKS5Command::CONNECT); // CMD CONNECT
-    vSocks5.push_back(0x00); // RSV Reserved must be 0
-    vSocks5.push_back(SOCKS5Atyp::DOMAINNAME); // ATYP DOMAINNAME
-    vSocks5.push_back(strDest.size()); // Length<=255 is checked at beginning of function
-    vSocks5.insert(vSocks5.end(), strDest.begin(), strDest.end());
-    vSocks5.push_back((port >> 8) & 0xFF);
-    vSocks5.push_back((port >> 0) & 0xFF);
-    ret = sock.Send(vSocks5.data(), vSocks5.size(), MSG_NOSIGNAL);
-    if (ret != (ssize_t)vSocks5.size()) {
-        return error("Error sending to proxy");
-    }
-    uint8_t pchRet2[4];
-    if ((recvr = InterruptibleRecv(pchRet2, 4, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
-        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
-             * error message. */
+        sock.SendComplete(vSocks5Init, g_socks5_recv_timeout, g_socks5_interrupt);
+        uint8_t pchRet1[2];
+        if (InterruptibleRecv(pchRet1, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
+            LogPrintf("Socks5() connect to %s:%d failed: InterruptibleRecv() timeout or other failure\n", strDest, port);
             return false;
+        }
+        if (pchRet1[0] != SOCKSVersion::SOCKS5) {
+            return error("Proxy failed to initialize");
+        }
+        if (pchRet1[1] == SOCKS5Method::USER_PASS && auth) {
+            // Perform username/password authentication (as described in RFC1929)
+            std::vector<uint8_t> vAuth;
+            vAuth.push_back(0x01); // Current (and only) version of user/pass subnegotiation
+            if (auth->username.size() > 255 || auth->password.size() > 255)
+                return error("Proxy username or password too long");
+            vAuth.push_back(auth->username.size());
+            vAuth.insert(vAuth.end(), auth->username.begin(), auth->username.end());
+            vAuth.push_back(auth->password.size());
+            vAuth.insert(vAuth.end(), auth->password.begin(), auth->password.end());
+            sock.SendComplete(vAuth, g_socks5_recv_timeout, g_socks5_interrupt);
+            LogPrint(BCLog::PROXY, "SOCKS5 sending proxy authentication %s:%s\n", auth->username, auth->password);
+            uint8_t pchRetA[2];
+            if (InterruptibleRecv(pchRetA, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
+                return error("Error reading proxy authentication response");
+            }
+            if (pchRetA[0] != 0x01 || pchRetA[1] != 0x00) {
+                return error("Proxy authentication unsuccessful");
+            }
+        } else if (pchRet1[1] == SOCKS5Method::NOAUTH) {
+            // Perform no authentication
         } else {
-            return error("Error while reading proxy response");
+            return error("Proxy requested wrong authentication method %02x", pchRet1[1]);
         }
-    }
-    if (pchRet2[0] != SOCKSVersion::SOCKS5) {
-        return error("Proxy failed to accept request");
-    }
-    if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
-        // Failures to connect to a peer that are not proxy errors
-        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
-        return error("Error: malformed proxy response");
-    }
-    uint8_t pchRet3[256];
-    switch (pchRet2[3])
-    {
+        std::vector<uint8_t> vSocks5;
+        vSocks5.push_back(SOCKSVersion::SOCKS5);   // VER protocol version
+        vSocks5.push_back(SOCKS5Command::CONNECT); // CMD CONNECT
+        vSocks5.push_back(0x00);                   // RSV Reserved must be 0
+        vSocks5.push_back(SOCKS5Atyp::DOMAINNAME); // ATYP DOMAINNAME
+        vSocks5.push_back(strDest.size());         // Length<=255 is checked at beginning of function
+        vSocks5.insert(vSocks5.end(), strDest.begin(), strDest.end());
+        vSocks5.push_back((port >> 8) & 0xFF);
+        vSocks5.push_back((port >> 0) & 0xFF);
+        sock.SendComplete(vSocks5, g_socks5_recv_timeout, g_socks5_interrupt);
+        uint8_t pchRet2[4];
+        if ((recvr = InterruptibleRecv(pchRet2, 4, g_socks5_recv_timeout, sock)) != IntrRecvError::OK) {
+            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
+                 * error message. */
+                return false;
+            } else {
+                return error("Error while reading proxy response");
+            }
+        }
+        if (pchRet2[0] != SOCKSVersion::SOCKS5) {
+            return error("Proxy failed to accept request");
+        }
+        if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
+            // Failures to connect to a peer that are not proxy errors
+            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
+            return error("Error: malformed proxy response");
+        }
+        uint8_t pchRet3[256];
+        switch (pchRet2[3]) {
         case SOCKS5Atyp::IPV4: recvr = InterruptibleRecv(pchRet3, 4, g_socks5_recv_timeout, sock); break;
         case SOCKS5Atyp::IPV6: recvr = InterruptibleRecv(pchRet3, 16, g_socks5_recv_timeout, sock); break;
-        case SOCKS5Atyp::DOMAINNAME:
-        {
+        case SOCKS5Atyp::DOMAINNAME: {
             recvr = InterruptibleRecv(pchRet3, 1, g_socks5_recv_timeout, sock);
             if (recvr != IntrRecvError::OK) {
                 return error("Error reading from proxy");
@@ -437,15 +428,18 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
             break;
         }
         default: return error("Error: malformed proxy response");
+        }
+        if (recvr != IntrRecvError::OK) {
+            return error("Error reading from proxy");
+        }
+        if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
+            return error("Error reading from proxy");
+        }
+        LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest);
+        return true;
+    } catch (const std::runtime_error& e) {
+        return error("Error during SOCKS5 proxy handshake: %s", e.what());
     }
-    if (recvr != IntrRecvError::OK) {
-        return error("Error reading from proxy");
-    }
-    if (InterruptibleRecv(pchRet3, 2, g_socks5_recv_timeout, sock) != IntrRecvError::OK) {
-        return error("Error reading from proxy");
-    }
-    LogPrint(BCLog::NET, "SOCKS5 connected %s\n", strDest);
-    return true;
 }
 
 std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
@@ -678,11 +672,6 @@ bool LookupSubNet(const std::string& subnet_str, CSubNet& subnet_out)
     return false;
 }
 
-void InterruptSocks5(bool interrupt)
-{
-    interruptSocks5Recv = interrupt;
-}
-
 bool IsBadPort(uint16_t port)
 {
     /* Don't forget to update doc/p2p-bad-ports.md if you change this list. */

src/netbase.h

@@ -13,6 +13,7 @@
 #include <netaddress.h>
 #include <serialize.h>
 #include <util/sock.h>
+#include <util/threadinterrupt.h>
 
 #include <functional>
 #include <memory>
@@ -220,7 +221,10 @@ bool ConnectSocketDirectly(const CService &addrConnect, const Sock& sock, int nT
  */
 bool ConnectThroughProxy(const Proxy& proxy, const std::string& strDest, uint16_t port, const Sock& sock, int nTimeout, bool& outProxyConnectionFailed);
 
-void InterruptSocks5(bool interrupt);
+/**
+ * Interrupt SOCKS5 reads or writes.
+ */
+extern CThreadInterrupt g_socks5_interrupt;
 
 /**
  * Connect to a specified destination service through an already connected

src/test/fuzz/socks5.cpp

@@ -32,7 +32,9 @@ FUZZ_TARGET(socks5, .init = initialize_socks5)
     ProxyCredentials proxy_credentials;
     proxy_credentials.username = fuzzed_data_provider.ConsumeRandomLengthString(512);
     proxy_credentials.password = fuzzed_data_provider.ConsumeRandomLengthString(512);
-    InterruptSocks5(fuzzed_data_provider.ConsumeBool());
+    if (fuzzed_data_provider.ConsumeBool()) {
+        g_socks5_interrupt();
+    }
     // Set FUZZED_SOCKET_FAKE_LATENCY=1 to exercise recv timeout code paths. This
     // will slow down fuzzing.
     g_socks5_recv_timeout = (fuzzed_data_provider.ConsumeBool() && std::getenv("FUZZED_SOCKET_FAKE_LATENCY") != nullptr) ? 1ms : default_socks5_recv_timeout;