Re: select() is not called properly in ssh_exchange_identification()

[Damien Miller <djm@xxxxxxxxxxx>]

On Tue, 30 May 2017, Balu Gajjala wrote:

> Hi,
> 
> I found an issue with select() not called properly in the ssh_exchange_identification().
> 
> Variable "fdset" is passed as readfd, exceptionfd to the select().
> Select() should be called with independent fdset so we should have two different variables instead of reusing the same variable "fdset".
> Here is the code snippet. The reported issue is in line 566, 567.
> 
> Please let me know the procedure to create an official bug.

It's probably time to switch this to poll(2)

diff --git a/sshconnect.c b/sshconnect.c
index a9cc9f3..e6c8388 100644
--- a/sshconnect.c
+++ b/sshconnect.c
@@ -26,6 +26,7 @@
 #include <fcntl.h>
 #include <netdb.h>
 #include <paths.h>
+#include <poll.h>
 #include <signal.h>
 #include <pwd.h>
 #include <stdio.h>
@@ -322,12 +323,10 @@ static int
 timeout_connect(int sockfd, const struct sockaddr *serv_addr,
     socklen_t addrlen, int *timeoutp)
 {
-	fd_set *fdset;
-	struct timeval tv, t_start;
+	struct timeval t_start;
 	socklen_t optlen;
 	int optval, rc, result = -1;
-
-	gettimeofday(&t_start, NULL);
+	struct pollfd pfd;
 
 	if (*timeoutp <= 0) {
 		result = connect(sockfd, serv_addr, addrlen);
@@ -346,16 +345,18 @@ timeout_connect(int sockfd, const struct sockaddr *serv_addr,
 		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);
+		gettimeofday(&t_start, NULL);
+		pfd.fd = sockfd;
+		pfd.events = POLLIN;
+		rc = poll(&pfd, 1, *timeoutp);
+		ms_subtract_diff(&t_start, timeoutp);
 		if (rc != -1 || errno != EINTR)
 			break;
 	}
+	/* Socket may have connected by exhausted timeout anyway */
+	if (*timeoutp <= 0)
+		rc = 0;
 
 	switch (rc) {
 	case 0:
@@ -364,7 +365,7 @@ timeout_connect(int sockfd, const struct sockaddr *serv_addr,
 		break;
 	case -1:
 		/* Select error */
-		debug("select: %s", strerror(errno));
+		debug("poll: %s", strerror(errno));
 		break;
 	case 1:
 		/* Completed or failed */
@@ -387,8 +388,6 @@ timeout_connect(int sockfd, const struct sockaddr *serv_addr,
 		fatal("Bogus return (%d) from select()", rc);
 	}
 
-	free(fdset);
-
  done:
  	if (result == 0 && *timeoutp > 0) {
 		ms_subtract_diff(&t_start, timeoutp);
@@ -536,12 +535,9 @@ ssh_exchange_identification(int timeout_ms)
 	int connection_out = packet_get_connection_out();
 	u_int i, n;
 	size_t len;
-	int fdsetsz, remaining, rc;
-	struct timeval t_start, t_remaining;
-	fd_set *fdset;
-
-	fdsetsz = howmany(connection_in + 1, NFDBITS) * sizeof(fd_mask);
-	fdset = xcalloc(1, fdsetsz);
+	int remaining, rc;
+	struct timeval t_start;
+	struct pollfd pfd;
 
 	send_client_banner(connection_out, 0);
 
@@ -551,10 +547,9 @@ ssh_exchange_identification(int timeout_ms)
 		for (i = 0; i < sizeof(buf) - 1; i++) {
 			if (timeout_ms > 0) {
 				gettimeofday(&t_start, NULL);
-				ms_to_timeval(&t_remaining, remaining);
-				FD_SET(connection_in, fdset);
-				rc = select(connection_in + 1, fdset, NULL,
-				    fdset, &t_remaining);
+				pfd.fd = connection_in;
+				pfd.events = POLLIN;
+				rc = poll(&pfd, 1, remaining);
 				ms_subtract_diff(&t_start, &remaining);
 				if (rc == 0 || remaining <= 0)
 					fatal("Connection timed out during "
@@ -563,7 +558,7 @@ ssh_exchange_identification(int timeout_ms)
 					if (errno == EINTR)
 						continue;
 					fatal("ssh_exchange_identification: "
-					    "select: %s", strerror(errno));
+					    "poll: %s", strerror(errno));
 				}
 			}
 
@@ -594,7 +589,6 @@ ssh_exchange_identification(int timeout_ms)
 		debug("ssh_exchange_identification: %s", buf);
 	}
 	server_version_string = xstrdup(buf);
-	free(fdset);
 
 	/*
 	 * Check that the versions match.  In future this might accept
_______________________________________________
openssh-unix-dev mailing list
openssh-unix-dev@xxxxxxxxxxx
https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev