connect: happy eyeballs cleanup

Make sure each separate index in connn->tempaddr[] is used for a fixed
family (and only that family) during the connection process.

If family one takes a long time and family two fails immediately, the
previous logic could misbehave and retry the same family two address
repeatedly.

Reported-by: Paul Vixie
Reported-by: Jay Satiro
Fixes #5083
Fixes #4954
Closes #5089

2 files changed, 51 insertions, 53 deletions

diff --git a/lib/connect.c b/lib/connect.c
index 0a7475cb6..5e631baa9 100644
--- a/lib/connect.c
+++ b/lib/connect.c
@@ -167,7 +167,7 @@ tcpkeepalive(struct Curl_easy *data,
 static CURLcode
 singleipconnect(struct connectdata *conn,
                 const Curl_addrinfo *ai, /* start connecting to this */
-                int sockindex);          /* 0 or 1 among the temp ones */
+                int tempindex);          /* 0 or 1 among the temp ones */
 
 /*
  * Curl_timeleft() returns the amount of milliseconds left allowed for the
@@ -555,13 +555,27 @@ static bool verifyconnect(curl_socket_t sockfd, int *error)
   return rc;
 }
 
+/* update tempaddr[tempindex] (to the next entry), makes sure to stick
+   to the correct family */
+static Curl_addrinfo *ainext(struct connectdata *conn,
+                             int tempindex,
+                             bool next) /* use current or next entry */
+{
+  Curl_addrinfo *ai = conn->tempaddr[tempindex];
+  if(ai && next)
+    ai = ai->ai_next;
+  while(ai && (ai->ai_family != conn->tempfamily[tempindex]))
+    ai = ai->ai_next;
+  conn->tempaddr[tempindex] = ai;
+  return ai;
+}
+
 /* Used within the multi interface. Try next IP address, return TRUE if no
    more address exists or error */
 static CURLcode trynextip(struct connectdata *conn,
                           int sockindex,
                           int tempindex)
 {
-  const int other = tempindex ^ 1;
   CURLcode result = CURLE_COULDNT_CONNECT;
 
   /* First clean up after the failed socket.
@@ -572,38 +586,15 @@ static CURLcode trynextip(struct connectdata *conn,
   conn->tempsock[tempindex] = CURL_SOCKET_BAD;
 
   if(sockindex == FIRSTSOCKET) {
-    Curl_addrinfo *ai = NULL;
-    int family = AF_UNSPEC;
-
-    if(conn->tempaddr[tempindex]) {
-      /* find next address in the same protocol family */
-      family = conn->tempaddr[tempindex]->ai_family;
-      ai = conn->tempaddr[tempindex]->ai_next;
-    }
-#ifdef ENABLE_IPV6
-    else if(conn->tempaddr[0]) {
-      /* happy eyeballs - try the other protocol family */
-      int firstfamily = conn->tempaddr[0]->ai_family;
-      family = (firstfamily == AF_INET) ? AF_INET6 : AF_INET;
-      ai = conn->tempaddr[0]->ai_next;
-    }
-#endif
+    Curl_addrinfo *ai = conn->tempaddr[tempindex];
 
     while(ai) {
-      if(conn->tempaddr[other]) {
-        /* we can safely skip addresses of the other protocol family */
-        while(ai && ai->ai_family != family)
-          ai = ai->ai_next;
-      }
-
       if(ai) {
         result = singleipconnect(conn, ai, tempindex);
         if(result == CURLE_COULDNT_CONNECT) {
-          ai = ai->ai_next;
+          ai = ainext(conn, tempindex, TRUE);
           continue;
         }
-
-        conn->tempaddr[tempindex] = ai;
       }
       break;
     }
@@ -905,9 +896,10 @@ CURLcode Curl_is_connected(struct connectdata *conn,
       }
 
       /* should we try another protocol family? */
-      if(i == 0 && conn->tempaddr[1] == NULL &&
+      if(i == 0 && !conn->parallel_connect &&
          (Curl_timediff(now, conn->connecttime) >=
           data->set.happy_eyeballs_timeout)) {
+        conn->parallel_connect = TRUE; /* starting now */
         trynextip(conn, sockindex, 1);
       }
     }
