socks: make the connect phase non-blocking

Removes two entries from KNOWN_BUGS.

Closes #4907

Author: bagder

docs/KNOWN_BUGS

@@ -87,8 +87,6 @@ problems may have been fixed or changed somewhat since this was written!
  9.1 SFTP doesn't do CURLOPT_POSTQUOTE correct
 
  10. SOCKS
- 10.1 SOCKS proxy connections are done blocking
- 10.2 SOCKS don't support timeouts
  10.3 FTPS over SOCKS
  10.4 active FTP over a SOCKS
 
@@ -621,21 +619,6 @@ problems may have been fixed or changed somewhat since this was written!
 
 10. SOCKS
 
-10.1 SOCKS proxy connections are done blocking
-
- Both SOCKS5 and SOCKS4 proxy connections are done blocking, which is very bad
- when used with the multi interface.
-
-10.2 SOCKS don't support timeouts
-
- The SOCKS4 connection codes don't properly acknowledge (connect) timeouts.
- According to bug #1556528, even the SOCKS5 connect code does not do it right:
- https://curl.haxx.se/bug/view.cgi?id=604
-
- When connecting to a SOCK proxy, the (connect) timeout is not properly
- acknowledged after the actual TCP connect (during the SOCKS "negotiate"
- phase).
-
 10.3 FTPS over SOCKS
 
  libcurl doesn't support FTPS over a SOCKS proxy.

docs/TODO

@@ -405,7 +405,6 @@
  EWOULDBLOCK or similar. Blocking cases include:
 
  - Name resolves on non-windows unless c-ares or the threaded resolver is used
- - SOCKS proxy handshakes
  - file:// transfers
  - TELNET transfers
  - The "DONE" operation (post transfer protocol-specific actions) for the

lib/connect.c

@@ -745,58 +745,82 @@ void Curl_updateconninfo(struct connectdata *conn, curl_socket_t sockfd)
   Curl_persistconninfo(conn);
 }
 
-/* after a TCP connection to the proxy has been verified, this function does
-   the next magic step.
+/* After a TCP connection to the proxy has been verified, this function does
+   the next magic steps. If 'done' isn't set TRUE, it is not done yet and
+   must be called again.
 
    Note: this function's sub-functions call failf()
 
 */
-static CURLcode connected_proxy(struct connectdata *conn, int sockindex)
+static CURLcode connect_SOCKS(struct connectdata *conn, int sockindex,
+                              bool *done)
 {
   CURLcode result = CURLE_OK;
 
+  infof(conn->data, "connect_SOCKS is called [socks state %d]\n",
+        conn->cnnct.state);
+
   if(conn->bits.socksproxy) {
 #ifndef CURL_DISABLE_PROXY
     /* for the secondary socket (FTP), use the "connect to host"
      * but ignore the "connect to port" (use the secondary port)
      */
-    const char * const host = conn->bits.httpproxy ?
-                              conn->http_proxy.host.name :
-                              conn->bits.conn_to_host ?
-                              conn->conn_to_host.name :
-                              sockindex == SECONDARYSOCKET ?
-                              conn->secondaryhostname : conn->host.name;
-    const int port = conn->bits.httpproxy ? (int)conn->http_proxy.port :
-                     sockindex == SECONDARYSOCKET ? conn->secondary_port :
-                     conn->bits.conn_to_port ? conn->conn_to_port :
-                     conn->remote_port;
-    conn->bits.socksproxy_connecting = TRUE;
+    const char * const host =
+      conn->bits.httpproxy ?
+      conn->http_proxy.host.name :
+      conn->bits.conn_to_host ?
+      conn->conn_to_host.name :
+      sockindex == SECONDARYSOCKET ?
+      conn->secondaryhostname : conn->host.name;
+    const int port =
+      conn->bits.httpproxy ? (int)conn->http_proxy.port :
+      sockindex == SECONDARYSOCKET ? conn->secondary_port :
+      conn->bits.conn_to_port ? conn->conn_to_port :
+      conn->remote_port;
     switch(conn->socks_proxy.proxytype) {
     case CURLPROXY_SOCKS5:
     case CURLPROXY_SOCKS5_HOSTNAME:
       result = Curl_SOCKS5(conn->socks_proxy.user, conn->socks_proxy.passwd,
-                         host, port, sockindex, conn);
+                           host, port, sockindex, conn, done);
       break;
 
     case CURLPROXY_SOCKS4:
     case CURLPROXY_SOCKS4A:
       result = Curl_SOCKS4(conn->socks_proxy.user, host, port, sockindex,
-                           conn);
+                           conn, done);
       break;
 
     default:
       failf(conn->data, "unknown proxytype option given");
       result = CURLE_COULDNT_CONNECT;
     } /* switch proxytype */
