stop managing connections as array; instead, allocate connections, and fd2cnx is the only pointer to it

Makefile

@@ -28,7 +28,7 @@ CC ?= gcc
 CFLAGS ?=-Wall -g $(CFLAGS_COV)
 
 LIBS=
-OBJS=sslh-conf.o common.o sslh-main.o probe.o tls.o argtable3.o udp-listener.o collection.o
+OBJS=sslh-conf.o common.o sslh-main.o probe.o tls.o argtable3.o udp-listener.o collection.o gap.o
 
 CONDITIONAL_TARGETS=
 
@@ -78,7 +78,7 @@ version.h:
 
 sslh: sslh-fork sslh-select
 
-$(OBJS): version.h common.h collection.h sslh-conf.h
+$(OBJS): version.h common.h collection.h sslh-conf.h gap.h
 
 sslh-conf.c: sslhconf.cfg
 	conf2struct sslhconf.cfg

collection.c

@@ -23,183 +23,72 @@
 #include "common.h"
 #include "collection.h"
 #include "sslh-conf.h"
-
+#include "gap.h"
 
 /* Info to keep track of all connections */
 struct cnx_collection {
-    int num_cnx;  /* Number of connections in *cnx */
+    gap_array* fd2cnx;  /* Array indexed by file descriptor to things in cnx[] */
-    struct connection *cnx; /* pointer to array of connections */
-
-    int num_fd; /* Number of file descriptors */
-    struct connection** fd2cnx; /* Array indexed by file descriptor to things in cnx[] */
-    /* We don't try to keep the size of cnx and fd2cnx in sync at all,
-     * so that the implementation is independant of other uses for file
-     * descriptors, e.g. if sslh get integrated in another process */
 };
 
-/* cnx_num_alloc is the number of connection to allocate at once (at start-up,
- * and then every time we get too many simultaneous connections: e.g. start
- * with 100 slots, then if we get more than 100 connections allocate another
- * 100 slots, and so on). We never free up connection structures. We try to
- * allocate as many structures at once as will fit in one page (which is 102
- * in sslh 1.9 on Linux on x86)
- */
-static long cnx_num_alloc;
-
-static long fd_num_alloc; /* same, but for the file descriptor array */
-
-
-
 /* Allocates and initialises a new collection of connections. */
 cnx_collection* collection_init(void)
 {
-    int i;
     cnx_collection* collection;
 
     collection = malloc(sizeof(*collection));
-    CHECK_ALLOC(collection, "malloc(collection)");
+    CHECK_ALLOC(collection, "collection_init(collection)");
 
     memset(collection, 0, sizeof(*collection));
-    cnx_num_alloc = getpagesize() / sizeof(struct connection);
 
-    collection->num_cnx = cnx_num_alloc; /* Start with a set pool of slots */
+    collection->fd2cnx = gap_init();
-    collection->cnx = malloc(collection->num_cnx * sizeof(struct connection));
-    CHECK_ALLOC(collection->cnx, "malloc(collection->cnx)");
-
-    for (i = 0; i < collection->num_cnx; i++) {
-        init_cnx(&collection->cnx[i]);
-    }
-
-    fd_num_alloc = getpagesize() / sizeof(collection->fd2cnx[0]);
-
-    collection->num_fd = fd_num_alloc;
-    collection->fd2cnx = malloc(collection->num_fd * sizeof(collection->fd2cnx[0])); 
-    CHECK_ALLOC(collection->fd2cnx, "malloc(collection->fd2cnx)");
-
-    for (i = 0; i < collection->num_fd; i++) {
-        collection->fd2cnx[i] = NULL;
-    }
 
     return collection;
 }
 
+/* Caveat: might not work, as has never been used */
 void collection_destroy(cnx_collection* collection)
 {
+    /* Caveat 2: no code to free connections yet */
+    gap_destroy(collection->fd2cnx);
     free(collection);
 }
 
-/* Increases the number of slots available in a collection of connections
- * After calling, collection->cnx might have moved
- * */
-static int extend_cnx(struct cnx_collection* collection)
-{
-    struct connection* new;
-    int i, new_length = collection->num_cnx + cnx_num_alloc;
-
-    if (cfg.verbose)
-        fprintf(stderr, "allocating %ld more slots (target: %d).\n", cnx_num_alloc, new_length);
-    new = realloc(collection->cnx, new_length * sizeof(collection->cnx[0]));
-    if (!new) return -1;
-
-    collection->cnx = new;
-
-    for (i = collection->num_cnx; i < new_length; i++) {
-        init_cnx(&collection->cnx[i]); 
-    }
-    collection->num_cnx = new_length;
-    return 0;
-}
-
-static int extend_fd2cnx(cnx_collection* collection)
-{
-    struct connection** new_array;
-    int i, new_length;
-
-    new_length = collection->num_fd + fd_num_alloc;
-
-    new_array = realloc(collection->fd2cnx, new_length * sizeof(*new_array));
-    if (!new_array) {
-        return -1;
-    }
-
-    collection->fd2cnx = new_array;
-
-    for (i = collection->num_fd; i < new_length; i++) {
-        collection->fd2cnx[i] = NULL;
-    }
-    collection->num_fd = new_length;
-
-    return 0;
-}
-
 /* Points the file descriptor to the specified connection index */
 int collection_add_fd(cnx_collection* collection, struct connection* cnx, int fd)
 {
-    if (fd > collection->num_fd) {
+    gap_set(collection->fd2cnx, fd, cnx);
-        int res = extend_fd2cnx(collection);
-        if (res) {
-            log_message(LOG_ERR, "unable to extend fd2cnx -- dropping connection\n");
-            return -1;
-        }
-    }
-    collection->fd2cnx[fd] = cnx;
-    int cnx_index = cnx - collection->cnx;
-    if(cfg.verbose) fprintf(stderr, "added fd %d on slot %d\n", fd, cnx_index);
     return 0;
 }
 
