openssh unix dev - May 2017 - select() is not called properly in ssh_exchange_identification()

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.

[cid:image001.png at 01D2D95A.23728F00]

Thanks,
Balu

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