Re: proposed change to ssh_connect_direct()

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

 



On Thu, Jan 12, 2017 at 01:49:29PM +1100, Darren Tucker wrote:
[...]
> I would have thought that using non-blocking connect (ie set
> O_NONBLOCK on the fds, initiate the connections then select on the set
> until one succeeds) would be feasible, though.

In fact most of the required pieces are already there.  Below is a
rough, barely tested patch that nonetheless does seem to sort of work.

So, it's at least feasible.  I'm still not sure it's worth doing, though.
I can see some downsides: it'll spam server logs with "Did not receive
identification string" and consume MaxStartups connections.  Maybe best
to leave it to a custom ProxyCommand for people who really want the
functionality.


diff --git a/sshconnect.c b/sshconnect.c
index 96b91ce..bb8b6ee 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -328,89 +328,6 @@ ssh_create_socket(int privileged, struct addrinfo *ai)
 	return sock;
 }
 
-static int
-timeout_connect(int sockfd, const struct sockaddr *serv_addr,
-    socklen_t addrlen, int *timeoutp)
-{
-	fd_set *fdset;
-	struct timeval tv, t_start;
-	socklen_t optlen;
-	int optval, rc, result = -1;
-
-	gettimeofday(&t_start, NULL);
-
-	if (*timeoutp <= 0) {
-		result = connect(sockfd, serv_addr, addrlen);
-		goto done;
-	}
-
-	set_nonblock(sockfd);
-	rc = connect(sockfd, serv_addr, addrlen);
-	if (rc == 0) {
-		unset_nonblock(sockfd);
-		result = 0;
-		goto done;
-	}
-	if (errno != EINPROGRESS) {
-		result = -1;
-		goto done;
-	}
-
-	fdset = xcalloc(howmany(sockfd + 1, NFDBITS),
-	    sizeof(fd_mask));
-	FD_SET(sockfd, fdset);
-	ms_to_timeval(&tv, *timeoutp);
-
-	for (;;) {
-		rc = select(sockfd + 1, NULL, fdset, NULL, &tv);
-		if (rc != -1 || errno != EINTR)
-			break;
-	}
-
-	switch (rc) {
-	case 0:
-		/* Timed out */
-		errno = ETIMEDOUT;
-		break;
-	case -1:
-		/* Select error */
-		debug("select: %s", strerror(errno));
-		break;
-	case 1:
-		/* Completed or failed */
-		optval = 0;
-		optlen = sizeof(optval);
-		if (getsockopt(sockfd, SOL_SOCKET, SO_ERROR, &optval,
-		    &optlen) == -1) {
-			debug("getsockopt: %s", strerror(errno));
-			break;
-		}
-		if (optval != 0) {
-			errno = optval;
-			break;
-		}
-		result = 0;
-		unset_nonblock(sockfd);
-		break;
-	default:
-		/* Should not occur */
-		fatal("Bogus return (%d) from select()", rc);
-	}
-
-	free(fdset);
-
- done:
- 	if (result == 0 && *timeoutp > 0) {
-		ms_subtract_diff(&t_start, timeoutp);
-		if (*timeoutp <= 0) {
-			errno = ETIMEDOUT;
-			result = -1;
-		}
-	}
-
-	return (result);
-}
-
 /*
  * Opens a TCP/IP connection to the remote server on the given host.
  * The address of the remote host will be returned in hostaddr.
@@ -427,15 +344,25 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop,
     struct sockaddr_storage *hostaddr, u_short port, int family,
     int connection_attempts, int *timeout_ms, int want_keepalive, int needpriv)
 {
-	int on = 1;
-	int sock = -1, attempt;
+	int i, on = 1, rc, inprogress = 0;
+	int sock = -1, connected_sock = -1, attempt, fdmax;
 	char ntop[NI_MAXHOST], strport[NI_MAXSERV];
 	struct addrinfo *ai;
+	struct timeval tv, t_start;
+	fd_set fdset;
+	socklen_t rlen;
+#define MAXCONNECT 16
+	int inprogress_fd[MAXCONNECT];
+	void *ai_addr[MAXCONNECT];
+	size_t ai_len[MAXCONNECT];
 
 	debug2("%s: needpriv %d", __func__, needpriv);
 	memset(ntop, 0, sizeof(ntop));
 	memset(strport, 0, sizeof(strport));
 
+	FD_ZERO(&fdset);
+	gettimeofday(&t_start, NULL);
+
 	for (attempt = 0; attempt < connection_attempts; attempt++) {
 		if (attempt > 0) {
 			/* Sleep a moment before retrying. */
@@ -443,10 +370,11 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop,
 			debug("Trying again...");
 		}
 		/*
-		 * Loop through addresses for this host, and try each one in
-		 * sequence until the connection succeeds.
+		 * Loop through addresses for this host, initiate a nonblocking
+		 * connnection then see which one succeeds.
 		 */
-		for (ai = aitop; ai; ai = ai->ai_next) {
+		for (ai = aitop; ai != NULL && inprogress < MAXCONNECT &&
+		     connected_sock == -1; ai = ai->ai_next) {
 			if (ai->ai_family != AF_INET &&
 			    ai->ai_family != AF_INET6)
 				continue;
@@ -465,24 +393,56 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop,
 				/* Any error is already output */
 				continue;
 
-			if (timeout_connect(sock, ai->ai_addr, ai->ai_addrlen,
-			    timeout_ms) >= 0) {
-				/* Successful connection. */
-				memcpy(hostaddr, ai->ai_addr, ai->ai_addrlen);
+			set_nonblock(sock);
+			if (connect(sock, ai->ai_addr, ai->ai_addrlen) == 0) {
+				debug("connection established immediately");
+				connected_sock = sock;
+			} else if (errno == EINPROGRESS)
+				debug("nonblocking connect fd %d", sock);
+			else
+				continue;
+			inprogress_fd[inprogress] = sock;
+			ai_len[inprogress] = ai->ai_addrlen;
+			ai_addr[inprogress] = ai->ai_addr;
+			inprogress++;
+		}
+	}
+
+	for (i = 0; i < inprogress; i++)
+		unset_nonblock(inprogress_fd[i]);
+
+	ms_to_timeval(&tv, *timeout_ms);
+	while (connected_sock == -1 && timeout_ms > 0) {
+		FD_ZERO(&fdset);
+		fdmax = 0;
+		for (i = 0; i < inprogress; i++) {
+			FD_SET(inprogress_fd[i], &fdset);
+			fdmax = MAX(fdmax, inprogress_fd[i]);
+		}
+		rc = select(fdmax + 1, NULL, &fdset, NULL, &tv);
+		ms_subtract_diff(&t_start, timeout_ms);
+		rlen = sizeof(rc);
+		for (i = 0; i < fdmax; i++) {
+			if (!FD_ISSET(i, &fdset))
+				continue;
+			if (getsockopt(i, SOL_SOCKET, SO_ERROR, &rc, &rlen)
+			    == 0 && rc == 0) {
+				debug("connected to fd %d", i);
+				connected_sock = i;
 				break;
-			} else {
-				debug("connect to address %s port %s: %s",
-				    ntop, strport, strerror(errno));
-				close(sock);
-				sock = -1;
 			}
 		}
-		if (sock != -1)
-			break;	/* Successful connection. */
+	}
+
+	for (i = 0; i < inprogress; i++) {
+		if (connected_sock == inprogress_fd[i])
+			memcpy(hostaddr, ai_addr[i], ai_len[i]);
+		else
+			close(inprogress_fd[i]);
 	}
 
 	/* Return failure if we didn't get a successful connection. */
-	if (sock == -1) {
+	if (connected_sock == -1) {
 		error("ssh: connect to host %s port %s: %s",
 		    host, strport, strerror(errno));
 		return (-1);
@@ -492,12 +452,12 @@ ssh_connect_direct(const char *host, struct addrinfo *aitop,
 
 	/* Set SO_KEEPALIVE if requested. */
 	if (want_keepalive &&
-	    setsockopt(sock, SOL_SOCKET, SO_KEEPALIVE, (void *)&on,
+	    setsockopt(connected_sock, SOL_SOCKET, SO_KEEPALIVE, (void *)&on,
 	    sizeof(on)) < 0)
 		error("setsockopt SO_KEEPALIVE: %.100s", strerror(errno));
 
 	/* Set the connection. */
-	packet_set_connection(sock, sock);
+	packet_set_connection(connected_sock, connected_sock);
 
 	return 0;
 }

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev



[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux