Fix some situations where the remote IP of a connecting/disconnecting client is wrong in the debug log and NOTICEs.

4 files changed, 30 insertions, 13 deletions

diff --git a/TODO b/TODO
index 2f7e060..8d8d80c 100644
--- a/TODO
+++ b/TODO
@@ -25,4 +25,4 @@ Can memory usage be reduced further?  (e.g. better channel struct management)
 
 Ability to load new certificate whilst running.
 
-Disconnecting IP address announced is not always correct.
+arrindex() shouldn't return 0 on failure as 0 is a valid index.  Instead return -1 and change callers to check this.
diff --git a/blabouncer.c b/blabouncer.c
index 5ee48b5..c4e92ed 100644
--- a/blabouncer.c
+++ b/blabouncer.c
@@ -758,12 +758,24 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) {
             if (newfd > fdmax) {    // keep track of the max
               fdmax = newfd;
             }
+
+            // If openssl_accept fails later on, this is the message we'll append to the usual connection announcement
+            char opensslfailmsg[] = ", but was disconnected due to a failure in SSL_accept()";
+            // Store the client's IP address for now, since we may need to refer to it after disconnecting
+            // them (thus clearing the array entry that the IP is read from) if something goes wrong
+            char remoteip[INET6_ADDRSTRLEN];
+            strncpy(remoteip, inet_ntop(remoteaddr.ss_family, get_in_addr((struct sockaddr*)&remoteaddr), remoteIP, INET6_ADDRSTRLEN), INET6_ADDRSTRLEN);
+
             // Find a free element in the clients array and set to new fd value (plus start SSL_accept() if using client TLS)
             for (int j = 0; j < MAXCLIENTS; j++) {
               if (clients[j].fd == 0) {
                 clients[j].fd = newfd;
                 // Ensure its authentication status is set to 0
                 clients[j].authed = 0;
+
+                // Record the remote IP address of this client in the clients array
+                strncpy(clients[j].remoteip, remoteip, INET6_ADDRSTRLEN);
+
                 // If using TLS then...
                 if (settings->clienttls) {
                   // ...set as OpenSSL FD and SSL_accept it
@@ -776,27 +788,28 @@ void dochat(int *serversockfd, int *clientsockfd, struct settings *settings) {
                     debugprint(DEBUG_CRIT, "fd_toggle_blocking on failed for fd %d: %s.\n", clients[j].fd, strerror(errno));
                     disconnectclient(clients[j].fd, clients, &ircdstate, settings, clientcodes);
                   }
-                  // Try to SSL_accept(), not interested in return code here since openssl_accept() does the right thing.
-                  openssl_accept(clients[j].fd, clients, &ircdstate, settings, clientcodes);
+                  // Try to SSL_accept()
+                  if (openssl_accept(clients[j].fd, clients, &ircdstate, settings, clientcodes)) {
+                    // It succeeded, so clear the failure message
+                    opensslfailmsg[0] = '\0';
+                  }
                 } else {
                   // If not using TLS then cast newfd to SSL* even though it will just be the original newfd int really
                   clients[j].ssl = (SSL*)(long int)newfd;
+                  // There can't be an openssl_accept failure if we're not using TLS
+                  opensslfailmsg[0] = '\0';
                 }
 
-                // Record the remote IP address of this client in the clients array
-                strncpy(clients[j].remoteip, inet_ntop(remoteaddr.ss_family, get_in_addr((struct sockaddr*)&remoteaddr), remoteIP, INET6_ADDRSTRLEN), INET6_ADDRSTRLEN);
-
                 break;
               }
             }
 
             // TODO - Handle the "find a free element" loop not finding a free element
-            debugprint(DEBUG_FULL, "bouncer-client: new connection from %s on socket %d\n",
-                       clients[arrindex(clients, newfd)].remoteip, newfd);
+            debugprint(DEBUG_FULL, "bouncer-client: new connection from %s on socket %d%s\n", remoteip, newfd, opensslfailmsg);
             // Alert other clients about the new connection
             char alertmsg[MAXDATASIZE];
-            if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client connected from %s.", ircdstate.ircnick,
-                          clients[arrindex(clients, newfd)].remoteip)) {
+            if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client connected from %s%s.", ircdstate.ircnick,
+                          remoteip, opensslfailmsg)) {
               fprintf(stderr, "Error while preparing new client connection NOTICE!\n");
               debugprint(DEBUG_CRIT, "Error while preparing new client connection NOTICE!\n");
               alertmsg[0] = '\0';
diff --git a/functions.c b/functions.c
index 2b039e0..10e71a1 100644
--- a/functions.c
+++ b/functions.c
@@ -404,6 +404,7 @@ int arrindex(struct client *clients, int clientfd) {
   }
 
   // Something went wrong, we didn't find it
+  // TODO - Don't return 0 since 0 is a valid array index, return -1 and have callers check for that.
   return 0;
 }
 
diff --git a/message.c b/message.c
index 7d50f0a..2c6eb16 100644
--- a/message.c
+++ b/message.c
@@ -846,12 +846,15 @@ int processclientmessage(SSL *server_ssl, char *str, struct client *clients, int
         }
       }
     } else {
-      debugprint(DEBUG_SOME, "Password rejected, disconnecting client %s with fd %d.\n", clients[arrindex(clients, sourcefd)].remoteip, sourcefd);
+      // Store the client's IP address for now, since we need to refer to it after disconnecting
+      // them (thus clearing the array entry that the IP is read from)
+      char remoteip[INET6_ADDRSTRLEN];
+      strncpy(remoteip, clients[arrindex(clients, sourcefd)].remoteip, INET6_ADDRSTRLEN);
+      debugprint(DEBUG_SOME, "Password rejected, disconnecting client %s with fd %d.\n", remoteip, sourcefd);
       disconnectclient(sourcefd, clients, ircdstate, settings, clientcodes);
       // Alert other clients about the failed authentication
       char alertmsg[MAXDATASIZE];
-      if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client %s has failed to authenticate.", ircdstate->ircnick,
-          clients[arrindex(clients, sourcefd)].remoteip)) {
+      if (!snprintf(alertmsg, MAXDATASIZE, "NOTICE %s :blabouncer: new client %s failed to authenticate.", ircdstate->ircnick, remoteip)) {
         fprintf(stderr, "Error while preparing authentication failure NOTICE!\n");
         debugprint(DEBUG_CRIT, "Error while preparing authentication failure NOTICE!\n");
         alertmsg[0] = '\0';