socks: make the connect phase non-blocking

Removes two entries from KNOWN_BUGS.

Closes #4907

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
+/* After a TCP connection to the proxy has been verified, this function does
-   the next magic step.
+   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 ?
+    const char * const host =
-                              conn->http_proxy.host.name :
+      conn->bits.httpproxy ?
-                              conn->bits.conn_to_host ?
+      conn->http_proxy.host.name :
-                              conn->conn_to_host.name :
+      conn->bits.conn_to_host ?
-                              sockindex == SECONDARYSOCKET ?
+      conn->conn_to_host.name :
-                              conn->secondaryhostname : conn->host.name;
+      sockindex == SECONDARYSOCKET ?
-    const int port = conn->bits.httpproxy ? (int)conn->http_proxy.port :
+      conn->secondaryhostname : conn->host.name;
-                     sockindex == SECONDARYSOCKET ? conn->secondary_port :
+    const int port =
-                     conn->bits.conn_to_port ? conn->conn_to_port :
+      conn->bits.httpproxy ? (int)conn->http_proxy.port :
-                     conn->remote_port;
+      sockindex == SECONDARYSOCKET ? conn->secondary_port :
-    conn->bits.socksproxy_connecting = TRUE;
+      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 */
+        /* see if we need to kick off any SOCKS proxy magic once we
-        result = connected_proxy(conn, sockindex);
+           connected */
-        if(result)
+        result = connect_SOCKS(conn, sockindex, connected);
+        if(result || !*connected)
           return result;
 
-        conn->bits.tcpconnect[sockindex] = TRUE;
+        post_SOCKS(conn, sockindex, connected);
-
-        *connected = TRUE;
-        if(sockindex == FIRSTSOCKET)
-          Curl_pgrsTime(data, TIMER_CONNECT); /* connect done */
-        Curl_updateconninfo(conn, conn->sock[sockindex]);
-        Curl_verboseconnect(conn);
 
         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,
+enum resolve_t Curl_resolv(struct connectdata *conn,
-                const char *hostname,
+                           const char *hostname,
-                int port,
+                           int port,
-                bool allowDOH,
+                           bool allowDOH,
-                struct Curl_dns_entry **entry)
+                           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,
+enum resolve_t Curl_resolv_timeout(struct connectdata *conn,
-                        const char *hostname,
+                                   const char *hostname,
-                        int port,
+                                   int port,
-                        struct Curl_dns_entry **entry,
+                                   struct Curl_dns_entry **entry,
-                        timediff_t timeoutms)
+                                   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
+enum resolve_t {
-#define CURLRESOLV_ERROR    -1
+  CURLRESOLV_TIMEDOUT = -2,
-#define CURLRESOLV_RESOLVED  0
+  CURLRESOLV_ERROR    = -1,
-#define CURLRESOLV_PENDING   1
+  CURLRESOLV_RESOLVED =  0,
-int Curl_resolv(struct connectdata *conn,
+  CURLRESOLV_PENDING  =  1
-                const char *hostname,
+};
-                int port,
+enum resolve_t Curl_resolv(struct connectdata *conn,
-                bool allowDOH,
+                           const char *hostname,
-                struct Curl_dns_entry **dnsentry);
+                           int port,
-int Curl_resolv_timeout(struct connectdata *conn, const char *hostname,
+                           bool allowDOH,
-                        int port, struct Curl_dns_entry **dnsentry,
+                           struct Curl_dns_entry **dnsentry);
-                        timediff_t timeoutms);
+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;
+    const int err = SOCKERRNO;
-    int return_error;
+    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;
 }

lib/socks.h

@@ -7,7 +7,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 1998 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 1998 - 2020, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -27,13 +27,13 @@
 #ifdef CURL_DISABLE_PROXY
 #define Curl_SOCKS4(a,b,c,d,e) CURLE_NOT_BUILT_IN
 #define Curl_SOCKS5(a,b,c,d,e,f) CURLE_NOT_BUILT_IN
