Addressed copilot's review comments

Author: danielinux

src/dohd.c

@@ -50,10 +50,25 @@
 #define HTTP2_MODE 1 /* Change to '1' once implemented */
 #define OCSP_RESPONDER 1
 
-/* Memory pool sizes */
+/*
+ * Memory pool sizes:
+ *
+ * MAX_CLIENTS and MAX_REQUESTS define the upper bounds for the internal
+ * connection and request pools. These limits are used to cap memory usage
+ * and reduce dynamic allocations under high load.
+ *
+ * The default values (1024 clients, 4096 requests) are chosen as a
+ * reasonable compromise between memory footprint and concurrency for
+ * typical deployments. If you need to tune these for a specific
+ * environment (e.g. very small embedded systems or large-scale servers),
+ * you can adjust the values at compile time.
+ */
 #define MAX_CLIENTS 1024
 #define MAX_REQUESTS 4096
 
+/* DNS request timeout in milliseconds */
+#define DNS_REQUEST_TIMEOUT_MS 5000
+
 #define IP6_LOCALHOST { 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1 }
 #define DOHD_REQ_MIN 20
 #define STR_HTTP2_PREFACE "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"
@@ -277,9 +292,12 @@ static void dohd_destroy_request(struct req_slot *req);
 static mempool_t *client_pool = NULL;
 static mempool_t *request_pool = NULL;
 
-/* Client hash table for O(1) lookup by fd */
-#define CLIENT_HASH_BITS  10
-#define CLIENT_HASH_SIZE  (1 << CLIENT_HASH_BITS)  /* 1024 buckets */
+/*
+ * Client hash table for O(1) lookup by fd.
+ * Size is 2x MAX_CLIENTS to maintain low load factor and minimize collisions.
+ * Using power-of-2 size enables fast modulo via bitmask.
+ */
+#define CLIENT_HASH_SIZE  (MAX_CLIENTS * 2)  /* 2x capacity for low collisions */
 #define CLIENT_HASH_MASK  (CLIENT_HASH_SIZE - 1)
 
 static struct client_data *client_hash_table[CLIENT_HASH_SIZE];
@@ -475,7 +493,7 @@ struct req_slot *dns_create_request_h2(struct client_data *cd, uint32_t stream_i
     }
     req = mempool_alloc(request_pool);
     if (req == NULL) {
-        dohprint(DOH_ERR, "Failed to allocate memory for a new DNS request.");
+        dohprint(DOH_ERR, "Request pool exhausted (capacity: %u)", MAX_REQUESTS);
         return req;
     }
     req->resolver = next_resolver();