-
 /* Allocates a connection and inits it with specified file descriptor */
 int collection_alloc_cnx_from_fd(struct cnx_collection* collection, int fd)
 {
-    int free, res;
+    struct connection* cnx = malloc(sizeof(*cnx));
-    struct connection* cnx = collection->cnx;
 
-    /* Find an empty slot */
+    if (!cnx) return -1;
-    for (free = 0; (free < collection->num_cnx) && (cnx[free].q[0].fd != -1); free++) {
-        /* nothing */
-    }
-    if (free >= collection->num_cnx)  {
-        res = extend_cnx(collection);
-        if (res) {
-            log_message(LOG_ERR, "unable to extend collection -- dropping connection\n");
-            return -1;
-        }
-    }
-    collection->cnx[free].q[0].fd = fd;
-    collection->cnx[free].state = ST_PROBING;
-    collection->cnx[free].probe_timeout = time(NULL) + cfg.timeout;
 
-    collection_add_fd(collection, &collection->cnx[free], fd);
+    init_cnx(cnx);
+    cnx->q[0].fd = fd;
+    cnx->state = ST_PROBING;
+    cnx->probe_timeout = time(NULL) + cfg.timeout;
+
+    gap_set(collection->fd2cnx, fd, cnx);
 
-    if (cfg.verbose) 
-        fprintf(stderr, "accepted fd %d on slot %d\n", fd, free);
     return 0;
 }
 
 /* Remove a connection from the collection */
 int collection_remove_cnx(cnx_collection* collection, struct connection *cnx)
 {
-    collection->fd2cnx[cnx->q[0].fd] = NULL;
+    gap_set(collection->fd2cnx, cnx->q[0].fd, NULL);
-    collection->fd2cnx[cnx->q[1].fd] = NULL;
+    gap_set(collection->fd2cnx, cnx->q[1].fd, NULL);
-    init_cnx(cnx);
+    free(cnx);
     return 0;
 }
 
-/* Returns the indexed connection in the collection */
-struct connection* collection_get_cnx_from_index(struct cnx_collection* collection, int index)
-{
-    return & collection->cnx[index];
-}
-
 /* Returns the connection that contains the file descriptor */
 struct connection* collection_get_cnx_from_fd(struct cnx_collection* collection, int fd)
 {
-    return collection->fd2cnx[fd];
+    return gap_get(collection->fd2cnx, fd);
 }
 
-/* Returns the number of connections in the collection */
-int collection_get_length(cnx_collection* collection)
-{
-    return collection->num_cnx;
-}

gap.c

@@ -0,0 +1,100 @@
+/*
+   gap.c: gap, a simple, dynamically-growing array
+   of pointers that never shrinks
+
+# Copyright (C) 2021  Yves Rutschle
+# 
+# This program is free software; you can redistribute it
+# and/or modify it under the terms of the GNU General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later
+# version.
+# 
+# This program is distributed in the hope that it will be
+# useful, but WITHOUT ANY WARRANTY; without even the implied
+# warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU General Public License for more
+# details.
+# 
+# The full text for the General Public License is here:
+# http://www.gnu.org/licenses/gpl.html
+
+*/
+
+
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include "gap.h"
+
+
+typedef struct gap_array {
+    int len; /* Number of elements in array */
+    void** array;
+} gap_array;
+
+/* Allocate one page-worth of elements */
+static int gap_len_alloc(int elem_size)
+{
+    return getpagesize() / elem_size;
+}
+
+/* Creates a new gap, all pointers are initialised at NULL */
+gap_array* gap_init(void)
+{
+    gap_array* gap = malloc(sizeof(*gap));
+    if (!gap) return NULL;
+    memset(gap, 0, sizeof(*gap));
+
+    int elem_size = sizeof(gap->array[0]);
+    gap->len = gap_len_alloc(elem_size);
+    gap->array = malloc(gap->len * elem_size);
+    if (!gap->array) return NULL;
+
+    for (int i = 0; i < gap->len; i++)
+        gap->array[i] = NULL;
+
+    return gap;
+}
+
+int gap_getlen(gap_array* gap)
+{
+    return gap->len;
+}
+
+void* gap_get(gap_array* gap, int index)
+{
+    int elem_size = sizeof(gap->array[0]);
+    return gap->array[index * elem_size];
+}
+
+static int gap_extend(gap_array* gap)
+{
+    int elem_size = sizeof(gap->array[0]);
+    int new_length = gap->len + gap_len_alloc(elem_size);
+    void** new = realloc(gap->array, new_length * elem_size);
+    if (!new) return -1;
+
+    for (int i = gap->len; i < new_length; i++)
+        gap->array[i] = NULL;
+    return 0;
+}
+
+int gap_set(gap_array* gap, int index, void* ptr)
+{
+    if (index > gap->len) {
+        int res = gap_extend(gap);
+        if (res == -1) return -1;
+    }
+
+    int elem_size = sizeof(gap->array[0]);
+    gap->array[index * elem_size] = ptr;
+    return 0;
+}
+
+void gap_destroy(gap_array* gap) 
+{
+    free(gap->array);
+    free(gap);
+}
+