+#define Curl_SOCKS_getsock(x,y,z) 0
 #else
 /*
  * Helper read-from-socket functions. Does the same as Curl_read() but it
  * blocks until all bytes amount of buffersize will be read. No more, no less.
  *
- * This is STUPID BLOCKING behaviour which we frown upon, but right now this
+ * This is STUPID BLOCKING behavior
- * is what we have...
  */
 int Curl_blockread_all(struct connectdata *conn,
                        curl_socket_t sockfd,
@@ -41,6 +41,9 @@ int Curl_blockread_all(struct connectdata *conn,
                        ssize_t buffersize,
                        ssize_t *n);
 
+int Curl_SOCKS_getsock(struct connectdata *conn,
+                       curl_socket_t *sock,
+                       int sockindex);
 /*
  * This function logs in to a SOCKS4(a) proxy and sends the specifics to the
  * final destination server.
@@ -49,7 +52,8 @@ CURLcode Curl_SOCKS4(const char *proxy_name,
                      const char *hostname,
                      int remote_port,
                      int sockindex,
-                     struct connectdata *conn);
+                     struct connectdata *conn,
+                     bool *done);
 
 /*
  * This function logs in to a SOCKS5 proxy and sends the specifics to the
@@ -60,7 +64,8 @@ CURLcode Curl_SOCKS5(const char *proxy_name,
                      const char *hostname,
                      int remote_port,
                      int sockindex,
-                     struct connectdata *conn);
+                     struct connectdata *conn,
+                     bool *done);
 
 #if defined(HAVE_GSSAPI) || defined(USE_WINDOWS_SSPI)
 /*

lib/socks_gssapi.c

@@ -5,8 +5,8 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
+ * Copyright (C) 2012 - 2020, Daniel Stenberg, <daniel@haxx.se>, et al.
  * Copyright (C) 2009, Markus Moeller, <markus_moeller@compuserve.com>
- * Copyright (C) 2012 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.
  *
  * This software is licensed as described in the file COPYING, which
  * you should have received as part of this distribution. The terms
@@ -167,6 +167,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(int sockindex,
     return CURLE_COULDNT_CONNECT;
   }
 
+  (void)curlx_nonblock(sock, FALSE);
+
   /* As long as we need to keep sending some context info, and there's no  */
   /* errors, keep sending it...                                            */
   for(;;) {
@@ -513,6 +515,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(int sockindex,
     gss_release_buffer(&gss_status, &gss_recv_token);
   }
 
+  (void)curlx_nonblock(sock, TRUE);
+
   infof(data, "SOCKS5 access with%s protection granted.\n",
         (socksreq[0] == 0)?"out GSS-API data":
         ((socksreq[0] == 1)?" GSS-API integrity":" GSS-API confidentiality"));

lib/socks_sspi.c

@@ -5,7 +5,7 @@
  *                            | (__| |_| |  _ <| |___
  *                             \___|\___/|_| \_\_____|
  *
- * Copyright (C) 2012 - 2019, Daniel Stenberg, <daniel@haxx.se>, et al.
+ * Copyright (C) 2012 - 2020, Daniel Stenberg, <daniel@haxx.se>, et al.
  * Copyright (C) 2009, 2011, Markus Moeller, <markus_moeller@compuserve.com>
  *
  * This software is licensed as described in the file COPYING, which
@@ -153,6 +153,8 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(int sockindex,
     return CURLE_COULDNT_CONNECT;
   }
 
+  (void)curlx_nonblock(sock, FALSE);
+
   /* As long as we need to keep sending some context info, and there's no  */
   /* errors, keep sending it...                                            */
   for(;;) {
@@ -587,6 +589,7 @@ CURLcode Curl_SOCKS5_gssapi_negotiate(int sockindex,
     memcpy(socksreq, sspi_w_token[0].pvBuffer, sspi_w_token[0].cbBuffer);
     s_pSecFn->FreeContextBuffer(sspi_w_token[0].pvBuffer);
   }
+  (void)curlx_nonblock(sock, TRUE);
 
   infof(data, "SOCKS5 access with%s protection granted.\n",
         (socksreq[0] == 0)?"out GSS-API data":

lib/urldata.h

@@ -476,7 +476,6 @@ struct ConnectBits {
   BIT(tcp_fastopen); /* use TCP Fast Open */
   BIT(tls_enable_npn);  /* TLS NPN extension? */
   BIT(tls_enable_alpn); /* TLS ALPN extension? */
-  BIT(socksproxy_connecting); /* connecting through a socks proxy */
   BIT(connect_only);
 };
 
@@ -817,6 +816,41 @@ struct http_connect_state {
 
 struct ldapconninfo;
 
+/* for the (SOCKS) connect state machine */
+enum connect_t {
+  CONNECT_INIT,
+  CONNECT_SOCKS_INIT, /* 1 */
+  CONNECT_SOCKS_SEND, /* 2 waiting to send more first data */
+  CONNECT_SOCKS_READ_INIT, /* 3 set up read */
+  CONNECT_SOCKS_READ, /* 4 read server response */
+  CONNECT_GSSAPI_INIT, /* 5 */
+  CONNECT_AUTH_INIT, /* 6 setup outgoing auth buffer */
+  CONNECT_AUTH_SEND, /* 7 send auth */
+  CONNECT_AUTH_READ, /* 8 read auth response */
+  CONNECT_REQ_INIT,  /* 9 init SOCKS "request" */
+  CONNECT_RESOLVING, /* 10 */
+  CONNECT_RESOLVED,  /* 11 */
+  CONNECT_RESOLVE_REMOTE, /* 12 */
+  CONNECT_REQ_SEND,  /* 13 */
+  CONNECT_REQ_SENDING, /* 14 */
+  CONNECT_REQ_READ,  /* 15 */
+  CONNECT_REQ_READ_MORE, /* 16 */
+  CONNECT_DONE /* 17 connected fine to the remote or the SOCKS proxy */
+};
+
+#define SOCKS_STATE(x) (((x) >= CONNECT_SOCKS_INIT) &&  \
+                        ((x) < CONNECT_DONE))
+#define SOCKS_REQUEST_BUFSIZE 600  /* room for large user/pw (255 max each) */
+
+struct connstate {
+  enum connect_t state;
+  unsigned char socksreq[SOCKS_REQUEST_BUFSIZE];
+
+  /* CONNECT_SOCKS_SEND */
+  ssize_t outstanding;  /* send this many bytes more */
+  unsigned char *outp; /* send from this pointer */
+};
+
 /*
  * The connectdata struct contains all fields and variables that should be
  * unique for an entire connection.
@@ -826,7 +860,7 @@ struct connectdata {
      caution that this might very well vary between different times this
      connection is used! */
   struct Curl_easy *data;
-
+  struct connstate cnnct;
   struct curl_llist_element bundle_node; /* conncache */
 
   /* chunk is for HTTP chunked encoding, but is in the general connectdata