@@ -967,7 +959,7 @@ CURLcode Curl_is_connected(struct connectdata *conn,
 
         conn->timeoutms_per_addr = conn->tempaddr[i]->ai_next == NULL ?
           allow : allow / 2;
-
+        ainext(conn, i, TRUE);
         status = trynextip(conn, sockindex, i);
         if((status != CURLE_COULDNT_CONNECT) ||
            conn->tempsock[other] == CURL_SOCKET_BAD)
@@ -984,7 +976,7 @@ CURLcode Curl_is_connected(struct connectdata *conn,
 
     /* if the first address family runs out of addresses to try before
        the happy eyeball timeout, go ahead and try the next family now */
-    if(conn->tempaddr[1] == NULL) {
+    {
       result = trynextip(conn, sockindex, 1);
       if(!result)
         return result;
@@ -1113,7 +1105,7 @@ void Curl_sndbufset(curl_socket_t sockfd)
  */
 static CURLcode singleipconnect(struct connectdata *conn,
                                 const Curl_addrinfo *ai,
-                                int sockindex)
+                                int tempindex)
 {
   struct Curl_sockaddr_ex addr;
   int rc = -1;
@@ -1129,15 +1121,12 @@ static CURLcode singleipconnect(struct connectdata *conn,
   int optval = 1;
 #endif
   char buffer[STRERROR_LEN];
-  curl_socket_t *sockp = &conn->tempsock[sockindex];
+  curl_socket_t *sockp = &conn->tempsock[tempindex];
   *sockp = CURL_SOCKET_BAD;
 
   result = Curl_socket(conn, ai, &addr, &sockfd);
   if(result)
-    /* Failed to create the socket, but still return OK since we signal the
-       lack of socket as well. This allows the parent function to keep looping
-       over alternative addresses/socket families etc. */
-    return CURLE_OK;
+    return result;
 
   /* store remote address and port used in this connection attempt */
   if(!Curl_addr2string((struct sockaddr*)&addr.sa_addr, addr.addrlen,
@@ -1257,7 +1246,7 @@ static CURLcode singleipconnect(struct connectdata *conn,
     else if(conn->transport == TRNSPRT_QUIC) {
       /* pass in 'sockfd' separately since it hasn't been put into the
          tempsock array at this point */
-      result = Curl_quic_connect(conn, sockfd, sockindex,
+      result = Curl_quic_connect(conn, sockfd, tempindex,
                                  &addr.sa_addr, addr.addrlen);
       if(result)
         error = SOCKERRNO;
@@ -1315,7 +1304,7 @@ CURLcode Curl_connecthost(struct connectdata *conn,  /* context */
   struct Curl_easy *data = conn->data;
   struct curltime before = Curl_now();
   CURLcode result = CURLE_COULDNT_CONNECT;
-
+  int i;
   timediff_t timeout_ms = Curl_timeleft(data, &before, TRUE);
 
   if(timeout_ms < 0) {
@@ -1325,28 +1314,34 @@ CURLcode Curl_connecthost(struct connectdata *conn,  /* context */
   }
 
   conn->num_addr = Curl_num_addresses(remotehost->addr);
-  conn->tempaddr[0] = remotehost->addr;
-  conn->tempaddr[1] = NULL;
-  conn->tempsock[0] = CURL_SOCKET_BAD;
-  conn->tempsock[1] = CURL_SOCKET_BAD;
+  conn->tempaddr[0] = conn->tempaddr[1] = remotehost->addr;
+  conn->tempsock[0] = conn->tempsock[1] = CURL_SOCKET_BAD;
 
   /* Max time for the next connection attempt */
   conn->timeoutms_per_addr =
     conn->tempaddr[0]->ai_next == NULL ? timeout_ms : timeout_ms / 2;
 
-  /* start connecting to first IP */
-  while(conn->tempaddr[0]) {
-    result = singleipconnect(conn, conn->tempaddr[0], 0);
-    if(!result)
-      break;
-    conn->tempaddr[0] = conn->tempaddr[0]->ai_next;
-  }
+  conn->tempfamily[0] = conn->tempaddr[0]?
+    conn->tempaddr[0]->ai_family:0;
+  conn->tempfamily[1] = conn->tempfamily[0] == AF_INET6 ?
+    AF_INET : AF_INET6;
+  ainext(conn, 1, FALSE); /* assigns conn->tempaddr[1] accordingly */
 
-  if(conn->tempsock[0] == CURL_SOCKET_BAD) {
-    if(!result)
-      result = CURLE_COULDNT_CONNECT;
-    return result;
+  DEBUGF(infof(data, "family0 == %s, family1 == %s\n",
+               conn->tempfamily[0] == AF_INET ? "v4" : "v6",
+               conn->tempfamily[1] == AF_INET ? "v4" : "v6"));
+
+  /* get through the list in family order in case of quick failures */
+  for(i = 0; (i < 2) && result; i++) {
+    while(conn->tempaddr[i]) {
+      result = singleipconnect(conn, conn->tempaddr[i], i);
+      if(!result)
+        break;
+      ainext(conn, i, TRUE);
+    }
   }
+  if(result)
+    return result;
 
   data->info.numconnects++; /* to track the number of connections made */
   Curl_expire(conn->data, data->set.happy_eyeballs_timeout,
diff --git a/lib/urldata.h b/lib/urldata.h
index fbb8b645e..4ee568fd6 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -957,6 +957,7 @@ struct connectdata {
   curl_socket_t sock[2]; /* two sockets, the second is used for the data
                             transfer when doing FTP */
   curl_socket_t tempsock[2]; /* temporary sockets for happy eyeballs */
+  int tempfamily[2]; /* family used for the temp sockets */
   Curl_recv *recv[2];
   Curl_send *send[2];
 
@@ -1113,6 +1114,8 @@ struct connectdata {
                               handle */
   BIT(sock_accepted); /* TRUE if the SECONDARYSOCKET was created with
                          accept() */
+  BIT(parallel_connect); /* set TRUE when a parallel connect attempt has
+                            started (happy eyeballs) */
 };
 
 /* The end of connectdata. */