gap.h

@@ -0,0 +1,12 @@
+#ifndef GAP_H
+#define GAP_H
+
+typedef struct gap_array gap_array;
+
+gap_array* gap_init();
+int gap_getlen(gap_array* gap);
+void* gap_get(gap_array* gap, int index);
+int gap_set(gap_array* gap, int index, void* ptr);
+void gap_destroy(gap_array* gap);
+
+#endif

sslh-select.c

@@ -26,6 +26,8 @@
 #include "probe.h"
 #include "collection.h"
 
+static int debug = 0;
+
 const char* server_type = "sslh-select";
 
 /* Global state for a select() loop */
@@ -331,6 +333,8 @@ int active_queue(struct connection* cnx, int fd)
 static void cnx_read_process(struct select_info* fd_info,
                              int fd)
 {
+    if (debug) fprintf(stderr, "cnx_read_process fd %d\n", fd);
+
     cnx_collection* collection = fd_info->collection;
     struct connection* cnx = collection_get_cnx_from_fd(collection, fd);
     /* Determine active queue (0 or 1): if fd is that of q[1], active_q = 1,
@@ -365,6 +369,8 @@ static void cnx_read_process(struct select_info* fd_info,
 /* Process a connection that is active in write */
 static void cnx_write_process(struct select_info* fd_info, int fd)
 {
+    if (debug) fprintf(stderr, "cnx_write_process fd %d\n", fd);
+
     struct connection* cnx = collection_get_cnx_from_fd(fd_info->collection, fd);
     int res;
     int queue = active_queue(cnx, fd);
@@ -386,6 +392,8 @@ static void cnx_write_process(struct select_info* fd_info, int fd)
 /* Process a connection that accepts a socket */
 void cnx_accept_process(struct select_info* fd_info, int fd)
 {
+    if (debug) fprintf(stderr, "cnx_accept_process fd %d\n", fd);
+
     int in_socket = accept_new_connection(fd, fd_info->collection);
     if (in_socket > 0) {
         fd_info->num_probing++;
@@ -465,18 +473,23 @@ void main_loop(struct listen_endpoint listen_sockets[], int num_addr_listen)
         /* Check all sockets for timeouts */
         /* TODO: refactor to use a list of probing connections to avoid linear
          * search through all connections */
-        for (i = 0; i < collection_get_length(fd_info.collection); i++) {
+        for (i = 0; i < fd_info.max_fd; i++) {
-            struct connection* cnx = collection_get_cnx_from_index(fd_info.collection, i);
+            struct connection* cnx = collection_get_cnx_from_fd(fd_info.collection, i);
-            if ((cnx->state == ST_PROBING) && (cnx->probe_timeout < time(NULL))) {
+            if (cnx) {
-                if (cfg.verbose)
+                if ((cnx->state == ST_PROBING) && (cnx->probe_timeout < time(NULL))) {
-                    fprintf(stderr, "timeout slot %d\n", i);
+                    if (cfg.verbose)
-                cnx_read_process(&fd_info, cnx->q[0].fd);
+                        fprintf(stderr, "timeout slot %d\n", i);
+                    cnx_read_process(&fd_info, cnx->q[0].fd);
+                }
             }
         }
 
         /* Check all sockets for read activity */
         for (i = 0; i < fd_info.max_fd; i++) {
-            if (FD_ISSET(i, &readfds)) {
+            /* Check if it's active AND currently monitored (if a connection
+             * died, it gets tidied, which closes both sockets, but readfs does
+             * not know about that */
+            if (FD_ISSET(i, &readfds) && FD_ISSET(i, &fd_info.fds_r)) {
                 cnx_read_process(&fd_info, i);
             }
         }