@@ -488,10 +506,18 @@ struct req_slot *dns_create_request_h2(struct client_data *cd, uint32_t stream_i
     req->dns_sd = socket(((struct sockaddr_in *)req->resolver)->sin_family,
             SOCK_DGRAM, 0);
     if (req->dns_sd < 0) {
-        dohprint(DOH_ERR, "Failed to create traditional DNS socket to forward the request.");
+        dohprint(DOH_ERR, "Failed to create UDP socket for DNS request");
         mempool_free(request_pool, req);
         return NULL;
     }
+
+    /* Set socket receive timeout to complement timer-based timeout.
+     * This ensures recv() won't block indefinitely if the event loop
+     * callback fires but data isn't immediately available. */
+    struct timeval tv = { .tv_sec = DNS_REQUEST_TIMEOUT_MS / 1000,
+                          .tv_usec = (DNS_REQUEST_TIMEOUT_MS % 1000) * 1000 };
+    setsockopt(req->dns_sd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
+
     /* append to list */
     req->next = cd->list;
     cd->list = req;
@@ -509,29 +535,31 @@ struct req_slot *dns_create_request_h2(struct client_data *cd, uint32_t stream_i
     return req;
 }
 
-/* DNS request timeout in milliseconds (5 seconds) */
-#define DNS_REQUEST_TIMEOUT_MS 5000
-
 /* Timeout callback - upstream DNS didn't respond in time */
 static void dns_request_timeout(void *arg)
 {
     struct req_slot *req = (struct req_slot *)arg;
     struct client_data *cd;
+    nghttp2_session *session;
 
-    dohprint(DOH_WARN, "DNS request timeout (stream %u)", req->h2_stream_id);
     DOH_Stats.socket_errors++;
 
     /* Use owner_fd to safely look up if client still exists without
-     * dereferencing the potentially-freed owner pointer */
+     * dereferencing the potentially-freed owner pointer.
+     * Note: This is single-threaded, so no race between lookup and use. */
     cd = client_hash_lookup(req->owner_fd);
-    if (cd && cd == req->owner && cd->h2_session) {
-        /* Send HTTP 504 Gateway Timeout to client */
-        nghttp2_nv nva[] = {
-            MAKE_NV(":status", "504"),
-            MAKE_NV("server", "dohd"),
-        };
-        nghttp2_submit_response(cd->h2_session, req->h2_stream_id, nva, 2, NULL);
-        nghttp2_session_send(cd->h2_session);
+    if (cd && cd == req->owner) {
+        /* Capture session pointer after validation */
+        session = cd->h2_session;
+        if (session) {
+            /* Send HTTP 504 Gateway Timeout to client */
+            nghttp2_nv nva[] = {
+                MAKE_NV(":status", "504"),
+                MAKE_NV("server", "dohd"),
+            };
+            nghttp2_submit_response(session, req->h2_stream_id, nva, 2, NULL);
+            nghttp2_session_send(session);
+        }
     }
 
     /* Timer already fired, clear it before destroy */
@@ -553,7 +581,6 @@ static int dns_send_request_h2(struct req_slot *req)
     ret = sendto(req->dns_sd, req->h2_request_buffer, req->h2_request_len, 0,
             (struct sockaddr *)req->resolver, req->resolver_sz);
     if (ret < 0) {
-        dohprint(DOH_ERR, "Resolver send failed: %s\n", strerror(errno));
         DOH_Stats.socket_errors++;
         check_stats();
         dohd_destroy_request(req);
@@ -572,17 +599,6 @@ static int dns_send_request_h2(struct req_slot *req)
 
 static ssize_t client_ssl_write(struct client_data *cd, const void *data, size_t len)
 {
-#ifdef VERBOSE_HTTP_DEBUG
-    int i;
-    uint8_t *reply = (uint8_t *)data;
-    dohprint(DOH_NOTICE,"\n\nSSL Write: %lu bytes\n", len);
-    for (i = 0; i < len; i++) {
-        dohprint(DOH_NOTICE,"%02x ", (reply[i]));
-        if ((i % 16) == 15)
-            dohprint(DOH_NOTICE,"\n");
-    }
-    dohprint(DOH_NOTICE,"\n\n");
-#endif
     return wolfSSL_write(cd->ssl, data, len);
 }
 
@@ -677,7 +693,6 @@ static uint32_t dnsreply_min_age(const void *p, size_t len)
     for (i = 0; i < ntohs(hdr->qdcount); i++) {
         skip = dns_skip_question(&record, len);
         if (skip < DNSQ_SUFFIX_LEN) {
-            dohprint(DOH_WARN, "Cannot parse DNS reply!\n");
             return min_ttl;
         }
         len -= skip;
@@ -819,7 +834,6 @@ static void dohd_reply(int fd, short __attribute__((unused)) revents,
 
     len = recv(fd, buff, bufsz, 0);
     if (len < 16) {
-        dohprint(DOH_ERR, "Error while receiving DNS reply: socket error %d\n", errno);
         goto destroy;
     }
 
@@ -855,7 +869,6 @@ static void dohd_reply(int fd, short __attribute__((unused)) revents,
         data_prd.read_callback = h2_cb_req_submit;
         if (nghttp2_submit_response(req->owner->h2_session,
                 req->h2_stream_id, nva, 4, &data_prd) < 0) {
-            dohprint(DOH_WARN, "H2: submit response failed\n");
             dohd_destroy_request(req);
             return;
         }
@@ -900,15 +913,12 @@ static int h2_cb_on_data_chunk_recv(nghttp2_session *session, uint8_t flags,
     }
     req = nghttp2_session_get_stream_user_data(session, stream_id);
     if (!req) {
-        dohprint(DOH_WARN, "H2: received bogus DATA not associated to request\n");
         goto data_fail;
     }
     if (req->owner != cd) {
-        dohprint(DOH_WARN, "H2: received DATA chunk with wrong stream_id\n");
         goto data_fail;
     }
     if (len > DNS_BUFFER_MAXSIZE) {
-        dohprint(DOH_WARN, "H2: received DATA chunk too large (%lu)\n", len);
         goto data_fail;
     }
     memcpy(req->h2_request_buffer, data, len);
@@ -1003,6 +1013,14 @@ static int h2_cb_on_header(nghttp2_session *session,
             if (!req) {
                 req = dns_create_request_h2(cd, frame->hd.stream_id);
                 if (!req) {
+                    /* Pool exhausted or socket creation failed - send 503 */
+                    nghttp2_nv nva[] = {
+                        MAKE_NV(":status", "503"),
+                        MAKE_NV("server", "dohd"),
+                        MAKE_NV("retry-after", "1"),
+                    };
+                    nghttp2_submit_response(session, frame->hd.stream_id, nva, 3, NULL);
+                    DOH_Stats.socket_errors++;
                     return 0;
                 }
             }

src/libevquick.c

@@ -17,7 +17,15 @@
 #include <fcntl.h>
 #include <assert.h>
 
-#define EVQUICK_MAX_EVENTS 256
+/*
+ * Maximum events returned per epoll_wait call.
+ * This bounds the stack-allocated event array and determines batch size.
+ * Higher values reduce syscall overhead under high load but use more stack.
+ * 512 balances efficiency with typical ulimit stack sizes.
+ */
+#ifndef EVQUICK_MAX_EVENTS
+#define EVQUICK_MAX_EVENTS 512
+#endif
 
 
 
@@ -464,7 +472,7 @@ void evquick_loop(void)
 #else
                 e->err_callback(e->fd, ep_events[i].events, e->arg);
 #endif
-            } else {
+            } else if (e->callback) {
                 /* Call normal callback for read/write or if no err_callback */
 #ifdef EVQUICK_PTHREAD
                 e->callback(ctx, e->fd, ep_events[i].events, e->arg);
@@ -474,7 +482,12 @@ void evquick_loop(void)
             }
         }
 
-        /* Free events that were deleted during this batch */
+        /* Free events that were deleted during this batch.
+         * Note: Deferred freeing prevents use-after-free when events are
+         * deleted from within callbacks. If many events are deleted in one
+         * batch, memory is held until this point. This is acceptable for
+         * typical workloads but could cause temporary memory spikes under
+         * extreme conditions. */
         while (ctx->pending_free) {
             evquick_event *e = ctx->pending_free;
             ctx->pending_free = e->next;

src/mempool.c

@@ -3,6 +3,7 @@
  *
  * Uses a freelist for O(1) alloc/free. Each free slot stores
  * the index of the next free slot, forming a linked list.
+ * A bitmap tracks allocation status to detect double-free attempts.
  */
 
 #include "mempool.h"
@@ -18,13 +19,30 @@ struct slot_header {
 
 struct mempool {
     uint8_t *data;              /* Pool memory */
+    uint8_t *alloc_bitmap;      /* Bitmap: 1 = allocated, 0 = free */
     size_t slot_size;           /* Size of each slot (including header alignment) */
     size_t user_size;           /* User-requested slot size */
     uint32_t capacity;          /* Total number of slots */
     uint32_t free_head;         /* Index of first free slot */
     struct mempool_stats stats; /* Statistics */
 };
 
+/* Bitmap helpers */
+static inline int bitmap_get(uint8_t *bitmap, uint32_t index)
+{
+    return (bitmap[index / 8] >> (index % 8)) & 1;
+}
+
+static inline void bitmap_set(uint8_t *bitmap, uint32_t index)
+{
+    bitmap[index / 8] |= (1 << (index % 8));
+}
+
+static inline void bitmap_clear(uint8_t *bitmap, uint32_t index)
+{
+    bitmap[index / 8] &= ~(1 << (index % 8));
+}
+
 /* Align slot size to at least hold the header and maintain alignment */
 static size_t align_slot_size(size_t user_size)
 {
@@ -39,6 +57,7 @@ mempool_t *mempool_create(size_t slot_size, uint32_t capacity)
     mempool_t *pool;
     uint32_t i;
     size_t aligned_size;
+    size_t bitmap_size;
 
     if (slot_size == 0 || capacity == 0)
         return NULL;
@@ -54,6 +73,16 @@ mempool_t *mempool_create(size_t slot_size, uint32_t capacity)
         return NULL;
     }
 
+    /* Allocate bitmap: 1 bit per slot, rounded up to bytes */
+    bitmap_size = (capacity + 7) / 8;
+    pool->alloc_bitmap = malloc(bitmap_size);
+    if (!pool->alloc_bitmap) {
+        free(pool->data);
+        free(pool);
+        return NULL;
+    }
+    memset(pool->alloc_bitmap, 0, bitmap_size);
+
     pool->slot_size = aligned_size;
     pool->user_size = slot_size;
     pool->capacity = capacity;
@@ -80,6 +109,7 @@ void mempool_destroy(mempool_t *pool)
 {
     if (!pool)
         return;
+    free(pool->alloc_bitmap);
     free(pool->data);
     free(pool);
 }
@@ -88,6 +118,7 @@ void *mempool_alloc(mempool_t *pool)
 {
     struct slot_header *slot;
     void *ptr;
+    uint32_t index;
 
     if (!pool || pool->free_head == SLOT_END) {
         if (pool)
@@ -96,9 +127,13 @@ void *mempool_alloc(mempool_t *pool)
     }
 
     /* Pop from freelist */
-    slot = (struct slot_header *)(pool->data + pool->free_head * pool->slot_size);
+    index = pool->free_head;
+    slot = (struct slot_header *)(pool->data + index * pool->slot_size);
     pool->free_head = slot->next_free;
 
+    /* Mark as allocated in bitmap */
+    bitmap_set(pool->alloc_bitmap, index);
+
     /* Zero the slot for safety */
     ptr = (void *)slot;
     memset(ptr, 0, pool->user_size);
@@ -132,6 +167,15 @@ void mempool_free(mempool_t *pool, void *ptr)
     if ((uint8_t *)ptr != pool->data + index * pool->slot_size)
         return; /* Misaligned pointer */
 
+    /* Check for double-free using bitmap */
+    if (!bitmap_get(pool->alloc_bitmap, index)) {
+        pool->stats.double_free++;
+        return; /* Already free - ignore to prevent corruption */
+    }
+
+    /* Mark as free in bitmap */
+    bitmap_clear(pool->alloc_bitmap, index);
+
     /* Push onto freelist */
     slot = (struct slot_header *)ptr;
     slot->next_free = pool->free_head;
@@ -153,10 +197,15 @@ void mempool_stats(mempool_t *pool, struct mempool_stats *stats)
 void mempool_reset(mempool_t *pool)
 {
     uint32_t i;
+    size_t bitmap_size;
 
     if (!pool)
         return;
 
+    /* Clear allocation bitmap */
+    bitmap_size = (pool->capacity + 7) / 8;
+    memset(pool->alloc_bitmap, 0, bitmap_size);
+
     /* Rebuild freelist */
     for (i = 0; i < pool->capacity - 1; i++) {
         struct slot_header *h = (struct slot_header *)(pool->data + i * pool->slot_size);

src/mempool.h

@@ -19,6 +19,7 @@ struct mempool_stats {
     uint32_t alloc_count;   /* Total allocations */
     uint32_t free_count;    /* Total frees */
     uint32_t exhausted;     /* Times pool was full */
+    uint32_t double_free;   /* Double-free attempts detected */
 };
 
 /* Opaque pool handle */

test/test_mempool.c

@@ -425,15 +425,20 @@ TEST(double_free_safe)
 
     mempool_free(pool, p);
 
-    /* Double free - pointer is now part of freelist, 
-     * but freeing again should be handled gracefully.
-     * The slot is already free, so this is a no-op or
-     * will corrupt the freelist. We just verify no crash. */
+    /* Double free - should be detected and ignored via bitmap */
     mempool_free(pool, p);
 
-    /* Pool should still be usable (though potentially corrupted) */
+    /* Verify double-free was detected */
+    struct mempool_stats stats;
+    mempool_stats(pool, &stats);
+    ASSERT_EQ(stats.double_free, 1);
+    ASSERT_EQ(stats.allocated, 0);
+    ASSERT_EQ(stats.free_count, 1);  /* Only first free counted */
+
+    /* Pool should still be usable and NOT corrupted */
     void *p2 = mempool_alloc(pool);
     ASSERT_NOT_NULL(p2);
+    ASSERT_EQ(p2, p);  /* Same slot reused */
 
     mempool_destroy(pool);
 }