-    conn->bits.socksproxy_connecting = FALSE;
 #else
   (void)sockindex;
 #endif /* CURL_DISABLE_PROXY */
   }
+  else
+    *done = TRUE; /* no SOCKS proxy, so consider us connected */
 
   return result;
 }
 
+/*
+ * post_SOCKS() is called after a successful connect to the peer, which
+ * *could* be a SOCKS proxy
+ */
+static void post_SOCKS(struct connectdata *conn,
+                       int sockindex,
+                       bool *connected)
+{
+  conn->bits.tcpconnect[sockindex] = TRUE;
+
+  *connected = TRUE;
+  if(sockindex == FIRSTSOCKET)
+    Curl_pgrsTime(conn->data, TIMER_CONNECT); /* connect done */
+  Curl_updateconninfo(conn, conn->sock[sockindex]);
+  Curl_verboseconnect(conn);
+}
+
 /*
  * Curl_is_connected() checks if the socket has connected.
  */
@@ -834,6 +858,14 @@ CURLcode Curl_is_connected(struct connectdata *conn,
     return CURLE_OPERATION_TIMEDOUT;
   }
 
+  if(SOCKS_STATE(conn->cnnct.state)) {
+    /* still doing SOCKS */
+    result = connect_SOCKS(conn, sockindex, connected);
+    if(!result && *connected)
+      post_SOCKS(conn, sockindex, connected);
+    return result;
+  }
+
   for(i = 0; i<2; i++) {
     const int other = i ^ 1;
     if(conn->tempsock[i] == CURL_SOCKET_BAD)
@@ -900,18 +932,13 @@ CURLcode Curl_is_connected(struct connectdata *conn,
           conn->tempsock[other] = CURL_SOCKET_BAD;
         }
 
-        /* see if we need to do any proxy magic first once we connected */
-        result = connected_proxy(conn, sockindex);
-        if(result)
+        /* see if we need to kick off any SOCKS proxy magic once we
+           connected */
+        result = connect_SOCKS(conn, sockindex, connected);
+        if(result || !*connected)
           return result;
 
-        conn->bits.tcpconnect[sockindex] = TRUE;
-
-        *connected = TRUE;
-        if(sockindex == FIRSTSOCKET)
-          Curl_pgrsTime(data, TIMER_CONNECT); /* connect done */
-        Curl_updateconninfo(conn, conn->sock[sockindex]);
-        Curl_verboseconnect(conn);
+        post_SOCKS(conn, sockindex, connected);
 
         return CURLE_OK;
       }

lib/ftp.c

@@ -55,7 +55,6 @@
 #include "transfer.h"
 #include "escape.h"
 #include "http.h" /* for HTTP proxy tunnel stuff */
-#include "socks.h"
 #include "ftp.h"
 #include "fileinfo.h"
 #include "ftplistparser.h"
@@ -78,6 +77,7 @@
 #include "warnless.h"
 #include "http_proxy.h"
 #include "non-ascii.h"
+#include "socks.h"
 /* The last 3 #include files should be in this order */
 #include "curl_printf.h"
 #include "curl_memory.h"
@@ -810,6 +810,9 @@ static int ftp_domore_getsock(struct connectdata *conn, curl_socket_t *socks)
    * handle ordinary commands.
    */
 
+  if(SOCKS_STATE(conn->cnnct.state))
+    return Curl_SOCKS_getsock(conn, socks, SECONDARYSOCKET);
+
   if(FTP_STOP == ftpc->state) {
     int bits = GETSOCK_READSOCK(0);
 
@@ -919,7 +922,7 @@ static CURLcode ftp_state_use_port(struct connectdata *conn,
   struct sockaddr_in6 * const sa6 = (void *)sa;
 #endif
   static const char mode[][5] = { "EPRT", "PORT" };
-  int rc;
+  enum resolve_t rc;
   int error;
   char *host = NULL;
   char *string_ftpport = data->set.str[STRING_FTPPORT];
@@ -1794,7 +1797,7 @@ static CURLcode ftp_state_pasv_resp(struct connectdata *conn,
   CURLcode result;
   struct Curl_easy *data = conn->data;
   struct Curl_dns_entry *addr = NULL;
-  int rc;
+  enum resolve_t rc;
   unsigned short connectport; /* the local port connect() should use! */
   char *str = &data->state.buffer[4];  /* start on the first letter */
 

lib/hostip.c

@@ -483,16 +483,16 @@ Curl_cache_addr(struct Curl_easy *data,
  * CURLRESOLV_PENDING  (1) = waiting for response, no pointer
  */
 
-int Curl_resolv(struct connectdata *conn,
-                const char *hostname,
-                int port,
-                bool allowDOH,
-                struct Curl_dns_entry **entry)
+enum resolve_t Curl_resolv(struct connectdata *conn,
+                           const char *hostname,
+                           int port,
+                           bool allowDOH,
+                           struct Curl_dns_entry **entry)
 {
   struct Curl_dns_entry *dns = NULL;
   struct Curl_easy *data = conn->data;
   CURLcode result;
-  int rc = CURLRESOLV_ERROR; /* default to failure */
+  enum resolve_t rc = CURLRESOLV_ERROR; /* default to failure */
 
   *entry = NULL;
 
@@ -642,11 +642,11 @@ RETSIGTYPE alarmfunc(int sig)
  * CURLRESOLV_PENDING  (1) = waiting for response, no pointer
  */
 
-int Curl_resolv_timeout(struct connectdata *conn,
-                        const char *hostname,
-                        int port,
-                        struct Curl_dns_entry **entry,
-                        timediff_t timeoutms)
+enum resolve_t Curl_resolv_timeout(struct connectdata *conn,
+                                   const char *hostname,
+                                   int port,
+                                   struct Curl_dns_entry **entry,
+                                   timediff_t timeoutms)
 {
 #ifdef USE_ALARM_TIMEOUT
 #ifdef HAVE_SIGACTION
@@ -662,7 +662,7 @@ int Curl_resolv_timeout(struct connectdata *conn,
   volatile unsigned int prev_alarm = 0;
   struct Curl_easy *data = conn->data;
 #endif /* USE_ALARM_TIMEOUT */
-  int rc;
+  enum resolve_t rc;
 
   *entry = NULL;
 

lib/hostip.h

@@ -79,18 +79,21 @@ struct Curl_dns_entry {
  * use, or we'll leak memory!
  */
 /* return codes */
-#define CURLRESOLV_TIMEDOUT -2
-#define CURLRESOLV_ERROR    -1
-#define CURLRESOLV_RESOLVED  0
-#define CURLRESOLV_PENDING   1
-int Curl_resolv(struct connectdata *conn,
-                const char *hostname,
-                int port,
-                bool allowDOH,
-                struct Curl_dns_entry **dnsentry);
-int Curl_resolv_timeout(struct connectdata *conn, const char *hostname,
-                        int port, struct Curl_dns_entry **dnsentry,
-                        timediff_t timeoutms);
+enum resolve_t {
+  CURLRESOLV_TIMEDOUT = -2,
+  CURLRESOLV_ERROR    = -1,
+  CURLRESOLV_RESOLVED =  0,
+  CURLRESOLV_PENDING  =  1
+};
+enum resolve_t Curl_resolv(struct connectdata *conn,
+                           const char *hostname,
+                           int port,
+                           bool allowDOH,
+                           struct Curl_dns_entry **dnsentry);
+enum resolve_t Curl_resolv_timeout(struct connectdata *conn,
+                                   const char *hostname, int port,
+                                   struct Curl_dns_entry **dnsentry,
+                                   timediff_t timeoutms);
 
 #ifdef CURLRES_IPV6
 /*

lib/multi.c

@@ -47,6 +47,7 @@
 #include "http_proxy.h"
 #include "http2.h"
 #include "socketpair.h"
+#include "socks.h"
 /* The last 3 #include files should be in this order */
 #include "curl_printf.h"
 #include "curl_memory.h"
@@ -856,6 +857,9 @@ static int waitconnect_getsock(struct connectdata *conn,
     return Curl_ssl_getsock(conn, sock);
 #endif
 
+  if(SOCKS_STATE(conn->cnnct.state))
+    return Curl_SOCKS_getsock(conn, sock, FIRSTSOCKET);
+
   for(i = 0; i<2; i++) {
     if(conn->tempsock[i] != CURL_SOCKET_BAD) {
       sock[s] = conn->tempsock[i];

lib/sendf.c

@@ -692,19 +692,20 @@ CURLcode Curl_read_plain(curl_socket_t sockfd,
   ssize_t nread = sread(sockfd, buf, bytesfromsocket);
 
   if(-1 == nread) {
-    int err = SOCKERRNO;
-    int return_error;
+    const int err = SOCKERRNO;
+    const bool return_error =
 #ifdef USE_WINSOCK
-    return_error = WSAEWOULDBLOCK == err;
+      WSAEWOULDBLOCK == err
 #else
-    return_error = EWOULDBLOCK == err || EAGAIN == err || EINTR == err;
+      EWOULDBLOCK == err || EAGAIN == err || EINTR == err
 #endif
+      ;
+    *n = 0; /* no data returned */
     if(return_error)
       return CURLE_AGAIN;
     return CURLE_RECV_ERROR;
   }
 
-  /* we only return number of bytes read when we return OK */
   *n = nread;
   return CURLE_OK;
 }