From a66c287b0604c257e80a8859f887ea491b1a1268 Mon Sep 17 00:00:00 2001
From: dmiller <dmiller@e0a8ed71-7df4-0310-8962-fdc924857419>
Date: Thu, 2 Jan 2020 16:04:30 +0000
Subject: [PATCH] Loop over client FDs, avoiding unused ones

As the FIXME comment had said, looping over every integer up to maxfd is
inefficient, especially if FDs are not continuous. This change has the
added benefit of skipping a call to get_fdinfo(), which also loops over
all the client FDs looking for a particular value. Unlikely to be a huge
performance gain, but the code is cleaner. #1834 - FIXME comment.
---
 ncat/ncat_listen.c | 51 ++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/ncat/ncat_listen.c b/ncat/ncat_listen.c
index 3c4c07503..4f585102e 100644
--- a/ncat/ncat_listen.c
+++ b/ncat/ncat_listen.c
@@ -360,47 +360,44 @@ static int ncat_listen_stream(int proto)
         if (fds_ready == 0)
             bye("Idle timeout expired (%d ms).", o.idletimeout);
 
-        /*
-         * FIXME: optimize this loop to look only at the fds in the fd list,
-         * doing it this way means that if you have one descriptor that is very
-         * large, say 500, and none close to it, that you'll loop many times for
-         * nothing.
-         */
-        for (i = 0; i <= client_fdlist.fdmax && fds_ready > 0; i++) {
+        for (i = 0; i < client_fdlist.nfds && fds_ready > 0; i++) {
+            struct fdinfo *fdi = &client_fdlist.fds[i];
+            int cfd = fdi->fd;
             /* Loop through descriptors until there's something to read */
-            if (!FD_ISSET(i, &readfds) && !FD_ISSET(i, &writefds))
+            if (!FD_ISSET(cfd, &readfds) && !FD_ISSET(cfd, &writefds))
                 continue;
 
             if (o.debug > 1)
-                logdebug("fd %d is ready\n", i);
+                logdebug("fd %d is ready\n", cfd);
 
 #ifdef HAVE_OPENSSL
             /* Is this an ssl socket pending a handshake? If so handle it. */
-            if (o.ssl && FD_ISSET(i, &sslpending_fds)) {
-                struct fdinfo *fdi = NULL;
-                FD_CLR(i, &master_readfds);
-                FD_CLR(i, &master_writefds);
-                fdi = get_fdinfo(&client_fdlist, i);
-                ncat_assert(fdi != NULL);
+            if (o.ssl && FD_ISSET(cfd, &sslpending_fds)) {
+                FD_CLR(cfd, &master_readfds);
+                FD_CLR(cfd, &master_writefds);
                 switch (ssl_handshake(fdi)) {
                 case NCAT_SSL_HANDSHAKE_COMPLETED:
                     /* Clear from sslpending_fds once ssl is established */
-                    FD_CLR(i, &sslpending_fds);
+                    FD_CLR(cfd, &sslpending_fds);
                     post_handle_connection(*fdi);
                     break;
                 case NCAT_SSL_HANDSHAKE_PENDING_WRITE:
-                    FD_SET(i, &master_writefds);
+                    FD_SET(cfd, &master_writefds);
                     break;
                 case NCAT_SSL_HANDSHAKE_PENDING_READ:
-                    FD_SET(i, &master_readfds);
+                    FD_SET(cfd, &master_readfds);
                     break;
                 case NCAT_SSL_HANDSHAKE_FAILED:
                 default:
                     SSL_free(fdi->ssl);
                     Close(fdi->fd);
-                    FD_CLR(i, &sslpending_fds);
-                    FD_CLR(i, &master_readfds);
-                    rm_fd(&client_fdlist, i);
+                    FD_CLR(cfd, &sslpending_fds);
+                    FD_CLR(cfd, &master_readfds);
+                    rm_fd(&client_fdlist, cfd);
+                    /* Since we removed this one, start loop over at the beginning.
+                     * Wastes a little time, but ensures correctness.
+                     */
+                    i = 0;
                     /* Are we in single listening mode(without -k)? If so
                        then we should quit also. */
                     if (!o.keepopen && !o.broker)
@@ -410,12 +407,12 @@ static int ncat_listen_stream(int proto)
                 }
             } else
 #endif
-            if (FD_ISSET(i, &listen_fds)) {
+            if (FD_ISSET(cfd, &listen_fds)) {
                 /* we have a new connection request */
-                handle_connection(i);
-            } else if (i == STDIN_FILENO) {
+                handle_connection(cfd);
+            } else if (cfd == STDIN_FILENO) {
                 if (o.broker) {
-                    read_and_broadcast(i);
+                    read_and_broadcast(cfd);
                 } else {
                     /* Read from stdin and write to all clients. */
                     rc = read_stdin();
@@ -432,10 +429,10 @@ static int ncat_listen_stream(int proto)
                 }
             } else if (!o.sendonly) {
                 if (o.broker) {
-                    read_and_broadcast(i);
+                    read_and_broadcast(cfd);
                 } else {
                     /* Read from a client and write to stdout. */
-                    rc = read_socket(i);
+                    rc = read_socket(cfd);
                     if (rc <= 0 && !o.keepopen)
                         return rc == 0 ? 0 : 1;
                 }