From 83f6b729e1e1bd1038bf44180f88986f661765f1 Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 10:46:52 +1000 Subject: [PATCH 01/10] dns_tcp: prevent frame length overflow --- CMakeLists.txt | 11 ++++ src/dns_listener_tcp.c | 18 +++++- tests/unit/test_dns_listener_tcp.c | 89 ++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 3 deletions(-) create mode 100644 tests/unit/test_dns_listener_tcp.c diff --git a/CMakeLists.txt b/CMakeLists.txt index c067c79..0d967f6 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -236,6 +236,17 @@ if(BUILD_TESTING) add_test(NAME resolver_update COMMAND resolver_update_test) set_tests_properties(resolver_update PROPERTIES LABELS unit) + add_executable(dns_listener_tcp_test + tests/unit/test_dns_listener_tcp.c + src/logging.c + src/ring_buffer.c) + set_property(SOURCE tests/unit/test_dns_listener_tcp.c APPEND + PROPERTY COMPILE_DEFINITIONS __FILENAME__="dns_listener_tcp_test") + target_link_libraries(dns_listener_tcp_test ev) + set_property(TARGET dns_listener_tcp_test PROPERTY C_STANDARD 11) + add_test(NAME dns_listener_tcp COMMAND dns_listener_tcp_test) + set_tests_properties(dns_listener_tcp PROPERTIES LABELS unit) + find_program(VALGRIND_EXE NAMES valgrind) if(VALGRIND_EXE) add_test(NAME dns_truncate_valgrind diff --git a/src/dns_listener_tcp.c b/src/dns_listener_tcp.c index 3b6d101..30fe07f 100644 --- a/src/dns_listener_tcp.c +++ b/src/dns_listener_tcp.c @@ -110,12 +110,24 @@ static void remove_client(struct tcp_client_s * client) { static int get_dns_request(struct tcp_client_s *client, char ** dns_req, uint16_t * req_size) { + if (client->input_buffer_used < sizeof(uint16_t)) { + return 0; // Partial length prefix + } + // check if whole request is available - *req_size = ntohs(*((uint16_t*)client->input_buffer)); - uint16_t data_size = sizeof(uint16_t) + *req_size; + uint16_t network_req_size = 0; + memcpy(&network_req_size, client->input_buffer, sizeof(network_req_size)); + *req_size = ntohs(network_req_size); + const size_t data_size = sizeof(uint16_t) + (size_t)*req_size; if (data_size > client->input_buffer_used) { return 0; // Partial request } + if (*req_size < DNS_HEADER_LENGTH) { + client->input_buffer_used -= (uint32_t)data_size; + memmove(client->input_buffer, client->input_buffer + data_size, client->input_buffer_used); + *dns_req = NULL; + return 1; + } // copy whole request *dns_req = (char *)malloc(*req_size); // freed when DoH request completes if (*dns_req == NULL) { @@ -123,7 +135,7 @@ static int get_dns_request(struct tcp_client_s *client, } memcpy(*dns_req, client->input_buffer + sizeof(uint16_t), *req_size); // move down data of next request(s) if any - client->input_buffer_used -= data_size; + client->input_buffer_used -= (uint32_t)data_size; memmove(client->input_buffer, client->input_buffer + data_size, client->input_buffer_used); return 1; } diff --git a/tests/unit/test_dns_listener_tcp.c b/tests/unit/test_dns_listener_tcp.c new file mode 100644 index 0000000..cffb354 --- /dev/null +++ b/tests/unit/test_dns_listener_tcp.c @@ -0,0 +1,89 @@ +#include +#include +#include + +#include "test_harness.h" +#include "../../src/dns_listener_tcp.c" + +static struct tcp_client_s client; +static char *dns_request; + +static void setUp(void) { + memset(&client, 0, sizeof(client)); + client.input_buffer_size = 64; + client.input_buffer = (char *)calloc(1, client.input_buffer_size); + dns_request = NULL; +} + +static void tearDown(void) { + free(dns_request); + free(client.input_buffer); +} + +static void test_waits_for_complete_length_prefix(void) { + client.input_buffer[0] = (char)UINT8_MAX; + client.input_buffer_used = 1; + uint16_t request_size = 0; + + const int received = get_dns_request(&client, &dns_request, &request_size); + + TEST_ASSERT(received == 0); + TEST_ASSERT(dns_request == NULL); + TEST_ASSERT(client.input_buffer_used == 1); +} + +static void test_large_length_does_not_wrap_frame_size(void) { + client.input_buffer[0] = (char)UINT8_MAX; + client.input_buffer[1] = (char)UINT8_MAX; + client.input_buffer_used = 2; + uint16_t request_size = 0; + + const int received = get_dns_request(&client, &dns_request, &request_size); + + TEST_ASSERT(received == 0); + TEST_ASSERT(request_size == UINT16_MAX); + TEST_ASSERT(dns_request == NULL); + TEST_ASSERT(client.input_buffer_used == 2); +} + +static void test_rejects_zero_length_request_without_allocating(void) { + client.input_buffer[0] = 0; + client.input_buffer[1] = 0; + client.input_buffer_used = sizeof(uint16_t); + uint16_t request_size = UINT16_MAX; + + const int received = get_dns_request(&client, &dns_request, &request_size); + + TEST_ASSERT(received == 1); + TEST_ASSERT(request_size == 0); + TEST_ASSERT(dns_request == NULL); + TEST_ASSERT(client.input_buffer_used == 0); +} + +static void test_extracts_complete_request(void) { + enum { REQUEST_SIZE = DNS_HEADER_LENGTH }; + client.input_buffer[0] = 0; + client.input_buffer[1] = REQUEST_SIZE; + for (size_t i = 0; i < REQUEST_SIZE; i++) { + client.input_buffer[sizeof(uint16_t) + i] = (char)i; + } + client.input_buffer_used = sizeof(uint16_t) + REQUEST_SIZE; + uint16_t request_size = 0; + + const int received = get_dns_request(&client, &dns_request, &request_size); + + TEST_ASSERT(received == 1); + TEST_ASSERT(request_size == REQUEST_SIZE); + TEST_ASSERT(client.input_buffer_used == 0); + for (size_t i = 0; i < REQUEST_SIZE; i++) { + TEST_ASSERT((uint8_t)dns_request[i] == i); + } +} + +int main(void) { + TEST_RUN(test_waits_for_complete_length_prefix); + TEST_RUN(test_large_length_does_not_wrap_frame_size); + TEST_RUN(test_rejects_zero_length_request_without_allocating); + TEST_RUN(test_extracts_complete_request); + return test_summary(); +} From 1fa4e766518fb773a3c1422b811bbd4032e70b3a Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 11:10:38 +1000 Subject: [PATCH 02/10] https_client: bound in-flight requests --- CMakeLists.txt | 13 +++ src/dns_listener.h | 3 + src/dns_listener_tcp.c | 127 +++++++++++++++++--------- src/dns_listener_udp.c | 5 + src/doh_proxy.c | 2 + src/https_client.c | 10 ++ src/https_client.h | 2 + tests/unit/test_dns_listener_tcp.c | 30 ++++++ tests/unit/test_https_client_limits.c | 82 +++++++++++++++++ 9 files changed, 231 insertions(+), 43 deletions(-) create mode 100644 tests/unit/test_https_client_limits.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 0d967f6..63427a4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -247,6 +247,19 @@ if(BUILD_TESTING) add_test(NAME dns_listener_tcp COMMAND dns_listener_tcp_test) set_tests_properties(dns_listener_tcp PROPERTIES LABELS unit) + add_executable(https_client_limits_test + tests/unit/test_https_client_limits.c + src/https_client.c + src/logging.c + src/ring_buffer.c + src/stat.c) + set_property(SOURCE tests/unit/test_https_client_limits.c APPEND + PROPERTY COMPILE_DEFINITIONS __FILENAME__="https_client_limits_test") + target_link_libraries(https_client_limits_test curl ev m) + set_property(TARGET https_client_limits_test PROPERTY C_STANDARD 11) + add_test(NAME https_client_limits COMMAND https_client_limits_test) + set_tests_properties(https_client_limits PROPERTIES LABELS unit) + find_program(VALGRIND_EXE NAMES valgrind) if(VALGRIND_EXE) add_test(NAME dns_truncate_valgrind diff --git a/src/dns_listener.h b/src/dns_listener.h index 7ec12f2..45fb4bb 100644 --- a/src/dns_listener.h +++ b/src/dns_listener.h @@ -33,6 +33,9 @@ struct dns_listener { void (*respond)(dns_listener_t *self, struct sockaddr *raddr, const char *dns_req, size_t dns_req_len, char *dns_resp, size_t dns_resp_len); + // Release any transport-level capacity reserved for this request. The + // proxy core calls this exactly once whether the request succeeds or fails. + void (*request_complete)(dns_listener_t *self, struct sockaddr *raddr); // Stop accepting new requests. Existing per-client state (TCP) is retained // so any in-flight DoH responses can still be delivered during graceful // drain. diff --git a/src/dns_listener_tcp.c b/src/dns_listener_tcp.c index 30fe07f..d11c09a 100644 --- a/src/dns_listener_tcp.c +++ b/src/dns_listener_tcp.c @@ -34,6 +34,7 @@ enum { RESPONSE_SEND_ATTEMPTS = 50, // 0.025 sec max wait RESPONSE_SEND_DELAY_US = 500, // 0.0005 sec TCP_DNS_MAX_PAYLOAD = UINT16_MAX - sizeof(uint16_t), // Max after 2-byte length prefix + TCP_CLIENT_REQUEST_LIMIT = 16, }; typedef struct dns_listener_tcp_s dns_listener_tcp_t; @@ -50,6 +51,7 @@ struct tcp_client_s { char * input_buffer; uint32_t input_buffer_size; uint32_t input_buffer_used; + uint16_t pending_requests; ev_io read_watcher; ev_timer timer_watcher; @@ -75,6 +77,15 @@ struct dns_listener_tcp_s { struct tcp_client_s * clients; }; +static struct tcp_client_s *find_client(dns_listener_tcp_t *d, + struct sockaddr *raddr) { + for (struct tcp_client_s *client = d->clients; client != NULL; client = client->next) { + if (memcmp(raddr, &client->raddr, client->addr_len) == 0) { + return client; + } + } + return NULL; +} static void remove_client(struct tcp_client_s * client) { dns_listener_tcp_t *d = client->d; @@ -148,57 +159,67 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, // Receive data char buf[DNS_REQUEST_BUFFER_SIZE]; // if there would be more data, callback will be called again ssize_t len = recv(w->fd, buf, DNS_REQUEST_BUFFER_SIZE, 0); - if (len <= 0) { - if (len == 0 || errno == ECONNRESET) { + if (len == 0) { + DLOG_CLIENT("TCP client closed connection"); + remove_client(client); + return; + } + if (len < 0) { + if (errno == ECONNRESET) { DLOG_CLIENT("TCP client closed connection"); } else if (errno == EAGAIN || errno == EWOULDBLOCK) { - return; + len = 0; } else { WLOG_CLIENT("Read error: %s (%d), dropping client", strerror(errno), errno); } - remove_client(client); - return; + if (len < 0) { + remove_client(client); + return; + } } - // Append data into input buffer - // Check for integer overflow and maximum message size - if (len > UINT16_MAX || client->input_buffer_used > UINT16_MAX - (uint32_t)len) { - WLOG_CLIENT("Request too large, dropping client"); - remove_client(client); - return; - } - const uint32_t free_space = client->input_buffer_size - client->input_buffer_used; - const uint32_t needed_space = client->input_buffer_used + (uint32_t)len; - // Limit buffer size to prevent memory exhaustion attacks - if (needed_space > TCP_DNS_MAX_PAYLOAD) { - WLOG_CLIENT("Request too large, dropping client"); - remove_client(client); - return; - } - DLOG_CLIENT("Received %d byte, free: %u", len, free_space); - if (free_space < len) { - for (client->input_buffer_size = 64; // lower value does not make much sense - client->input_buffer_size < needed_space; - client->input_buffer_size *= 2) { - if (client->input_buffer_size > TCP_DNS_MAX_PAYLOAD) { - FLOG_CLIENT("Unrealistic input buffer size: %u", client->input_buffer_size); - } + if (len > 0) { + // Append data into input buffer + // Check for integer overflow and maximum message size + if (len > UINT16_MAX || client->input_buffer_used > UINT16_MAX - (uint32_t)len) { + WLOG_CLIENT("Request too large, dropping client"); + remove_client(client); + return; } - DLOG_CLIENT("Resize input buffer to %u", client->input_buffer_size); - client->input_buffer = (char *) realloc((void*) client->input_buffer, // NOLINT(bugprone-suspicious-realloc-usage) if realloc fails, program stops - client->input_buffer_size); - if (client->input_buffer == NULL) { - FLOG_CLIENT("Out of mem"); + const uint32_t free_space = client->input_buffer_size - client->input_buffer_used; + const uint32_t needed_space = client->input_buffer_used + (uint32_t)len; + // Limit buffer size to prevent memory exhaustion attacks + if (needed_space > TCP_DNS_MAX_PAYLOAD) { + WLOG_CLIENT("Request too large, dropping client"); + remove_client(client); + return; + } + DLOG_CLIENT("Received %d byte, free: %u", len, free_space); + if (free_space < len) { + for (client->input_buffer_size = 64; // lower value does not make much sense + client->input_buffer_size < needed_space; + client->input_buffer_size *= 2) { + if (client->input_buffer_size > TCP_DNS_MAX_PAYLOAD) { + FLOG_CLIENT("Unrealistic input buffer size: %u", client->input_buffer_size); + } + } + DLOG_CLIENT("Resize input buffer to %u", client->input_buffer_size); + client->input_buffer = (char *) realloc((void*) client->input_buffer, // NOLINT(bugprone-suspicious-realloc-usage) if realloc fails, program stops + client->input_buffer_size); + if (client->input_buffer == NULL) { + FLOG_CLIENT("Out of mem"); + } } + memcpy(client->input_buffer + client->input_buffer_used, buf, (size_t)len); + client->input_buffer_used = needed_space; } - memcpy(client->input_buffer + client->input_buffer_used, buf, (size_t)len); - client->input_buffer_used = needed_space; // Split requests char *dns_req = NULL; uint16_t req_size = 0; uint8_t request_received = 0; - while (get_dns_request(client, &dns_req, &req_size)) { + while (client->pending_requests < TCP_CLIENT_REQUEST_LIMIT && + get_dns_request(client, &dns_req, &req_size)) { if (req_size < DNS_HEADER_LENGTH) { WLOG_CLIENT("Malformed request received, too short: %u, dropping client", req_size); free(dns_req); @@ -207,6 +228,10 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, } DLOG_CLIENT("Requested %04hX", ntohs(*((uint16_t*)dns_req))); + client->pending_requests++; + if (client->pending_requests == TCP_CLIENT_REQUEST_LIMIT) { + ev_io_stop(d->loop, &client->read_watcher); + } d->cb(d->cb_data, &d->base, (struct sockaddr*)&client->raddr, dns_req, req_size); request_received = 1; } @@ -216,6 +241,27 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, } } +static void tcp_request_complete(dns_listener_t *self, struct sockaddr *raddr) { + dns_listener_tcp_t *d = (dns_listener_tcp_t *)self; + struct tcp_client_s *client = find_client(d, raddr); + if (client == NULL) { + return; + } + if (client->pending_requests == 0) { + WLOG_CLIENT("Completed request with no request pending"); + return; + } + + const int resume_reads = client->pending_requests == TCP_CLIENT_REQUEST_LIMIT; + client->pending_requests--; + if (resume_reads) { + ev_io_start(d->loop, &client->read_watcher); + if (client->input_buffer_used > 0) { + ev_feed_event(d->loop, &client->read_watcher, EV_READ); + } + } +} + static void timer_cb(struct ev_loop __attribute__((unused)) *loop, ev_timer *w, int __attribute__((unused)) revents) { struct tcp_client_s *client = (struct tcp_client_s *)w->data; @@ -350,13 +396,7 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr, const uint16_t response_id = ntohs(*((uint16_t*)resp)); // find client data - struct tcp_client_s *client = NULL; - for (struct tcp_client_s * cur = d->clients; cur != NULL; cur = cur->next) { - if (memcmp(raddr, &(cur->raddr), cur->addr_len) == 0) { - client = cur; - break; - } - } + struct tcp_client_s *client = find_client(d, raddr); if (client == NULL) { WLOG("Could not find client, can not send DNS response: %04hX", response_id); return; @@ -432,6 +472,7 @@ dns_listener_t * dns_tcp_listener_create(struct ev_loop *loop, FLOG("Out of mem"); } d->base.respond = tcp_respond; + d->base.request_complete = tcp_request_complete; d->base.stop = tcp_stop; d->base.destroy = tcp_destroy; d->base.transport = DNS_TRANSPORT_TCP; diff --git a/src/dns_listener_udp.c b/src/dns_listener_udp.c index 96cef08..e2e09ba 100644 --- a/src/dns_listener_udp.c +++ b/src/dns_listener_udp.c @@ -102,6 +102,10 @@ static void udp_respond(dns_listener_t *self, struct sockaddr *raddr, } } +static void udp_request_complete(dns_listener_t __attribute__((unused)) *self, + struct sockaddr __attribute__((unused)) *raddr) { +} + static void udp_stop(dns_listener_t *self) { dns_listener_udp_t *d = (dns_listener_udp_t *)self; ev_io_stop(d->loop, &d->watcher); @@ -121,6 +125,7 @@ dns_listener_t * dns_udp_listener_create(struct ev_loop *loop, FLOG("Out of mem"); } d->base.respond = udp_respond; + d->base.request_complete = udp_request_complete; d->base.stop = udp_stop; d->base.destroy = udp_destroy; d->base.transport = DNS_TRANSPORT_UDP; diff --git a/src/doh_proxy.c b/src/doh_proxy.c index e148e3e..9d64389 100644 --- a/src/doh_proxy.c +++ b/src/doh_proxy.c @@ -200,6 +200,7 @@ static void doh_response_cb(void *data, char *buf, size_t buflen) { } } + req->listener->request_complete(req->listener, (struct sockaddr*)&req->raddr); free((void*)req->dns_req); free(req); } @@ -214,6 +215,7 @@ void doh_proxy_handle_request(void *ctx, dns_listener_t *listener, if (p->awaiting_bootstrap) { WLOG("%04hX: Query received before bootstrapping is completed, discarding.", req_id); + listener->request_complete(listener, raddr); free(dns_req); return; } diff --git a/src/https_client.c b/src/https_client.c index fdafb03..a73cc74 100644 --- a/src/https_client.c +++ b/src/https_client.c @@ -287,6 +287,7 @@ static void https_fetch_ctx_init(https_client_t *client, ctx->buflen = 0; ctx->next = client->fetches; client->fetches = ctx; + client->fetch_count++; ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_RESOLVE, resolv); @@ -555,6 +556,10 @@ static void https_fetch_ctx_cleanup(https_client_t *client, } else { client->fetches = ctx->next; } + if (client->fetch_count == 0) { + FLOG_REQ("HTTPS fetch count underflow"); + } + client->fetch_count--; free(ctx); } @@ -718,6 +723,11 @@ void https_client_fetch(https_client_t *c, const char *url, const char* postdata, size_t postdata_len, struct curl_slist *resolv, uint16_t id, https_response_cb cb, void *data) { + if (c->fetch_count >= HTTPS_FETCH_LIMIT) { + WLOG("%04hX: Too many HTTPS requests in flight, dropping request", id); + cb(data, NULL, 0); + return; + } struct https_fetch_ctx *ctx = (struct https_fetch_ctx *)calloc(1, sizeof(struct https_fetch_ctx)); if (!ctx) { diff --git a/src/https_client.h b/src/https_client.h index de60bbb..419bfcb 100644 --- a/src/https_client.h +++ b/src/https_client.h @@ -9,6 +9,7 @@ enum { HTTPS_SOCKET_LIMIT = 12, HTTPS_CONNECTION_LIMIT = 8, + HTTPS_FETCH_LIMIT = 64, }; // Callback type for receiving data when a transfer finishes. @@ -36,6 +37,7 @@ typedef struct { CURLM *curlm; struct curl_slist *header_list; struct https_fetch_ctx *fetches; + size_t fetch_count; ev_timer timer; ev_io io_events[HTTPS_SOCKET_LIMIT]; diff --git a/tests/unit/test_dns_listener_tcp.c b/tests/unit/test_dns_listener_tcp.c index cffb354..b552689 100644 --- a/tests/unit/test_dns_listener_tcp.c +++ b/tests/unit/test_dns_listener_tcp.c @@ -1,6 +1,8 @@ #include #include #include +#include +#include #include "test_harness.h" #include "../../src/dns_listener_tcp.c" @@ -80,10 +82,38 @@ static void test_extracts_complete_request(void) { } } +static void test_request_completion_resumes_client_reads(void) { + int sockets[2]; + TEST_ASSERT(socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) == 0); + struct ev_loop *loop = ev_loop_new(0); + TEST_ASSERT(loop != NULL); + + dns_listener_tcp_t listener; + memset(&listener, 0, sizeof(listener)); + listener.loop = loop; + listener.clients = &client; + client.d = &listener; + client.sock = sockets[0]; + client.pending_requests = TCP_CLIENT_REQUEST_LIMIT; + ev_io_init(&client.read_watcher, read_cb, client.sock, EV_READ); + client.read_watcher.data = &client; + + tcp_request_complete(&listener.base, (struct sockaddr*)&client.raddr); + + TEST_ASSERT(client.pending_requests == TCP_CLIENT_REQUEST_LIMIT - 1); + TEST_ASSERT(ev_is_active(&client.read_watcher)); + + ev_io_stop(loop, &client.read_watcher); + ev_loop_destroy(loop); + close(sockets[0]); + close(sockets[1]); +} + int main(void) { TEST_RUN(test_waits_for_complete_length_prefix); TEST_RUN(test_large_length_does_not_wrap_frame_size); TEST_RUN(test_rejects_zero_length_request_without_allocating); TEST_RUN(test_extracts_complete_request); + TEST_RUN(test_request_completion_resumes_client_reads); return test_summary(); } diff --git a/tests/unit/test_https_client_limits.c b/tests/unit/test_https_client_limits.c new file mode 100644 index 0000000..fddc01a --- /dev/null +++ b/tests/unit/test_https_client_limits.c @@ -0,0 +1,82 @@ +#include +#include +#include +#include + +#include "dns_common.h" +#include "https_client.h" +#include "test_harness.h" + +typedef struct { + size_t calls; + size_t failures; +} callback_state_t; + +static struct ev_loop *loop; +static https_client_t client; +static options_t options; +static callback_state_t callback_state; + +static void response_cb(void *data, char *buf, size_t buflen) { + callback_state_t *state = (callback_state_t *)data; + state->calls++; + if (buf == NULL && buflen == 0) { + state->failures++; + } +} + +static void setUp(void) { + memset(&client, 0, sizeof(client)); + memset(&options, 0, sizeof(options)); + memset(&callback_state, 0, sizeof(callback_state)); + options.use_http_version = 1; + options.max_idle_time = 118; + options.conn_loss_time = 15; + loop = ev_loop_new(0); + if (loop == NULL) { + test_fail(__FILE__, __LINE__, "ev_loop_new(0)"); + return; + } + if (curl_global_init(CURL_GLOBAL_DEFAULT) != CURLE_OK) { + test_fail(__FILE__, __LINE__, "curl_global_init"); + return; + } + https_client_init(&client, &options, NULL, loop); +} + +static void tearDown(void) { + if (client.curlm != NULL) { + https_client_cleanup(&client); + } + curl_global_cleanup(); + if (loop != NULL) { + ev_loop_destroy(loop); + loop = NULL; + } +} + +static void test_rejects_requests_above_fetch_limit(void) { + static const char request[DNS_HEADER_LENGTH] = {0}; + + for (size_t i = 0; i < HTTPS_FETCH_LIMIT; i++) { + https_client_fetch(&client, "https://127.0.0.1/dns-query", + request, sizeof(request), NULL, (uint16_t)i, + response_cb, &callback_state); + } + + TEST_ASSERT(client.fetch_count == HTTPS_FETCH_LIMIT); + TEST_ASSERT(callback_state.calls == 0); + + https_client_fetch(&client, "https://127.0.0.1/dns-query", + request, sizeof(request), NULL, UINT16_MAX, + response_cb, &callback_state); + + TEST_ASSERT(client.fetch_count == HTTPS_FETCH_LIMIT); + TEST_ASSERT(callback_state.calls == 1); + TEST_ASSERT(callback_state.failures == 1); +} + +int main(void) { + TEST_RUN(test_rejects_requests_above_fetch_limit); + return test_summary(); +} From 2746cdd6fff4f61ddc09b23e02880708acaa3536 Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 00:48:58 +1000 Subject: [PATCH 03/10] options: drop to user's primary group --- README.md | 1 + src/options.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index fc61f0e..0c2b9e0 100644 --- a/README.md +++ b/README.md @@ -204,6 +204,7 @@ Usage: ./https_dns_proxy [-a ] [-p ] [-T uid = p->pw_uid; + if (!opt->group) { + opt->gid = p->pw_gid; + } } if (opt->group) { struct group *g = getgrnam(opt->group); @@ -169,7 +172,8 @@ enum OptionsParseResult options_parse_args(struct Options *opt, int argc, char * } opt->dscp <<= 2; // Get noisy about bad security practices. - if (getuid() == 0 && (!opt->user || !opt->group)) { + if (getuid() == 0 && + (opt->uid == (uid_t)-1 || opt->gid == (gid_t)-1)) { printf("----------------------------\n" "WARNING: Running as root without dropping privileges " "is NOT recommended.\n" @@ -278,6 +282,7 @@ void options_show_usage(int __attribute__((unused)) argc, char **argv) { printf("\n Process\n"); printf(" -d Daemonize.\n"); printf(" -u user Optional user to drop to if launched as root.\n"); + printf(" Also uses the user's primary group unless -g is set.\n"); printf(" -g group Optional group to drop to if launched as root.\n"); printf("\n Logging\n"); printf(" -v Increase logging verbosity. (Default: error)\n"); From d33431a27a8ee3ce8a78480feb108602a55d2669 Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 11:11:29 +1000 Subject: [PATCH 04/10] doh_proxy: detect newly added resolver addresses --- CMakeLists.txt | 11 ++++ tests/unit/test_doh_proxy_resolver.c | 98 ++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+) create mode 100644 tests/unit/test_doh_proxy_resolver.c diff --git a/CMakeLists.txt b/CMakeLists.txt index 63427a4..de9e084 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -260,6 +260,17 @@ if(BUILD_TESTING) add_test(NAME https_client_limits COMMAND https_client_limits_test) set_tests_properties(https_client_limits PROPERTIES LABELS unit) + add_executable(doh_proxy_resolver_test + tests/unit/test_doh_proxy_resolver.c + src/logging.c + src/ring_buffer.c) + set_property(SOURCE tests/unit/test_doh_proxy_resolver.c APPEND + PROPERTY COMPILE_DEFINITIONS __FILENAME__="doh_proxy_resolver_test") + target_link_libraries(doh_proxy_resolver_test curl ev) + set_property(TARGET doh_proxy_resolver_test PROPERTY C_STANDARD 11) + add_test(NAME doh_proxy_resolver COMMAND doh_proxy_resolver_test) + set_tests_properties(doh_proxy_resolver PROPERTIES LABELS unit) + find_program(VALGRIND_EXE NAMES valgrind) if(VALGRIND_EXE) add_test(NAME dns_truncate_valgrind diff --git a/tests/unit/test_doh_proxy_resolver.c b/tests/unit/test_doh_proxy_resolver.c new file mode 100644 index 0000000..bb4b620 --- /dev/null +++ b/tests/unit/test_doh_proxy_resolver.c @@ -0,0 +1,98 @@ +#include +#include + +#include "test_harness.h" +#include "../../src/doh_proxy.c" + +static https_client_t client; +static doh_proxy_t *proxy; +static unsigned reset_calls; + +void https_client_reset(https_client_t *unused_client) { + (void)unused_client; + reset_calls++; +} + +void https_client_fetch(https_client_t *unused_client, const char *url, + const char *postdata, size_t postdata_len, + struct curl_slist *resolv, uint16_t id, + https_response_cb cb, void *data) { + (void)unused_client; + (void)url; + (void)postdata; + (void)postdata_len; + (void)resolv; + (void)id; + (void)cb; + (void)data; +} + +void stat_request_begin(stat_t *stat, size_t request_len, uint8_t is_tcp) { + (void)stat; + (void)request_len; + (void)is_tcp; +} + +void stat_request_end(stat_t *stat, size_t response_len, + ev_tstamp latency, uint8_t is_tcp) { + (void)stat; + (void)response_len; + (void)latency; + (void)is_tcp; +} + +static void setUp(void) { + memset(&client, 0, sizeof(client)); + reset_calls = 0; + proxy = doh_proxy_create(NULL, &client, "https://resolver.test/dns-query", NULL); + doh_proxy_set_resolv(proxy, "resolver.test:443:192.0.2.1,192.0.2.2"); +} + +static void tearDown(void) { + doh_proxy_destroy(proxy); + proxy = NULL; +} + +static void update_resolver(const char *addresses) { + char *owned_addresses = strdup(addresses); + if (owned_addresses == NULL) { + test_fail(__FILE__, __LINE__, "strdup(addresses)"); + return; + } + doh_proxy_handle_resolver_update("resolver.test", proxy, owned_addresses); +} + +static void test_new_address_triggers_resolver_update(void) { + update_resolver("192.0.2.1,192.0.2.2,192.0.2.3"); + + TEST_ASSERT(reset_calls == 1); + TEST_ASSERT(strcmp(proxy->resolv->data, + "resolver.test:443:192.0.2.1,192.0.2.2,192.0.2.3") == 0); +} + +static void test_removed_address_triggers_resolver_update(void) { + update_resolver("192.0.2.1"); + + TEST_ASSERT(reset_calls == 1); + TEST_ASSERT(strcmp(proxy->resolv->data, + "resolver.test:443:192.0.2.1") == 0); +} + +static void test_exact_address_match_after_prefix_match(void) { + curl_slist_free_all(proxy->resolv); + proxy->resolv = NULL; + doh_proxy_set_resolv(proxy, "resolver.test:443:192.0.2.10,192.0.2.1"); + + update_resolver("192.0.2.1,192.0.2.10"); + + TEST_ASSERT(reset_calls == 0); + TEST_ASSERT(strcmp(proxy->resolv->data, + "resolver.test:443:192.0.2.10,192.0.2.1") == 0); +} + +int main(void) { + TEST_RUN(test_new_address_triggers_resolver_update); + TEST_RUN(test_removed_address_triggers_resolver_update); + TEST_RUN(test_exact_address_match_after_prefix_match); + return test_summary(); +} From 63de734fdeaf7955b78fccdb10339c3bff740f9c Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 11:38:38 +1000 Subject: [PATCH 05/10] dns_tcp: queue responses without blocking --- CMakeLists.txt | 2 +- src/dns_listener.h | 8 +- src/dns_listener_tcp.c | 288 ++++++++++++++------- src/dns_listener_udp.c | 6 +- src/doh_proxy.c | 10 +- src/doh_proxy.h | 2 +- tests/unit/test_dns_listener_tcp.c | 368 +++++++++++++++++++++++++-- tests/unit/test_doh_proxy_resolver.c | 66 ++++- 8 files changed, 635 insertions(+), 115 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index de9e084..0046a15 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -245,7 +245,7 @@ if(BUILD_TESTING) target_link_libraries(dns_listener_tcp_test ev) set_property(TARGET dns_listener_tcp_test PROPERTY C_STANDARD 11) add_test(NAME dns_listener_tcp COMMAND dns_listener_tcp_test) - set_tests_properties(dns_listener_tcp PROPERTIES LABELS unit) + set_tests_properties(dns_listener_tcp PROPERTIES LABELS unit TIMEOUT 20) add_executable(https_client_limits_test tests/unit/test_https_client_limits.c diff --git a/src/dns_listener.h b/src/dns_listener.h index 45fb4bb..4aa11ec 100644 --- a/src/dns_listener.h +++ b/src/dns_listener.h @@ -2,6 +2,7 @@ #define _DNS_LISTENER_H_ #include +#include #include // A DNS listener accepts requests on some transport (UDP, TCP, ...) and routes @@ -23,19 +24,20 @@ typedef enum { // Invoked once per fully-received DNS request. `dns_req` is heap-allocated // and ownership transfers to the callee. `listener` is a back-pointer the // callee uses later to deliver the matching response. +// connection_id identifies the TCP connection; UDP uses zero. typedef void (*dns_request_fn)(void *ctx, dns_listener_t *listener, - struct sockaddr *raddr, + struct sockaddr *raddr, uint64_t connection_id, char *dns_req, size_t dns_req_len); struct dns_listener { // Send `dns_resp` to `raddr`. UDP listeners may EDNS-truncate the response // in place using `dns_req`; TCP listeners ignore the request bytes. - void (*respond)(dns_listener_t *self, struct sockaddr *raddr, + void (*respond)(dns_listener_t *self, struct sockaddr *raddr, uint64_t connection_id, const char *dns_req, size_t dns_req_len, char *dns_resp, size_t dns_resp_len); // Release any transport-level capacity reserved for this request. The // proxy core calls this exactly once whether the request succeeds or fails. - void (*request_complete)(dns_listener_t *self, struct sockaddr *raddr); + void (*request_complete)(dns_listener_t *self, struct sockaddr *raddr, uint64_t connection_id); // Stop accepting new requests. Existing per-client state (TCP) is retained // so any in-flight DoH responses can still be delivered during graceful // drain. diff --git a/src/dns_listener_tcp.c b/src/dns_listener_tcp.c index d11c09a..d6a3785 100644 --- a/src/dns_listener_tcp.c +++ b/src/dns_listener_tcp.c @@ -31,14 +31,23 @@ enum { LISTEN_BACKLOG = 5, IDLE_TIMEOUT_S = 120, // "two minutes" according to RFC1035 4.2.2 - RESPONSE_SEND_ATTEMPTS = 50, // 0.025 sec max wait - RESPONSE_SEND_DELAY_US = 500, // 0.0005 sec - TCP_DNS_MAX_PAYLOAD = UINT16_MAX - sizeof(uint16_t), // Max after 2-byte length prefix + SHUTDOWN_TIMEOUT_S = 15, + TCP_DNS_MAX_PAYLOAD = UINT16_MAX, + TCP_DNS_MAX_FRAME = TCP_DNS_MAX_PAYLOAD + sizeof(uint16_t), TCP_CLIENT_REQUEST_LIMIT = 16, + TCP_CLIENT_OUTPUT_LIMIT = 2 * UINT16_MAX, + TCP_WRITE_BUDGET = UINT16_MAX, }; typedef struct dns_listener_tcp_s dns_listener_tcp_t; +struct tcp_response_s { + struct tcp_response_s *next; + size_t length; + size_t sent; + char data[]; +}; + struct tcp_client_s { dns_listener_tcp_t * d; @@ -52,10 +61,16 @@ struct tcp_client_s { uint32_t input_buffer_size; uint32_t input_buffer_used; uint16_t pending_requests; + int read_eof; ev_io read_watcher; + ev_io write_watcher; ev_timer timer_watcher; + struct tcp_response_s *output_head; + struct tcp_response_s *output_tail; + size_t output_bytes; + struct tcp_client_s * next; }; @@ -74,33 +89,47 @@ struct dns_listener_tcp_s { uint64_t client_id; uint16_t client_count; uint16_t client_limit; + int stopping; struct tcp_client_s * clients; }; static struct tcp_client_s *find_client(dns_listener_tcp_t *d, - struct sockaddr *raddr) { + struct sockaddr *raddr, uint64_t connection_id) { for (struct tcp_client_s *client = d->clients; client != NULL; client = client->next) { - if (memcmp(raddr, &client->raddr, client->addr_len) == 0) { + if (client->id == connection_id && + memcmp(raddr, &client->raddr, client->addr_len) == 0) { return client; } } return NULL; } +static void free_output_queue(struct tcp_client_s *client) { + while (client->output_head != NULL) { + struct tcp_response_s *response = client->output_head; + client->output_head = response->next; + free(response); + } + client->output_tail = NULL; + client->output_bytes = 0; +} + static void remove_client(struct tcp_client_s * client) { dns_listener_tcp_t *d = client->d; DLOG_CLIENT("Removing client, socket %d", client->sock); - if (d->client_count == d->client_limit) { + if (!d->stopping && d->client_count == d->client_limit) { ev_io_start(d->loop, &d->accept_watcher); // continue accepting new client connections } d->client_count--; ev_io_stop(d->loop, &client->read_watcher); + ev_io_stop(d->loop, &client->write_watcher); ev_timer_stop(d->loop, &client->timer_watcher); free(client->input_buffer); + free_output_queue(client); close(client->sock); @@ -151,19 +180,61 @@ static int get_dns_request(struct tcp_client_s *client, return 1; } +static int dispatch_client_requests(struct tcp_client_s *client) { + dns_listener_tcp_t *d = client->d; + char *dns_req = NULL; + uint16_t req_size = 0; + int request_received = 0; + while (client->pending_requests < TCP_CLIENT_REQUEST_LIMIT && + get_dns_request(client, &dns_req, &req_size)) { + if (req_size < DNS_HEADER_LENGTH) { + WLOG_CLIENT("Malformed request received, too short: %u, dropping client", req_size); + free(dns_req); + remove_client(client); + return -1; + } + client->pending_requests++; + if (client->pending_requests == TCP_CLIENT_REQUEST_LIMIT) { + ev_io_stop(d->loop, &client->read_watcher); + } + d->cb(d->cb_data, &d->base, (struct sockaddr *)&client->raddr, + client->id, dns_req, req_size); + request_received = 1; + } + if (request_received) { + ev_timer_again(d->loop, &client->timer_watcher); + } + return client->pending_requests == TCP_CLIENT_REQUEST_LIMIT; +} + static void read_cb(struct ev_loop __attribute__((unused)) *loop, ev_io *w, int __attribute__((unused)) revents) { struct tcp_client_s *client = (struct tcp_client_s *)w->data; - dns_listener_tcp_t *d = client->d; + if (client->read_eof) { + return; + } + if (dispatch_client_requests(client) != 0) { + return; + } // Receive data char buf[DNS_REQUEST_BUFFER_SIZE]; // if there would be more data, callback will be called again - ssize_t len = recv(w->fd, buf, DNS_REQUEST_BUFFER_SIZE, 0); - if (len == 0) { - DLOG_CLIENT("TCP client closed connection"); + const size_t remaining = TCP_DNS_MAX_FRAME - client->input_buffer_used; + if (remaining == 0) { + WLOG_CLIENT("Request too large, dropping client"); remove_client(client); return; } + const size_t read_size = remaining < sizeof(buf) ? remaining : sizeof(buf); + ssize_t len = recv(w->fd, buf, read_size, 0); + if (len == 0) { + client->read_eof = 1; + ev_io_stop(client->d->loop, &client->read_watcher); + if (client->pending_requests == 0 && client->output_head == NULL) { + remove_client(client); + } + return; + } if (len < 0) { if (errno == ECONNRESET) { DLOG_CLIENT("TCP client closed connection"); @@ -181,7 +252,7 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, if (len > 0) { // Append data into input buffer // Check for integer overflow and maximum message size - if (len > UINT16_MAX || client->input_buffer_used > UINT16_MAX - (uint32_t)len) { + if ((size_t)len > TCP_DNS_MAX_FRAME - client->input_buffer_used) { WLOG_CLIENT("Request too large, dropping client"); remove_client(client); return; @@ -189,7 +260,7 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, const uint32_t free_space = client->input_buffer_size - client->input_buffer_used; const uint32_t needed_space = client->input_buffer_used + (uint32_t)len; // Limit buffer size to prevent memory exhaustion attacks - if (needed_space > TCP_DNS_MAX_PAYLOAD) { + if (needed_space > TCP_DNS_MAX_FRAME) { WLOG_CLIENT("Request too large, dropping client"); remove_client(client); return; @@ -199,9 +270,9 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, for (client->input_buffer_size = 64; // lower value does not make much sense client->input_buffer_size < needed_space; client->input_buffer_size *= 2) { - if (client->input_buffer_size > TCP_DNS_MAX_PAYLOAD) { - FLOG_CLIENT("Unrealistic input buffer size: %u", client->input_buffer_size); - } + } + if (client->input_buffer_size > TCP_DNS_MAX_FRAME) { + client->input_buffer_size = TCP_DNS_MAX_FRAME; } DLOG_CLIENT("Resize input buffer to %u", client->input_buffer_size); client->input_buffer = (char *) realloc((void*) client->input_buffer, // NOLINT(bugprone-suspicious-realloc-usage) if realloc fails, program stops @@ -214,36 +285,13 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, client->input_buffer_used = needed_space; } - // Split requests - char *dns_req = NULL; - uint16_t req_size = 0; - uint8_t request_received = 0; - while (client->pending_requests < TCP_CLIENT_REQUEST_LIMIT && - get_dns_request(client, &dns_req, &req_size)) { - if (req_size < DNS_HEADER_LENGTH) { - WLOG_CLIENT("Malformed request received, too short: %u, dropping client", req_size); - free(dns_req); - remove_client(client); - return; - } - - DLOG_CLIENT("Requested %04hX", ntohs(*((uint16_t*)dns_req))); - client->pending_requests++; - if (client->pending_requests == TCP_CLIENT_REQUEST_LIMIT) { - ev_io_stop(d->loop, &client->read_watcher); - } - d->cb(d->cb_data, &d->base, (struct sockaddr*)&client->raddr, dns_req, req_size); - request_received = 1; - } - - if (request_received) { - ev_timer_again(d->loop, &client->timer_watcher); - } + (void)dispatch_client_requests(client); } -static void tcp_request_complete(dns_listener_t *self, struct sockaddr *raddr) { +static void tcp_request_complete(dns_listener_t *self, struct sockaddr *raddr, + uint64_t connection_id) { dns_listener_tcp_t *d = (dns_listener_tcp_t *)self; - struct tcp_client_s *client = find_client(d, raddr); + struct tcp_client_s *client = find_client(d, raddr, connection_id); if (client == NULL) { return; } @@ -254,7 +302,11 @@ static void tcp_request_complete(dns_listener_t *self, struct sockaddr *raddr) { const int resume_reads = client->pending_requests == TCP_CLIENT_REQUEST_LIMIT; client->pending_requests--; - if (resume_reads) { + if (client->read_eof && client->pending_requests == 0 && client->output_head == NULL) { + remove_client(client); + return; + } + if (resume_reads && !client->read_eof) { ev_io_start(d->loop, &client->read_watcher); if (client->input_buffer_used > 0) { ev_feed_event(d->loop, &client->read_watcher, EV_READ); @@ -269,6 +321,62 @@ static void timer_cb(struct ev_loop __attribute__((unused)) *loop, remove_client(client); } +static int flush_client_output(struct tcp_client_s *client, int *made_progress) { + size_t budget = TCP_WRITE_BUDGET; + *made_progress = 0; + + while (client->output_head != NULL && budget > 0) { + struct tcp_response_s *response = client->output_head; + const size_t remaining = response->length - response->sent; + const size_t send_size = remaining < budget ? remaining : budget; + const ssize_t sent = send(client->sock, response->data + response->sent, + send_size, MSG_NOSIGNAL); + if (sent > 0) { + const size_t sent_size = (size_t)sent; + response->sent += sent_size; + client->output_bytes -= sent_size; + budget -= sent_size; + *made_progress = 1; + if (response->sent == response->length) { + client->output_head = response->next; + if (client->output_head == NULL) { + client->output_tail = NULL; + } + free(response); + } + continue; + } + if (sent < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) { + break; + } + WLOG_CLIENT("Send error: %s (%d), dropping client", strerror(errno), errno); + return -1; + } + + if (client->output_head == NULL) { + ev_io_stop(client->d->loop, &client->write_watcher); + } else { + ev_io_start(client->d->loop, &client->write_watcher); + } + return 0; +} + +static void write_cb(struct ev_loop __attribute__((unused)) *loop, + ev_io *w, int __attribute__((unused)) revents) { + struct tcp_client_s *client = (struct tcp_client_s *)w->data; + int made_progress = 0; + if (flush_client_output(client, &made_progress) != 0) { + remove_client(client); + return; + } + if (made_progress && !client->d->stopping) { + ev_timer_again(client->d->loop, &client->timer_watcher); + } + if (client->read_eof && client->pending_requests == 0 && client->output_head == NULL) { + remove_client(client); + } +} + static void accept_cb(struct ev_loop __attribute__((unused)) *loop, ev_io *w, int __attribute__((unused)) revents) { dns_listener_tcp_t *d = (dns_listener_tcp_t *)w->data; @@ -323,6 +431,9 @@ static void accept_cb(struct ev_loop __attribute__((unused)) *loop, client->read_watcher.data = client; ev_io_start(d->loop, &client->read_watcher); + ev_io_init(&client->write_watcher, write_cb, client->sock, EV_WRITE); + client->write_watcher.data = client; + ev_init(&client->timer_watcher, timer_cb); client->timer_watcher.repeat = IDLE_TIMEOUT_S; client->timer_watcher.data = client; @@ -382,13 +493,12 @@ static int get_tcp_listen_sock(struct addrinfo *listen_addrinfo) { } static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr, + uint64_t connection_id, const char __attribute__((unused)) *dns_req, size_t __attribute__((unused)) dns_req_len, char *resp, size_t resp_len) { dns_listener_tcp_t *d = (dns_listener_tcp_t *)self; - // Limit response size to prevent overflow when accounting for the 2-byte - // length prefix. The total on-wire size would be resp_len + sizeof(uint16_t). if (resp_len < DNS_HEADER_LENGTH || resp_len > TCP_DNS_MAX_PAYLOAD) { WLOG("Malformed response received, invalid length: %zu", resp_len); return; @@ -396,65 +506,73 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr, const uint16_t response_id = ntohs(*((uint16_t*)resp)); // find client data - struct tcp_client_s *client = find_client(d, raddr); + struct tcp_client_s *client = find_client(d, raddr, connection_id); if (client == NULL) { WLOG("Could not find client, can not send DNS response: %04hX", response_id); return; } - // NOTE: Single-threaded libev event loop ensures no TOCTOU race here. - // No other callbacks can execute while this function runs, and usleep() - // below is a blocking syscall (not an event loop yield). If remove_client() - // is called due to send errors, the function returns immediately. - - DLOG_CLIENT("Sending %zu bytes", resp_len); - - // send length of response - uint16_t resp_size = htons((uint16_t)resp_len); - ssize_t len = send(client->sock, &resp_size, sizeof(uint16_t), MSG_MORE | MSG_NOSIGNAL); - if (len != sizeof(uint16_t)) { - WLOG_CLIENT("Send error: %s (%d), len: %d, dropping client", strerror(errno), errno, len); + const size_t frame_len = sizeof(uint16_t) + resp_len; + if (frame_len > TCP_CLIENT_OUTPUT_LIMIT - client->output_bytes) { + WLOG_CLIENT("Output queue limit exceeded, dropping client"); remove_client(client); return; } - // send the response - ssize_t sent = 0; - int attempts = 0; - for (; attempts < RESPONSE_SEND_ATTEMPTS; ++attempts) - { - len = send(client->sock, resp + sent, resp_len - (size_t)sent, MSG_NOSIGNAL); - if (len > 0) { - sent += len; - if (sent == (ssize_t)resp_len) { - break; - } - } else if (len < 0) { - if (errno != EAGAIN && errno != EWOULDBLOCK) { - WLOG_CLIENT("Send error: %s (%d), dropping client", strerror(errno), errno); - remove_client(client); - return; - } - } - usleep(RESPONSE_SEND_DELAY_US); + struct tcp_response_s *response = + (struct tcp_response_s *)malloc(sizeof(*response) + frame_len); + if (response == NULL) { + FLOG_CLIENT("Out of mem"); } - if (sent != (ssize_t)resp_len) { - WLOG_CLIENT("Send timeout after %d attempts, sent %zd/%zu bytes, dropping client", - attempts, sent, resp_len); + response->next = NULL; + response->length = frame_len; + response->sent = 0; + const uint16_t response_size = htons((uint16_t)resp_len); + memcpy(response->data, &response_size, sizeof(response_size)); + memcpy(response->data + sizeof(response_size), resp, resp_len); + + if (client->output_tail != NULL) { + client->output_tail->next = response; + } else { + client->output_head = response; + } + client->output_tail = response; + client->output_bytes += frame_len; + + DLOG_CLIENT("Queued %zu bytes, %zu bytes pending", resp_len, client->output_bytes); + int made_progress = 0; + if (flush_client_output(client, &made_progress) != 0) { remove_client(client); return; } - DLOG_CLIENT("Responded %04hX", response_id); - - ev_timer_again(d->loop, &client->timer_watcher); + if (made_progress) { + DLOG_CLIENT("Responded %04hX", response_id); + if (!d->stopping) { + ev_timer_again(d->loop, &client->timer_watcher); + } + } } static void tcp_stop(dns_listener_t *self) { dns_listener_tcp_t *d = (dns_listener_tcp_t *)self; - while (d->clients) { - remove_client(d->clients); //NOLINT(clang-analyzer-unix.Malloc) false use after free detection + if (d->stopping) { + return; } + d->stopping = 1; ev_io_stop(d->loop, &d->accept_watcher); + for (struct tcp_client_s *client = d->clients; client != NULL;) { + struct tcp_client_s *next = client->next; + client->read_eof = 1; + ev_io_stop(d->loop, &client->read_watcher); + if (client->pending_requests == 0 && client->output_head == NULL) { + remove_client(client); + } else { + ev_timer_stop(d->loop, &client->timer_watcher); + ev_timer_set(&client->timer_watcher, SHUTDOWN_TIMEOUT_S, 0); + ev_timer_start(d->loop, &client->timer_watcher); + } + client = next; + } } static void tcp_destroy(dns_listener_t *self) { diff --git a/src/dns_listener_udp.c b/src/dns_listener_udp.c index e2e09ba..49dad2e 100644 --- a/src/dns_listener_udp.c +++ b/src/dns_listener_udp.c @@ -82,10 +82,11 @@ static void watcher_cb(struct ev_loop __attribute__((unused)) *loop, } memcpy(dns_req, tmp_buf, (size_t)len); - d->cb(d->cb_data, &d->base, (struct sockaddr*)&tmp_raddr, dns_req, (size_t)len); + d->cb(d->cb_data, &d->base, (struct sockaddr*)&tmp_raddr, 0, dns_req, (size_t)len); } static void udp_respond(dns_listener_t *self, struct sockaddr *raddr, + uint64_t __attribute__((unused)) connection_id, const char *dns_req, size_t dns_req_len, char *dns_resp, size_t dns_resp_len) { dns_listener_udp_t *d = (dns_listener_udp_t *)self; @@ -103,7 +104,8 @@ static void udp_respond(dns_listener_t *self, struct sockaddr *raddr, } static void udp_request_complete(dns_listener_t __attribute__((unused)) *self, - struct sockaddr __attribute__((unused)) *raddr) { + struct sockaddr __attribute__((unused)) *raddr, + uint64_t __attribute__((unused)) connection_id) { } static void udp_stop(dns_listener_t *self) { diff --git a/src/doh_proxy.c b/src/doh_proxy.c index 9d64389..2db67d0 100644 --- a/src/doh_proxy.c +++ b/src/doh_proxy.c @@ -34,6 +34,7 @@ typedef struct { dns_listener_t *listener; ev_tstamp start_tstamp; struct sockaddr_storage raddr; + uint64_t connection_id; char *dns_req; size_t dns_req_len; } doh_request_t; @@ -190,6 +191,7 @@ static void doh_response_cb(void *data, char *buf, size_t buflen) { req_id, resp_id); } else { req->listener->respond(req->listener, (struct sockaddr*)&req->raddr, + req->connection_id, req->dns_req, req->dns_req_len, buf, buflen); if (p->stat) { stat_request_end(p->stat, buflen, @@ -200,13 +202,14 @@ static void doh_response_cb(void *data, char *buf, size_t buflen) { } } - req->listener->request_complete(req->listener, (struct sockaddr*)&req->raddr); + req->listener->request_complete(req->listener, (struct sockaddr*)&req->raddr, + req->connection_id); free((void*)req->dns_req); free(req); } void doh_proxy_handle_request(void *ctx, dns_listener_t *listener, - struct sockaddr *raddr, + struct sockaddr *raddr, uint64_t connection_id, char *dns_req, size_t dns_req_len) { doh_proxy_t *p = (doh_proxy_t *)ctx; @@ -215,7 +218,7 @@ void doh_proxy_handle_request(void *ctx, dns_listener_t *listener, if (p->awaiting_bootstrap) { WLOG("%04hX: Query received before bootstrapping is completed, discarding.", req_id); - listener->request_complete(listener, raddr); + listener->request_complete(listener, raddr, connection_id); free(dns_req); return; } @@ -226,6 +229,7 @@ void doh_proxy_handle_request(void *ctx, dns_listener_t *listener, } req->proxy = p; req->listener = listener; + req->connection_id = connection_id; req->dns_req = dns_req; req->dns_req_len = dns_req_len; // raddr length depends on family; sockaddr_storage holds either. Copy what diff --git a/src/doh_proxy.h b/src/doh_proxy.h index a0dd2f4..cdf96ae 100644 --- a/src/doh_proxy.h +++ b/src/doh_proxy.h @@ -42,7 +42,7 @@ void doh_proxy_set_resolv(doh_proxy_t *p, const char *buf); // dns_request_fn — pass to dns_*_listener_create as the request callback. // `ctx` must be a doh_proxy_t *. void doh_proxy_handle_request(void *ctx, dns_listener_t *listener, - struct sockaddr *raddr, + struct sockaddr *raddr, uint64_t connection_id, char *dns_req, size_t dns_req_len); // dns_poller_cb — pass to dns_poller_init as the resolver-update callback. diff --git a/tests/unit/test_dns_listener_tcp.c b/tests/unit/test_dns_listener_tcp.c index b552689..5cacdff 100644 --- a/tests/unit/test_dns_listener_tcp.c +++ b/tests/unit/test_dns_listener_tcp.c @@ -8,18 +8,57 @@ #include "../../src/dns_listener_tcp.c" static struct tcp_client_s client; +static dns_listener_tcp_t listener; static char *dns_request; +static int sockets[2]; static void setUp(void) { memset(&client, 0, sizeof(client)); + memset(&listener, 0, sizeof(listener)); + sockets[0] = -1; + sockets[1] = -1; client.input_buffer_size = 64; client.input_buffer = (char *)calloc(1, client.input_buffer_size); dns_request = NULL; + + listener.loop = ev_loop_new(0); + listener.clients = &client; + client.d = &listener; + if (listener.loop == NULL || socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) != 0) { + test_fail(__FILE__, __LINE__, "test socket setup"); + return; + } + client.sock = sockets[0]; + client.addr_len = sizeof(struct sockaddr); + client.raddr.ss_family = AF_UNIX; + ev_io_init(&client.read_watcher, read_cb, client.sock, EV_READ); + client.read_watcher.data = &client; + ev_io_init(&client.write_watcher, write_cb, client.sock, EV_WRITE); + client.write_watcher.data = &client; + ev_timer_init(&client.timer_watcher, timer_cb, IDLE_TIMEOUT_S, IDLE_TIMEOUT_S); + client.timer_watcher.data = &client; } static void tearDown(void) { + if (listener.clients != NULL && listener.clients != &client) { + remove_client(listener.clients); + sockets[0] = -1; + } free(dns_request); free(client.input_buffer); + free_output_queue(&client); + if (listener.loop != NULL) { + ev_io_stop(listener.loop, &client.read_watcher); + ev_io_stop(listener.loop, &client.write_watcher); + ev_timer_stop(listener.loop, &client.timer_watcher); + ev_loop_destroy(listener.loop); + } + if (sockets[0] >= 0) { + close(sockets[0]); + } + if (sockets[1] >= 0) { + close(sockets[1]); + } } static void test_waits_for_complete_length_prefix(void) { @@ -82,31 +121,313 @@ static void test_extracts_complete_request(void) { } } -static void test_request_completion_resumes_client_reads(void) { - int sockets[2]; - TEST_ASSERT(socketpair(AF_UNIX, SOCK_STREAM, 0, sockets) == 0); - struct ev_loop *loop = ev_loop_new(0); - TEST_ASSERT(loop != NULL); +static void test_large_partial_request_grows_without_exiting(void) { + client.input_buffer_size = 32768; + client.input_buffer = realloc(client.input_buffer, client.input_buffer_size); + TEST_ASSERT(client.input_buffer != NULL); + client.input_buffer_used = client.input_buffer_size; + const uint16_t length = htons(60000); + memcpy(client.input_buffer, &length, sizeof(length)); + char extra[1024] = {0}; + TEST_ASSERT(send(sockets[1], extra, sizeof(extra), 0) == sizeof(extra)); + read_cb(listener.loop, &client.read_watcher, EV_READ); + TEST_ASSERT(client.input_buffer_size >= client.input_buffer_used); + TEST_ASSERT(client.input_buffer_size <= TCP_DNS_MAX_FRAME); + TEST_ASSERT(client.input_buffer_used == 32768 + sizeof(extra)); +} - dns_listener_tcp_t listener; - memset(&listener, 0, sizeof(listener)); - listener.loop = loop; - listener.clients = &client; - client.d = &listener; - client.sock = sockets[0]; +static void test_request_completion_resumes_client_reads(void) { client.pending_requests = TCP_CLIENT_REQUEST_LIMIT; - ev_io_init(&client.read_watcher, read_cb, client.sock, EV_READ); - client.read_watcher.data = &client; - tcp_request_complete(&listener.base, (struct sockaddr*)&client.raddr); + tcp_request_complete(&listener.base, (struct sockaddr*)&client.raddr, client.id); TEST_ASSERT(client.pending_requests == TCP_CLIENT_REQUEST_LIMIT - 1); TEST_ASSERT(ev_is_active(&client.read_watcher)); +} + +static void consume_request(void *ctx, dns_listener_t *unused_listener, + struct sockaddr *unused_address, + uint64_t unused_id, char *request, size_t unused_length) { + (void)unused_listener; + (void)unused_address; + (void)unused_id; + (void)unused_length; + unsigned *requests = ctx; + (*requests)++; + free(request); +} + +static void test_resume_drains_buffer_before_reading_socket(void) { + char frame[sizeof(uint16_t) + DNS_HEADER_LENGTH] = {0}; + const uint16_t length = htons(DNS_HEADER_LENGTH); + memcpy(frame, &length, sizeof(length)); + memcpy(client.input_buffer, frame, sizeof(frame)); + memcpy(client.input_buffer + sizeof(frame), frame, sizeof(frame)); + client.input_buffer_used = 2 * sizeof(frame); + client.pending_requests = TCP_CLIENT_REQUEST_LIMIT; + unsigned requests = 0; + listener.cb = consume_request; + listener.cb_data = &requests; + TEST_ASSERT(send(sockets[1], frame, sizeof(frame), 0) == sizeof(frame)); + + tcp_request_complete(&listener.base, (struct sockaddr *)&client.raddr, client.id); + read_cb(listener.loop, &client.read_watcher, EV_READ); + + TEST_ASSERT(requests == 1); + TEST_ASSERT(client.pending_requests == TCP_CLIENT_REQUEST_LIMIT); + TEST_ASSERT(client.input_buffer_used == sizeof(frame)); + TEST_ASSERT(!ev_is_active(&client.read_watcher)); + char unread[sizeof(frame)]; + TEST_ASSERT(recv(sockets[0], unread, sizeof(unread), MSG_DONTWAIT) == sizeof(frame)); + TEST_ASSERT(memcmp(unread, frame, sizeof(frame)) == 0); +} + +static void test_read_respects_remaining_input_capacity(void) { + client.input_buffer_size = TCP_DNS_MAX_FRAME; + client.input_buffer = realloc(client.input_buffer, client.input_buffer_size); + TEST_ASSERT(client.input_buffer != NULL); + client.input_buffer_used = TCP_DNS_MAX_FRAME - 8; + const uint16_t length = htons(UINT16_MAX); + memcpy(client.input_buffer, &length, sizeof(length)); + unsigned requests = 0; + listener.cb = consume_request; + listener.cb_data = &requests; + char extra[16] = {0}; + TEST_ASSERT(send(sockets[1], extra, sizeof(extra), 0) == sizeof(extra)); + read_cb(listener.loop, &client.read_watcher, EV_READ); + TEST_ASSERT(requests == 1); + TEST_ASSERT(client.input_buffer_used == 0); + TEST_ASSERT(recv(sockets[0], extra, sizeof(extra), MSG_DONTWAIT) == 8); +} + +static void test_old_connection_cannot_complete_or_respond_to_new_client(void) { + client.id = 2; + client.pending_requests = TCP_CLIENT_REQUEST_LIMIT; + tcp_request_complete(&listener.base, (struct sockaddr *)&client.raddr, 1); + TEST_ASSERT(client.pending_requests == TCP_CLIENT_REQUEST_LIMIT); + TEST_ASSERT(!ev_is_active(&client.read_watcher)); + + char response[DNS_HEADER_LENGTH] = {0}; + tcp_respond(&listener.base, (struct sockaddr *)&client.raddr, + 1, NULL, 0, response, sizeof(response)); + TEST_ASSERT(client.output_head == NULL); + TEST_ASSERT(client.output_bytes == 0); + char received[DNS_HEADER_LENGTH]; + TEST_ASSERT(recv(sockets[1], received, sizeof(received), MSG_DONTWAIT) == -1); + TEST_ASSERT(errno == EAGAIN || errno == EWOULDBLOCK); +} - ev_io_stop(loop, &client.read_watcher); - ev_loop_destroy(loop); - close(sockets[0]); - close(sockets[1]); +static void test_queues_response_without_blocking_event_loop(void) { + const int flags = fcntl(sockets[0], F_GETFL, 0); + TEST_ASSERT(flags >= 0); + TEST_ASSERT(fcntl(sockets[0], F_SETFL, flags | O_NONBLOCK) == 0); + + char fill[4096] = {0}; + while (send(sockets[0], fill, sizeof(fill), MSG_NOSIGNAL) > 0) { + } + TEST_ASSERT(errno == EAGAIN || errno == EWOULDBLOCK); + + char response_one[DNS_HEADER_LENGTH]; + char response_two[DNS_HEADER_LENGTH]; + memset(response_one, 0x11, sizeof(response_one)); + memset(response_two, 0x22, sizeof(response_two)); + response_one[0] = 0x12; + response_one[1] = 0x34; + response_two[0] = 0x56; + response_two[1] = 0x78; + tcp_respond(&listener.base, (struct sockaddr *)&client.raddr, + client.id, NULL, 0, response_one, sizeof(response_one)); + tcp_respond(&listener.base, (struct sockaddr *)&client.raddr, + client.id, NULL, 0, response_two, sizeof(response_two)); + + TEST_ASSERT(client.output_head != NULL); + TEST_ASSERT(client.output_bytes == + 2 * (sizeof(uint16_t) + sizeof(response_one))); + TEST_ASSERT(ev_is_active(&client.write_watcher)); + + const int peer_flags = fcntl(sockets[1], F_GETFL, 0); + TEST_ASSERT(peer_flags >= 0); + TEST_ASSERT(fcntl(sockets[1], F_SETFL, peer_flags | O_NONBLOCK) == 0); + while (recv(sockets[1], fill, sizeof(fill), 0) > 0) { + } + TEST_ASSERT(errno == EAGAIN || errno == EWOULDBLOCK); + + write_cb(listener.loop, &client.write_watcher, EV_WRITE); + + char wire_data[2 * (sizeof(uint16_t) + DNS_HEADER_LENGTH)]; + size_t received = 0; + while (received < sizeof(wire_data)) { + const ssize_t bytes = recv(sockets[1], wire_data + received, + sizeof(wire_data) - received, 0); + TEST_ASSERT(bytes > 0); + received += (size_t)bytes; + } + + uint16_t frame_size = 0; + memcpy(&frame_size, wire_data, sizeof(frame_size)); + TEST_ASSERT(ntohs(frame_size) == sizeof(response_one)); + TEST_ASSERT(memcmp(wire_data + sizeof(frame_size), + response_one, sizeof(response_one)) == 0); + const size_t second_frame = sizeof(frame_size) + sizeof(response_one); + memcpy(&frame_size, wire_data + second_frame, sizeof(frame_size)); + TEST_ASSERT(ntohs(frame_size) == sizeof(response_two)); + TEST_ASSERT(memcmp(wire_data + second_frame + sizeof(frame_size), + response_two, sizeof(response_two)) == 0); + + TEST_ASSERT(client.output_head == NULL); + TEST_ASSERT(client.output_tail == NULL); + TEST_ASSERT(client.output_bytes == 0); + TEST_ASSERT(!ev_is_active(&client.write_watcher)); +} + +static struct tcp_client_s *heap_client(void) { + struct tcp_client_s *owned = malloc(sizeof(*owned)); + if (owned == NULL) { + return NULL; + } + *owned = client; + client.input_buffer = NULL; + listener.clients = owned; + listener.client_count = 1; + listener.client_limit = 2; + ev_io_init(&owned->read_watcher, read_cb, owned->sock, EV_READ); + owned->read_watcher.data = owned; + ev_io_start(listener.loop, &owned->read_watcher); + ev_io_init(&owned->write_watcher, write_cb, owned->sock, EV_WRITE); + owned->write_watcher.data = owned; + ev_timer_init(&owned->timer_watcher, timer_cb, IDLE_TIMEOUT_S, IDLE_TIMEOUT_S); + owned->timer_watcher.data = owned; + return owned; +} + +static void check_queued_response_drains(int stopping) { + struct tcp_client_s *owned = heap_client(); + TEST_ASSERT(owned != NULL); + TEST_ASSERT(fcntl(sockets[0], F_SETFL, O_NONBLOCK) == 0); + TEST_ASSERT(fcntl(sockets[1], F_SETFL, O_NONBLOCK) == 0); + char fill[4096] = {0}; + while (send(sockets[0], fill, sizeof(fill), MSG_NOSIGNAL) > 0) { + } + TEST_ASSERT(errno == EAGAIN || errno == EWOULDBLOCK); + char response[DNS_HEADER_LENGTH] = {0}; + tcp_respond(&listener.base, (struct sockaddr *)&owned->raddr, + owned->id, NULL, 0, response, sizeof(response)); + TEST_ASSERT(owned->output_head != NULL); + if (stopping) { + tcp_stop(&listener.base); + TEST_ASSERT(ev_is_active(&owned->timer_watcher)); + TEST_ASSERT(owned->timer_watcher.repeat == 0); + } else { + TEST_ASSERT(shutdown(sockets[1], SHUT_WR) == 0); + read_cb(listener.loop, &owned->read_watcher, EV_READ); + } + TEST_ASSERT(listener.clients == owned); + TEST_ASSERT(owned->read_eof); + TEST_ASSERT(!ev_is_active(&owned->read_watcher)); + while (recv(sockets[1], fill, sizeof(fill), 0) > 0) { + } + write_cb(listener.loop, &owned->write_watcher, EV_WRITE); + TEST_ASSERT(listener.clients == NULL); + sockets[0] = -1; + char frame[sizeof(uint16_t) + DNS_HEADER_LENGTH]; + TEST_ASSERT(recv(sockets[1], frame, sizeof(frame), 0) == sizeof(frame)); + uint16_t length; + memcpy(&length, frame, sizeof(length)); + TEST_ASSERT(ntohs(length) == sizeof(response)); + TEST_ASSERT(memcmp(frame + sizeof(length), response, sizeof(response)) == 0); + TEST_ASSERT(recv(sockets[1], frame, sizeof(frame), 0) == 0); +} + +static void test_half_close_keeps_queued_response_until_drained(void) { + check_queued_response_drains(0); +} + +static void test_shutdown_keeps_queued_response_until_drained(void) { + check_queued_response_drains(1); +} + +static void test_shutdown_waits_for_pending_response(void) { + struct tcp_client_s *owned = heap_client(); + TEST_ASSERT(owned != NULL); + owned->pending_requests = TCP_CLIENT_REQUEST_LIMIT; + listener.client_limit = 1; + tcp_stop(&listener.base); + TEST_ASSERT(listener.clients == owned); + TEST_ASSERT(!ev_is_active(&owned->read_watcher)); + TEST_ASSERT(!ev_is_active(&listener.accept_watcher)); + const ev_tstamp deadline = owned->timer_watcher.at; + char response[DNS_HEADER_LENGTH] = {0}; + tcp_respond(&listener.base, (struct sockaddr *)&owned->raddr, + owned->id, NULL, 0, response, sizeof(response)); + tcp_request_complete(&listener.base, (struct sockaddr *)&owned->raddr, owned->id); + TEST_ASSERT(!ev_is_active(&owned->read_watcher)); + TEST_ASSERT(ev_is_active(&owned->timer_watcher)); + TEST_ASSERT(owned->timer_watcher.at == deadline); + TEST_ASSERT(owned->timer_watcher.repeat == 0); + tcp_stop(&listener.base); + TEST_ASSERT(owned->timer_watcher.at == deadline); + char frame[sizeof(uint16_t) + DNS_HEADER_LENGTH]; + TEST_ASSERT(recv(sockets[1], frame, sizeof(frame), 0) == sizeof(frame)); + timer_cb(listener.loop, &owned->timer_watcher, EV_TIMER); + sockets[0] = -1; + TEST_ASSERT(listener.clients == NULL); + TEST_ASSERT(!ev_is_active(&listener.accept_watcher)); +} + +static void test_shutdown_removes_idle_client(void) { + TEST_ASSERT(heap_client() != NULL); + listener.client_limit = 1; + tcp_stop(&listener.base); + sockets[0] = -1; + TEST_ASSERT(listener.clients == NULL); + TEST_ASSERT(!ev_is_active(&listener.accept_watcher)); +} + +static void test_half_close_waits_for_pending_request_completion(void) { + struct tcp_client_s *owned = heap_client(); + TEST_ASSERT(owned != NULL); + owned->pending_requests = 1; + TEST_ASSERT(shutdown(sockets[1], SHUT_WR) == 0); + read_cb(listener.loop, &owned->read_watcher, EV_READ); + TEST_ASSERT(listener.clients == owned); + TEST_ASSERT(owned->read_eof); + struct sockaddr_storage address = owned->raddr; + const uint64_t connection_id = owned->id; + tcp_request_complete(&listener.base, (struct sockaddr *)&address, connection_id); + TEST_ASSERT(listener.clients == NULL); + sockets[0] = -1; +} + +static void test_accepts_maximum_payload_request_and_response(void) { + client.input_buffer_size = TCP_DNS_MAX_FRAME; + client.input_buffer = realloc(client.input_buffer, client.input_buffer_size); + TEST_ASSERT(client.input_buffer != NULL); + memset(client.input_buffer, 0, client.input_buffer_size); + const uint16_t length = htons(UINT16_MAX); + memcpy(client.input_buffer, &length, sizeof(length)); + client.input_buffer_used = TCP_DNS_MAX_FRAME; + uint16_t request_size = 0; + TEST_ASSERT(get_dns_request(&client, &dns_request, &request_size) == 1); + TEST_ASSERT(request_size == UINT16_MAX); + TEST_ASSERT(client.input_buffer_used == 0); + TEST_ASSERT(fcntl(sockets[0], F_SETFL, O_NONBLOCK) == 0); + tcp_respond(&listener.base, (struct sockaddr *)&client.raddr, + client.id, NULL, 0, dns_request, request_size); + size_t received = 0; + char buffer[4096]; + while (received < TCP_DNS_MAX_FRAME) { + const size_t remaining = TCP_DNS_MAX_FRAME - received; + const ssize_t count = recv(sockets[1], buffer, + remaining < sizeof(buffer) ? remaining : sizeof(buffer), 0); + TEST_ASSERT(count > 0); + if (received == 0) { + TEST_ASSERT(count >= (ssize_t)sizeof(length)); + TEST_ASSERT(memcmp(buffer, &length, sizeof(length)) == 0); + } + received += (size_t)count; + write_cb(listener.loop, &client.write_watcher, EV_WRITE); + } + TEST_ASSERT(client.output_head == NULL); } int main(void) { @@ -114,6 +435,17 @@ int main(void) { TEST_RUN(test_large_length_does_not_wrap_frame_size); TEST_RUN(test_rejects_zero_length_request_without_allocating); TEST_RUN(test_extracts_complete_request); + TEST_RUN(test_large_partial_request_grows_without_exiting); TEST_RUN(test_request_completion_resumes_client_reads); + TEST_RUN(test_resume_drains_buffer_before_reading_socket); + TEST_RUN(test_read_respects_remaining_input_capacity); + TEST_RUN(test_old_connection_cannot_complete_or_respond_to_new_client); + TEST_RUN(test_queues_response_without_blocking_event_loop); + TEST_RUN(test_half_close_keeps_queued_response_until_drained); + TEST_RUN(test_shutdown_keeps_queued_response_until_drained); + TEST_RUN(test_shutdown_waits_for_pending_response); + TEST_RUN(test_shutdown_removes_idle_client); + TEST_RUN(test_half_close_waits_for_pending_request_completion); + TEST_RUN(test_accepts_maximum_payload_request_and_response); return test_summary(); } diff --git a/tests/unit/test_doh_proxy_resolver.c b/tests/unit/test_doh_proxy_resolver.c index bb4b620..9acb399 100644 --- a/tests/unit/test_doh_proxy_resolver.c +++ b/tests/unit/test_doh_proxy_resolver.c @@ -7,6 +7,12 @@ static https_client_t client; static doh_proxy_t *proxy; static unsigned reset_calls; +static https_response_cb pending_response_cb; +static void *pending_response_data; +static uint64_t response_connection_id; +static uint64_t completion_connection_id; +static unsigned response_calls; +static unsigned completion_calls; void https_client_reset(https_client_t *unused_client) { (void)unused_client; @@ -23,8 +29,8 @@ void https_client_fetch(https_client_t *unused_client, const char *url, (void)postdata_len; (void)resolv; (void)id; - (void)cb; - (void)data; + pending_response_cb = cb; + pending_response_data = data; } void stat_request_begin(stat_t *stat, size_t request_len, uint8_t is_tcp) { @@ -44,6 +50,12 @@ void stat_request_end(stat_t *stat, size_t response_len, static void setUp(void) { memset(&client, 0, sizeof(client)); reset_calls = 0; + pending_response_cb = NULL; + pending_response_data = NULL; + response_connection_id = 0; + completion_connection_id = 0; + response_calls = 0; + completion_calls = 0; proxy = doh_proxy_create(NULL, &client, "https://resolver.test/dns-query", NULL); doh_proxy_set_resolv(proxy, "resolver.test:443:192.0.2.1,192.0.2.2"); } @@ -90,9 +102,59 @@ static void test_exact_address_match_after_prefix_match(void) { "resolver.test:443:192.0.2.10,192.0.2.1") == 0); } +static void record_response(dns_listener_t *unused_listener, + struct sockaddr *unused_address, + uint64_t connection_id, + const char *unused_request, size_t unused_request_len, + char *unused_response, size_t unused_response_len) { + (void)unused_listener; + (void)unused_address; + (void)unused_request; + (void)unused_request_len; + (void)unused_response; + (void)unused_response_len; + response_connection_id = connection_id; + response_calls++; +} + +static void record_completion(dns_listener_t *unused_listener, + struct sockaddr *unused_address, + uint64_t connection_id) { + (void)unused_listener; + (void)unused_address; + completion_connection_id = connection_id; + completion_calls++; +} + +static void test_preserves_connection_identity_through_proxy_callbacks(void) { + dns_listener_t listener = { + .respond = record_response, + .request_complete = record_completion, + .transport = DNS_TRANSPORT_TCP + }; + struct sockaddr_in address = {.sin_family = AF_INET}; + for (unsigned attempt = 0; attempt < 2; attempt++) { + char *request = calloc(1, DNS_HEADER_LENGTH); + TEST_ASSERT(request != NULL); + const uint64_t connection_id = 100 + attempt; + doh_proxy_handle_request(proxy, &listener, (struct sockaddr *)&address, + connection_id, request, DNS_HEADER_LENGTH); + TEST_ASSERT(pending_response_cb != NULL); + TEST_ASSERT(completion_calls == attempt); + char response[DNS_HEADER_LENGTH] = {0}; + pending_response_cb(pending_response_data, attempt == 0 ? response : NULL, + attempt == 0 ? sizeof(response) : 0); + TEST_ASSERT(completion_calls == attempt + 1); + TEST_ASSERT(completion_connection_id == connection_id); + TEST_ASSERT(response_calls == 1); + TEST_ASSERT(response_connection_id == 100); + } +} + int main(void) { TEST_RUN(test_new_address_triggers_resolver_update); TEST_RUN(test_removed_address_triggers_resolver_update); TEST_RUN(test_exact_address_match_after_prefix_match); + TEST_RUN(test_preserves_connection_identity_through_proxy_callbacks); return test_summary(); } From d7540a2917cc3ebe153459e46ad63686ac61c94d Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 00:51:58 +1000 Subject: [PATCH 06/10] doh_proxy: retain resolve list through client reset --- src/doh_proxy.c | 10 ++++++++-- tests/unit/test_doh_proxy_resolver.c | 12 ++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/doh_proxy.c b/src/doh_proxy.c index 2db67d0..4ce084f 100644 --- a/src/doh_proxy.c +++ b/src/doh_proxy.c @@ -156,12 +156,18 @@ void doh_proxy_handle_resolver_update(const char *hostname, void *ctx, } free((void*)addr_list); DLOG("Received new DNS server IP '%s'", buf + ip_start); - curl_slist_free_all(p->resolv); - p->resolv = curl_slist_append(NULL, buf); + struct curl_slist *new_resolv = curl_slist_append(NULL, buf); + if (new_resolv == NULL) { + FLOG("Out of mem updating DNS server IP"); + } // Reset libcurl: in-flight connections were aimed at the old IP, and curl // gets confused if we leave them around with a different CURLOPT_RESOLVE. + // The old list must remain alive until every easy handle referencing it has + // been removed and cleaned up by the reset. https_client_reset(p->client); + curl_slist_free_all(p->resolv); + p->resolv = new_resolv; if (p->awaiting_bootstrap) { p->awaiting_bootstrap = 0; diff --git a/tests/unit/test_doh_proxy_resolver.c b/tests/unit/test_doh_proxy_resolver.c index 9acb399..1383751 100644 --- a/tests/unit/test_doh_proxy_resolver.c +++ b/tests/unit/test_doh_proxy_resolver.c @@ -13,10 +13,17 @@ static uint64_t response_connection_id; static uint64_t completion_connection_id; static unsigned response_calls; static unsigned completion_calls; +static const char *expected_resolver_during_reset; +static int reset_saw_expected_resolver; void https_client_reset(https_client_t *unused_client) { (void)unused_client; reset_calls++; + if (expected_resolver_during_reset != NULL && + proxy != NULL && proxy->resolv != NULL && + strcmp(proxy->resolv->data, expected_resolver_during_reset) == 0) { + reset_saw_expected_resolver = 1; + } } void https_client_fetch(https_client_t *unused_client, const char *url, @@ -56,6 +63,8 @@ static void setUp(void) { completion_connection_id = 0; response_calls = 0; completion_calls = 0; + expected_resolver_during_reset = NULL; + reset_saw_expected_resolver = 0; proxy = doh_proxy_create(NULL, &client, "https://resolver.test/dns-query", NULL); doh_proxy_set_resolv(proxy, "resolver.test:443:192.0.2.1,192.0.2.2"); } @@ -75,9 +84,12 @@ static void update_resolver(const char *addresses) { } static void test_new_address_triggers_resolver_update(void) { + expected_resolver_during_reset = + "resolver.test:443:192.0.2.1,192.0.2.2"; update_resolver("192.0.2.1,192.0.2.2,192.0.2.3"); TEST_ASSERT(reset_calls == 1); + TEST_ASSERT(reset_saw_expected_resolver); TEST_ASSERT(strcmp(proxy->resolv->data, "resolver.test:443:192.0.2.1,192.0.2.2,192.0.2.3") == 0); } From b4dd4d4944f548d1aadc4332276330e7053a0191 Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 27 Sep 2026 12:05:20 +1000 Subject: [PATCH 07/10] https_client: allocate socket watchers on demand --- CMakeLists.txt | 1 - src/https_client.c | 95 ++++++++++++++++++--------- src/https_client.h | 7 +- tests/unit/test_https_client_limits.c | 37 +++++++++++ 4 files changed, 108 insertions(+), 32 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 0046a15..b7d5245 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -249,7 +249,6 @@ if(BUILD_TESTING) add_executable(https_client_limits_test tests/unit/test_https_client_limits.c - src/https_client.c src/logging.c src/ring_buffer.c src/stat.c) diff --git a/src/https_client.c b/src/https_client.c index a73cc74..6e65936 100644 --- a/src/https_client.c +++ b/src/https_client.c @@ -55,6 +55,12 @@ static void https_fetch_ctx_cleanup(https_client_t *client, struct https_fetch_ctx *ctx, int curl_result_code); +static void schedule_client_reset(https_client_t *client, ev_tstamp delay) { + ev_timer_stop(client->loop, &client->reset_timer); + ev_timer_set(&client->reset_timer, delay, 0); + ev_timer_start(client->loop, &client->reset_timer); +} + static size_t write_buffer(void *buf, size_t size, size_t nmemb, void *userp) { GET_PTR(struct https_fetch_ctx, ctx, userp); size_t write_size = size * nmemb; @@ -373,7 +379,7 @@ static int https_fetch_ctx_process_response(https_client_t *client, recoverable = 1; if (!ev_is_active(&client->reset_timer)) { ILOG_REQ("Client reset timer started"); - ev_timer_start(client->loop, &client->reset_timer); + schedule_client_reset(client, (ev_tstamp)client->opt->conn_loss_time); } ILOG_REQ("curl request failed with %d: %s (recoverable, reset timer will recycle connection)", curl_result_code, curl_easy_strerror(curl_result_code)); @@ -585,6 +591,19 @@ static void check_multi_info(https_client_t *c) { } } +static int handle_multi_action_result(https_client_t *client, CURLMcode code) { + if (code == CURLM_OK) { + return 0; + } + if (code == CURLM_ABORTED_BY_CALLBACK) { + WLOG("curl_multi_socket_action aborted by callback; scheduling HTTPS client reset"); + schedule_client_reset(client, 0); + return -1; + } + FLOG("curl_multi_socket_action error %d: %s", code, curl_multi_strerror(code)); + return -1; +} + static void sock_cb(struct ev_loop __attribute__((unused)) *loop, struct ev_io *w, int revents) { GET_PTR(https_client_t, c, w->data); @@ -593,16 +612,10 @@ static void sock_cb(struct ev_loop __attribute__((unused)) *loop, c->curlm, w->fd, (revents & EV_READ ? CURL_CSELECT_IN : 0) | (revents & EV_WRITE ? CURL_CSELECT_OUT : 0), &ignore); - if (code == CURLM_OK) { - check_multi_info(c); - } - else { - FLOG("curl_multi_socket_action error %d: %s", code, curl_multi_strerror(code)); - if (code == CURLM_ABORTED_BY_CALLBACK) { - WLOG("Resetting HTTPS client to recover from faulty state!"); - https_client_reset(c); - } + if (handle_multi_action_result(c, code) != 0) { + return; } + check_multi_info(c); } static void timer_cb(struct ev_loop __attribute__((unused)) *loop, @@ -611,27 +624,41 @@ static void timer_cb(struct ev_loop __attribute__((unused)) *loop, int ignore = 0; CURLMcode code = curl_multi_socket_action(c->curlm, CURL_SOCKET_TIMEOUT, 0, &ignore); - if (code != CURLM_OK) { - ELOG("curl_multi_socket_action error %d: %s", code, curl_multi_strerror(code)); + if (handle_multi_action_result(c, code) != 0) { + return; } check_multi_info(c); } -static struct ev_io * get_io_event(struct ev_io io_events[], curl_socket_t sock) { - for (int i = 0; i < HTTPS_SOCKET_LIMIT; i++) { - if (io_events[i].fd == sock) { - return &io_events[i]; +static struct ev_io * get_io_event(struct https_io_event *io_events, + curl_socket_t sock) { + for (struct https_io_event *event = io_events; + event != NULL; event = event->next) { + if (event->watcher.fd == sock) { + return &event->watcher; } } return NULL; } -static void dump_io_events(struct ev_io io_events[]) { - for (int i = 0; i < HTTPS_SOCKET_LIMIT; i++) { +static void dump_io_events(struct https_io_event *io_events) { + int index = 0; + for (struct https_io_event *event = io_events; + event != NULL; event = event->next) { + const ev_io *watcher = &event->watcher; ILOG("IO event #%d: fd=%d, events=%d/%s%s", - i+1, io_events[i].fd, io_events[i].events, - (io_events[i].events & EV_READ ? "R" : ""), - (io_events[i].events & EV_WRITE ? "W" : "")); + ++index, watcher->fd, watcher->events, + (watcher->events & EV_READ ? "R" : ""), + (watcher->events & EV_WRITE ? "W" : "")); + } +} + +static void free_io_events(https_client_t *client) { + while (client->io_events != NULL) { + struct https_io_event *event = client->io_events; + client->io_events = event->next; + ev_io_stop(client->loop, &event->watcher); + free(event); } } @@ -645,19 +672,26 @@ static int multi_sock_cb(CURL *curl, curl_socket_t sock, int what, struct ev_io *io_event_ptr = get_io_event(c->io_events, sock); if (io_event_ptr) { ev_io_stop(c->loop, io_event_ptr); - io_event_ptr->fd = 0; + io_event_ptr->fd = CURL_SOCKET_BAD; DLOG("Released used io event: %p", io_event_ptr); } if (what == CURL_POLL_REMOVE) { return 0; } // reserve and start new event on unused slot - io_event_ptr = get_io_event(c->io_events, 0); + io_event_ptr = get_io_event(c->io_events, CURL_SOCKET_BAD); if (!io_event_ptr) { - ELOG("curl needed more IO event handler, than the number of maximum sockets: %d", HTTPS_SOCKET_LIMIT); - dump_io_events(c->io_events); - logging_flight_recorder_dump(); - return -1; + struct https_io_event *event = + (struct https_io_event *)calloc(1, sizeof(*event)); + if (event == NULL) { + dump_io_events(c->io_events); + FLOG("Out of mem allocating curl socket watcher"); + } + event->watcher.fd = CURL_SOCKET_BAD; + event->watcher.data = c; + event->next = c->io_events; + c->io_events = event; + io_event_ptr = &event->watcher; } DLOG("Reserved new io event: %p", io_event_ptr); ev_io_init(io_event_ptr, sock_cb, sock, @@ -704,9 +738,6 @@ void https_client_init(https_client_t *c, options_t *opt, c->loop = loop; c->fetches = NULL; c->timer.data = c; - for (int i = 0; i < HTTPS_SOCKET_LIMIT; i++) { - c->io_events[i].data = c; - } c->opt = opt; c->stat = stat; @@ -740,6 +771,7 @@ void https_client_reset(https_client_t *c) { struct curl_slist *header_list = c->header_list; c->header_list = NULL; https_client_cleanup(c); + ev_timer_set(&c->reset_timer, (ev_tstamp)c->opt->conn_loss_time, 0); https_client_multi_init(c, header_list); } @@ -749,5 +781,8 @@ void https_client_cleanup(https_client_t *c) { } curl_slist_free_all(c->header_list); curl_multi_cleanup(c->curlm); + c->curlm = NULL; + ev_timer_stop(c->loop, &c->timer); ev_timer_stop(c->loop, &c->reset_timer); + free_io_events(c); } diff --git a/src/https_client.h b/src/https_client.h index 419bfcb..81d608b 100644 --- a/src/https_client.h +++ b/src/https_client.h @@ -31,6 +31,11 @@ struct https_fetch_ctx { struct https_fetch_ctx *next; }; +struct https_io_event { + ev_io watcher; + struct https_io_event *next; +}; + // Holds state on the whole multiplexed CURL machine. typedef struct { struct ev_loop *loop; @@ -40,7 +45,7 @@ typedef struct { size_t fetch_count; ev_timer timer; - ev_io io_events[HTTPS_SOCKET_LIMIT]; + struct https_io_event *io_events; int connections; options_t *opt; diff --git a/tests/unit/test_https_client_limits.c b/tests/unit/test_https_client_limits.c index fddc01a..36a7d90 100644 --- a/tests/unit/test_https_client_limits.c +++ b/tests/unit/test_https_client_limits.c @@ -6,6 +6,7 @@ #include "dns_common.h" #include "https_client.h" #include "test_harness.h" +#include "../../src/https_client.c" typedef struct { size_t calls; @@ -76,7 +77,43 @@ static void test_rejects_requests_above_fetch_limit(void) { TEST_ASSERT(callback_state.failures == 1); } +static void test_socket_watchers_grow_and_support_fd_zero(void) { + CURL *fake_easy_handle = (CURL *)(uintptr_t)1; + enum { WATCHER_COUNT = 16 }; + + for (curl_socket_t fd = 0; fd < WATCHER_COUNT; fd++) { + TEST_ASSERT(multi_sock_cb(fake_easy_handle, fd, CURL_POLL_IN, + &client, NULL) == 0); + struct ev_io *watcher = get_io_event(client.io_events, fd); + TEST_ASSERT(watcher != NULL); + TEST_ASSERT(watcher->fd == fd); + TEST_ASSERT(ev_is_active(watcher)); + } + + struct ev_io *fd_zero_watcher = get_io_event(client.io_events, 0); + TEST_ASSERT(fd_zero_watcher != NULL); + TEST_ASSERT(multi_sock_cb(fake_easy_handle, 0, CURL_POLL_REMOVE, + &client, NULL) == 0); + TEST_ASSERT(get_io_event(client.io_events, 0) == NULL); + TEST_ASSERT(!ev_is_active(fd_zero_watcher)); + + TEST_ASSERT(multi_sock_cb(fake_easy_handle, WATCHER_COUNT, + CURL_POLL_OUT, &client, NULL) == 0); + TEST_ASSERT(get_io_event(client.io_events, WATCHER_COUNT) == fd_zero_watcher); +} + +static void test_aborted_multi_action_schedules_client_reset(void) { + TEST_ASSERT(!ev_is_active(&client.reset_timer)); + + TEST_ASSERT(handle_multi_action_result( + &client, CURLM_ABORTED_BY_CALLBACK) == -1); + + TEST_ASSERT(ev_is_active(&client.reset_timer)); +} + int main(void) { TEST_RUN(test_rejects_requests_above_fetch_limit); + TEST_RUN(test_socket_watchers_grow_and_support_fd_zero); + TEST_RUN(test_aborted_multi_action_schedules_client_reset); return test_summary(); } From c437451cbe1030cb46e8ebaf1d3fae83d7c98f91 Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 4 Oct 2026 21:24:49 +1100 Subject: [PATCH 08/10] Bind HTTPS and bootstrap DNS sockets to an outbound interface --- .github/workflows/cmake.yml | 3 + CMakeLists.txt | 32 ++++++++ README.md | 25 ++++++ src/dns_poller.c | 28 +++++++ src/dns_poller.h | 8 +- src/https_client.c | 19 ++++- src/main.c | 28 +++++++ src/options.c | 45 +++++++++- src/options.h | 5 +- src/outbound_interface.c | 41 ++++++++++ src/outbound_interface.h | 7 ++ tests/linux/test_outbound_interface_linux.c | 27 ++++++ tests/unit/test_dns_poller.c | 9 +- tests/unit/test_https_client_limits.c | 13 +++ tests/unit/test_options.c | 87 ++++++++++++++++++++ tests/unit/test_outbound_interface.c | 91 +++++++++++++++++++++ 16 files changed, 462 insertions(+), 6 deletions(-) create mode 100644 src/outbound_interface.c create mode 100644 src/outbound_interface.h create mode 100644 tests/linux/test_outbound_interface_linux.c create mode 100644 tests/unit/test_options.c create mode 100644 tests/unit/test_outbound_interface.c diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index d218f15..d5c4386 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -44,6 +44,9 @@ jobs: - name: Test run: make -C ${{github.workspace}}/ test ARGS="--verbose" + - name: Test Linux interface binding + run: sudo env HDP_RUN_PRIVILEGED_TEST=1 ctest --test-dir "${{github.workspace}}" -R '^outbound_interface_linux$' --output-on-failure --no-tests=error + - uses: actions/upload-artifact@v7 if: ${{ success() || failure() }} with: diff --git a/CMakeLists.txt b/CMakeLists.txt index b7d5245..b3385ee 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -205,6 +205,7 @@ if(BUILD_TESTING) add_executable(dns_poller_watchers_test tests/unit/test_dns_poller.c src/logging.c + src/outbound_interface.c src/ring_buffer.c) set_property(SOURCE tests/unit/test_dns_poller.c APPEND PROPERTY COMPILE_DEFINITIONS __FILENAME__="dns_poller_test") @@ -250,6 +251,7 @@ if(BUILD_TESTING) add_executable(https_client_limits_test tests/unit/test_https_client_limits.c src/logging.c + src/outbound_interface.c src/ring_buffer.c src/stat.c) set_property(SOURCE tests/unit/test_https_client_limits.c APPEND @@ -270,6 +272,36 @@ if(BUILD_TESTING) add_test(NAME doh_proxy_resolver COMMAND doh_proxy_resolver_test) set_tests_properties(doh_proxy_resolver PROPERTIES LABELS unit) + add_executable(outbound_interface_test + tests/unit/test_outbound_interface.c) + set_property(SOURCE tests/unit/test_outbound_interface.c APPEND + PROPERTY COMPILE_DEFINITIONS __FILENAME__="outbound_interface_test") + set_property(TARGET outbound_interface_test PROPERTY C_STANDARD 11) + add_test(NAME outbound_interface COMMAND outbound_interface_test) + set_tests_properties(outbound_interface PROPERTIES LABELS unit) + + add_executable(options_test + tests/unit/test_options.c) + set_property(SOURCE tests/unit/test_options.c APPEND + PROPERTY COMPILE_DEFINITIONS __FILENAME__="options_test") + set_property(TARGET options_test PROPERTY C_STANDARD 11) + add_test(NAME options COMMAND options_test) + set_tests_properties(options PROPERTIES LABELS unit) + + if(CMAKE_SYSTEM_NAME STREQUAL "Linux") + add_executable(outbound_interface_linux_test + tests/linux/test_outbound_interface_linux.c + src/outbound_interface.c) + target_include_directories(outbound_interface_linux_test PRIVATE tests/unit) + set_property(SOURCE tests/linux/test_outbound_interface_linux.c APPEND + PROPERTY COMPILE_DEFINITIONS __FILENAME__="outbound_interface_linux_test") + set_property(TARGET outbound_interface_linux_test PROPERTY C_STANDARD 11) + add_test(NAME outbound_interface_linux COMMAND outbound_interface_linux_test) + set_tests_properties(outbound_interface_linux PROPERTIES + SKIP_RETURN_CODE 77 + LABELS linux-privileged) + endif() + find_program(VALGRIND_EXE NAMES valgrind) if(VALGRIND_EXE) add_test(NAME dns_truncate_valgrind diff --git a/README.md b/README.md index 0c2b9e0..312a45c 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,7 @@ Just run it as a daemon and point traffic at it. Commandline flags are: Usage: ./https_dns_proxy [-a ] [-p ] [-T ] [-b ] [-i ] [-4] [-r ] [-t ] [-S ] [-x] [-q] [-C ] [-c ] + [--outbound-interface ] [-d] [-u ] [-g ] [-v]+ [-l ] [-s ] [-F ] [-V] [-h] @@ -189,6 +190,9 @@ Usage: ./https_dns_proxy [-a ] [-p ] [-T ] [-p ] [-T +#include #include #include #include "dns_poller.h" #include "logging.h" +#include "outbound_interface.h" static void sock_cb(struct ev_loop __attribute__((unused)) *loop, ev_io *w, int revents) { @@ -166,6 +168,19 @@ static void set_bootstrap_source_addr(ares_channel channel, } } +static int configure_bootstrap_socket(ares_socket_t socket_fd, + int __attribute__((unused)) type, + void *userdata) { + dns_poller_t *d = (dns_poller_t *)userdata; + if (outbound_interface_bind_socket(socket_fd, d->outbound_interface) != 0) { + const int bind_errno = errno; + ELOG("Failed to bind bootstrap DNS socket to interface '%s': %s (%d)", + d->outbound_interface, strerror(bind_errno), bind_errno); + return -1; + } + return ARES_SUCCESS; +} + static ev_tstamp get_timeout(dns_poller_t *d) { static struct timeval max_tv = {.tv_sec = 5, .tv_usec = 0}; @@ -219,6 +234,7 @@ void dns_poller_init(dns_poller_t *d, struct ev_loop *loop, const char *bootstrap_dns, int bootstrap_dns_polling_interval, const char *source_addr, + const char *outbound_interface, const char *hostname, int family, dns_poller_cb cb, void *cb_data) { d->io_events = NULL; @@ -258,8 +274,19 @@ void dns_poller_init(dns_poller_t *d, struct ev_loop *loop, if (d->hostname == NULL) { FLOG("Out of mem"); } + d->outbound_interface = NULL; + if (outbound_interface != NULL) { + d->outbound_interface = strdup(outbound_interface); + if (d->outbound_interface == NULL) { + FLOG("Out of mem"); + } + } d->family = family; set_bootstrap_source_addr(d->ares, source_addr, family); + if (d->outbound_interface != NULL) { + ares_set_socket_configure_callback(d->ares, configure_bootstrap_socket, + d); + } d->cb = cb; d->polling_interval = bootstrap_dns_polling_interval; d->request_ongoing = 0; @@ -280,4 +307,5 @@ void dns_poller_cleanup(dns_poller_t *d) { free(event); } free(d->hostname); + free(d->outbound_interface); } diff --git a/src/dns_poller.h b/src/dns_poller.h index 2fa1c62..f1d1fa9 100644 --- a/src/dns_poller.h +++ b/src/dns_poller.h @@ -27,6 +27,7 @@ typedef struct { ares_channel ares; struct ev_loop *loop; char *hostname; // owned; strdup'd in init, freed in cleanup + char *outbound_interface; // owned; optional interface used by socket callback int family; // AF_UNSPEC for IPv4 or IPv6, AF_INET for IPv4 only. dns_poller_cb cb; int polling_interval; @@ -41,14 +42,17 @@ typedef struct { // provided ev_loop. `bootstrap_dns` is a comma-separated list of DNS servers to // use for the lookup `hostname` every `interval_seconds`. For each successful // lookup, `cb` will be called with the resolved address. -// `source_addr` optionally binds bootstrap DNS lookups to a specific IP. +// `source_addr` optionally binds bootstrap DNS lookups to a specific IP and +// `outbound_interface` optionally binds them to a Linux network interface. // `family` should be AF_INET for IPv4 or AF_UNSPEC for both IPv4 and IPv6. // -// Note: hostname is copied; the caller's buffer need not outlive this call. +// Note: hostname and outbound_interface are copied; the caller's buffers need +// not outlive this call. void dns_poller_init(dns_poller_t *d, struct ev_loop *loop, const char *bootstrap_dns, int bootstrap_dns_polling_interval, const char *source_addr, + const char *outbound_interface, const char *hostname, int family, dns_poller_cb cb, void *cb_data); diff --git a/src/https_client.c b/src/https_client.c index 6e65936..b4a9c7e 100644 --- a/src/https_client.c +++ b/src/https_client.c @@ -12,6 +12,7 @@ #include "https_client.h" #include "logging.h" #include "options.h" +#include "outbound_interface.h" #include "stat.h" #define DOH_CONTENT_TYPE "application/dns-message" @@ -96,6 +97,15 @@ static curl_socket_t opensocket_callback(void *clientp, curlsocktype purpose, ELOG("Could not open curl socket %d:%s", errno, strerror(errno)); return CURL_SOCKET_BAD; } + if (client->opt->outbound_interface != NULL && + outbound_interface_bind_socket(sock, + client->opt->outbound_interface) != 0) { + const int bind_errno = errno; + ELOG("Failed to bind HTTPS socket to interface '%s': %s (%d)", + client->opt->outbound_interface, strerror(bind_errno), bind_errno); + (void)close(sock); + return CURL_SOCKET_BAD; + } DLOG("curl opened socket: %d", sock); client->connections++; @@ -329,7 +339,14 @@ static void https_fetch_ctx_init(https_client_t *client, } if (client->opt->source_addr) { DLOG_REQ("Using source address: %s", client->opt->source_addr); - ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_INTERFACE, client->opt->source_addr); + if (client->opt->outbound_interface != NULL) { + char source_address[sizeof("host!") + INET6_ADDRSTRLEN]; + (void)snprintf(source_address, sizeof(source_address), "host!%s", + client->opt->source_addr); + ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_INTERFACE, source_address); + } else { + ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_INTERFACE, client->opt->source_addr); + } } if (client->opt->ca_info) { ASSERT_CURL_EASY_SETOPT(ctx, CURLOPT_CAINFO, client->opt->ca_info); diff --git a/src/main.c b/src/main.c index 95aafd7..ccbbfc7 100644 --- a/src/main.c +++ b/src/main.c @@ -22,6 +22,7 @@ #include "https_client.h" #include "logging.h" #include "options.h" +#include "outbound_interface.h" #include "stat.h" static int is_ipv4_address(const char *str) { @@ -161,6 +162,30 @@ static const char * sw_version(void) { #endif } +static void validate_outbound_settings(const options_t *opt) { + if (opt->outbound_interface == NULL) { + return; + } + if (outbound_interface_validate(opt->outbound_interface) != 0) { + const int bind_errno = errno; + switch (bind_errno) { + case ENODEV: + FLOG("Outbound interface '%s' does not exist", opt->outbound_interface); + break; + case EPERM: + case EACCES: + FLOG("Permission denied binding to outbound interface '%s'; " + "start as the final service user with CAP_NET_RAW", + opt->outbound_interface); + break; + default: + FLOG("Cannot bind outbound sockets to interface '%s': %s (%d)", + opt->outbound_interface, strerror(bind_errno), bind_errno); + break; + } + } +} + int main(int argc, char *argv[]) { struct Options opt; options_init(&opt); @@ -278,6 +303,8 @@ int main(int argc, char *argv[]) { FLOG("Failed to set uid"); } + validate_outbound_settings(&opt); + if (opt.daemonize) { // daemon() is non-standard. If needed, see OpenSSH openbsd-compat/daemon.c if (daemon(0, 0) == -1) { @@ -316,6 +343,7 @@ int main(int argc, char *argv[]) { doh_proxy_await_bootstrap(proxy, systemd_notify_ready); dns_poller_init(dns_poller, loop, opt.bootstrap_dns, opt.bootstrap_dns_polling_interval, opt.source_addr, + opt.outbound_interface, hostname, opt.ipv4 ? AF_INET : AF_UNSPEC, doh_proxy_handle_resolver_update, proxy); ILOG("DNS polling initialized for '%s'", hostname); diff --git a/src/options.c b/src/options.c index 29b557a..3e7fd30 100644 --- a/src/options.c +++ b/src/options.c @@ -1,5 +1,8 @@ +#include #include +#include #include +#include #include #include #include @@ -41,6 +44,7 @@ void options_init(struct Options *opt) { opt->resolver_ip = NULL; opt->curl_proxy = NULL; opt->source_addr = NULL; + opt->outbound_interface = NULL; opt->use_http_version = DEFAULT_HTTP_VERSION; opt->max_idle_time = 118; opt->conn_loss_time = 15; @@ -59,8 +63,14 @@ int parse_int(char * str) { } enum OptionsParseResult options_parse_args(struct Options *opt, int argc, char **argv) { + static const struct option long_options[] = { + {"outbound-interface", required_argument, NULL, 'I'}, + {NULL, 0, NULL, 0} + }; int c = 0; - while ((c = getopt(argc, argv, "a:c:p:T:du:g:b:i:4r:R:e:t:l:vxqm:L:s:S:C:F:hV")) != -1) { + while ((c = getopt_long(argc, argv, + "a:c:p:T:du:g:b:i:4r:R:e:t:l:vxqm:L:s:S:I:C:F:hV", + long_options, NULL)) != -1) { switch (c) { case 'a': // listen_addr opt->listen_addr = optarg; @@ -131,6 +141,9 @@ enum OptionsParseResult options_parse_args(struct Options *opt, int argc, char * case 'S': // source address opt->source_addr = optarg; break; + case 'I': // outbound interface + opt->outbound_interface = optarg; + break; case 'C': // CA info opt->ca_info = optarg; break; @@ -226,6 +239,32 @@ enum OptionsParseResult options_parse_args(struct Options *opt, int argc, char * printf("TCP client limit must be between 0 and %u.\n", MAX_TCP_CLIENTS); return OPR_OPTION_ERROR; } + if (opt->outbound_interface != NULL) { + if (opt->source_addr != NULL) { + struct in_addr address_v4; + struct in6_addr address_v6; + if (inet_pton(AF_INET, opt->source_addr, &address_v4) != 1 && + inet_pton(AF_INET6, opt->source_addr, &address_v6) != 1) { + printf("-S must be an IP literal when using --outbound-interface.\n"); + return OPR_OPTION_ERROR; + } + } +#if !defined(__linux__) && !defined(OUTBOUND_INTERFACE_TEST) + printf("Outbound interface binding is supported only on Linux.\n"); + return OPR_OPTION_ERROR; +#endif + const size_t interface_length = strlen(opt->outbound_interface); + if (interface_length == 0 || interface_length >= IFNAMSIZ) { + printf("Outbound interface name must contain between 1 and %u characters.\n", + (unsigned)(IFNAMSIZ - 1)); + return OPR_OPTION_ERROR; + } + if (opt->user != NULL) { + printf("--outbound-interface cannot be combined with -u. Start directly " + "as the target user with CAP_NET_RAW supplied by the service manager.\n"); + return OPR_OPTION_ERROR; + } + } return OPR_SUCCESS; } @@ -235,6 +274,7 @@ void options_show_usage(int __attribute__((unused)) argc, char **argv) { printf("Usage: %s [-a ] [-p ] [-T ]\n", argv[0]); printf(" [-b ] [-i ] [-4]\n"); printf(" [-r ] [-R ] [-t ] [-S ]\n"); + printf(" [--outbound-interface ]\n"); printf(" [-x] [-q] [-C ] [-c ]\n"); printf(" [-d] [-u ] [-g ] \n"); printf(" [-v]+ [-l ] [-s ] [-F ] [-V] [-h]\n"); @@ -266,6 +306,9 @@ void options_show_usage(int __attribute__((unused)) argc, char **argv) { printf(" bootstrap DNS servers.\n"); printf(" -S source_addr Source IPv4/v6 address for outbound HTTPS and bootstrap DNS.\n"); printf(" (Default: system default)\n"); + printf(" -I, --outbound-interface interface\n"); + printf(" Linux network interface for outbound HTTPS and bootstrap DNS.\n"); + printf(" Incompatible with -u; requires CAP_NET_RAW.\n"); printf(" -x Use HTTP/1.1 instead of HTTP/2. Useful with broken\n" " or limited builds of libcurl.\n"); printf(" -q Use HTTP/3 (QUIC) only.\n"); diff --git a/src/options.h b/src/options.h index 4e2aea4..7b6cf81 100644 --- a/src/options.h +++ b/src/options.h @@ -46,9 +46,12 @@ struct Options { // e.g. "socks5://127.0.0.1:1080" const char *curl_proxy; - // Source address for outbound HTTPS connections + // Source address for outbound HTTPS and bootstrap DNS sockets. const char *source_addr; + // Linux network interface for outbound HTTPS and bootstrap DNS sockets. + const char *outbound_interface; + // 1 = Use only HTTP/1.1 for limited OpenWRT libcurl (which is not built with HTTP/2 support) // 2 = Use only HTTP/2 default // 3 = Use only HTTP/3 QUIC diff --git a/src/outbound_interface.c b/src/outbound_interface.c new file mode 100644 index 0000000..17172da --- /dev/null +++ b/src/outbound_interface.c @@ -0,0 +1,41 @@ +#include +#include +#include +#include +#include + +#include "outbound_interface.h" + +int outbound_interface_bind_socket(int socket_fd, const char *interface_name) { + if (interface_name == NULL || interface_name[0] == '\0') { + errno = EINVAL; + return -1; + } + const size_t interface_length = strlen(interface_name); + if (interface_length >= IFNAMSIZ) { + errno = ENAMETOOLONG; + return -1; + } + +#if defined(__linux__) || defined(OUTBOUND_INTERFACE_TEST) + return setsockopt(socket_fd, SOL_SOCKET, SO_BINDTODEVICE, + interface_name, (socklen_t)(interface_length + 1)); +#else + (void)socket_fd; + errno = ENOTSUP; + return -1; +#endif +} + +int outbound_interface_validate(const char *interface_name) { + int socket_fd = socket(AF_INET, SOCK_DGRAM, 0); + if (socket_fd < 0) { + return -1; + } + + const int result = outbound_interface_bind_socket(socket_fd, interface_name); + const int saved_errno = errno; + (void)close(socket_fd); + errno = saved_errno; + return result; +} diff --git a/src/outbound_interface.h b/src/outbound_interface.h new file mode 100644 index 0000000..ac78068 --- /dev/null +++ b/src/outbound_interface.h @@ -0,0 +1,7 @@ +#ifndef _OUTBOUND_INTERFACE_H_ +#define _OUTBOUND_INTERFACE_H_ + +int outbound_interface_bind_socket(int socket_fd, const char *interface_name); +int outbound_interface_validate(const char *interface_name); + +#endif diff --git a/tests/linux/test_outbound_interface_linux.c b/tests/linux/test_outbound_interface_linux.c new file mode 100644 index 0000000..6532f58 --- /dev/null +++ b/tests/linux/test_outbound_interface_linux.c @@ -0,0 +1,27 @@ +#include +#include +#include + +#include "test_harness.h" +#include "outbound_interface.h" + +static void setUp(void) { +} + +static void tearDown(void) { +} + +static void test_real_interface_binding(void) { + TEST_ASSERT(geteuid() == 0); + TEST_ASSERT(outbound_interface_validate("lo") == 0); + TEST_ASSERT(outbound_interface_validate("hdp-no-such-if") == -1); + TEST_ASSERT(errno == ENODEV); +} + +int main(void) { + if (getenv("HDP_RUN_PRIVILEGED_TEST") == NULL) { + return 77; + } + TEST_RUN(test_real_interface_binding); + return test_summary(); +} diff --git a/tests/unit/test_dns_poller.c b/tests/unit/test_dns_poller.c index 9e9f75c..6dc59db 100644 --- a/tests/unit/test_dns_poller.c +++ b/tests/unit/test_dns_poller.c @@ -25,7 +25,7 @@ static void setUp(void) { test_fail(__FILE__, __LINE__, "ev_loop_new(0)"); return; } - dns_poller_init(&poller, loop, "127.0.0.1,127.0.0.2", 60, NULL, + dns_poller_init(&poller, loop, "127.0.0.1,127.0.0.2", 60, NULL, NULL, "example.com", AF_INET, NULL, NULL); poller_initialized = 1; ev_timer_stop(loop, &poller.timer); @@ -140,11 +140,18 @@ static void test_query_cache_disabled(void) { #endif } +static void test_rejects_socket_when_interface_binding_fails(void) { + poller.outbound_interface = strdup(""); + TEST_ASSERT(poller.outbound_interface != NULL); + TEST_ASSERT(configure_bootstrap_socket(ARES_SOCKET_BAD, 0, &poller) == -1); +} + int main(void) { TEST_RUN(test_allocates_watchers_beyond_nameserver_count); TEST_RUN(test_reuses_released_watcher); TEST_RUN(test_fd_zero_does_not_alias_free_watcher); TEST_RUN(test_cleanup_stops_active_watchers); TEST_RUN(test_query_cache_disabled); + TEST_RUN(test_rejects_socket_when_interface_binding_fails); return test_summary(); } diff --git a/tests/unit/test_https_client_limits.c b/tests/unit/test_https_client_limits.c index 36a7d90..8b41617 100644 --- a/tests/unit/test_https_client_limits.c +++ b/tests/unit/test_https_client_limits.c @@ -111,9 +111,22 @@ static void test_aborted_multi_action_schedules_client_reset(void) { TEST_ASSERT(ev_is_active(&client.reset_timer)); } +static void test_rejects_socket_when_interface_binding_fails(void) { + struct curl_sockaddr address; + memset(&address, 0, sizeof(address)); + address.family = AF_INET; + address.socktype = SOCK_STREAM; + options.outbound_interface = ""; + + TEST_ASSERT(opensocket_callback(&client, CURLSOCKTYPE_IPCXN, &address) == + CURL_SOCKET_BAD); + TEST_ASSERT(client.connections == 0); +} + int main(void) { TEST_RUN(test_rejects_requests_above_fetch_limit); TEST_RUN(test_socket_watchers_grow_and_support_fd_zero); TEST_RUN(test_aborted_multi_action_schedules_client_reset); + TEST_RUN(test_rejects_socket_when_interface_binding_fails); return test_summary(); } diff --git a/tests/unit/test_options.c b/tests/unit/test_options.c new file mode 100644 index 0000000..211eccc --- /dev/null +++ b/tests/unit/test_options.c @@ -0,0 +1,87 @@ +#include +#include + +#include "test_harness.h" + +static uid_t test_getuid(void) { + return 1000; +} + +static struct passwd *test_getpwnam(const char __attribute__((unused)) *name) { + static struct passwd user = { + .pw_name = "proxy", + .pw_uid = 1000, + .pw_gid = 1000 + }; + return &user; +} + +#define getuid test_getuid +#define getpwnam test_getpwnam +#define OUTBOUND_INTERFACE_TEST +#include "../../src/options.c" +#undef OUTBOUND_INTERFACE_TEST +#undef getpwnam +#undef getuid + +static options_t options; + +static void setUp(void) { + options_init(&options); + optind = 1; +#ifdef __APPLE__ + optreset = 1; +#endif +} + +static void tearDown(void) { + options_cleanup(&options); +} + +static void test_parses_outbound_interface(void) { + char *arguments[] = { + "https_dns_proxy", "-S", "192.0.2.10", + "--outbound-interface", "wg0", NULL + }; + + TEST_ASSERT(options_parse_args(&options, 5, arguments) == OPR_SUCCESS); + TEST_ASSERT(options.source_addr != NULL); + TEST_ASSERT(strcmp(options.source_addr, "192.0.2.10") == 0); + TEST_ASSERT(options.outbound_interface != NULL); + TEST_ASSERT(strcmp(options.outbound_interface, "wg0") == 0); +} + +static void test_rejects_interface_with_internal_user_drop(void) { + char *arguments[] = { + "https_dns_proxy", "-u", "proxy", "-I", "wg0", NULL + }; + + TEST_ASSERT(options_parse_args(&options, 5, arguments) == OPR_OPTION_ERROR); +} + +static void test_allows_interface_with_proxy(void) { + char *arguments[] = { + "https_dns_proxy", "-t", "socks5h://proxy.example", "-I", "wg0", NULL + }; + + TEST_ASSERT(options_parse_args(&options, 5, arguments) == OPR_SUCCESS); +} + +static void test_interface_rejects_source_interface_or_hostname(void) { + char *arguments[] = {"https_dns_proxy", "-I", "wg0", "-S", "eth0", NULL}; + TEST_ASSERT(options_parse_args(&options, 5, arguments) == OPR_OPTION_ERROR); +} + +static void test_interface_accepts_ipv6_source(void) { + char *arguments[] = {"https_dns_proxy", "-I", "wg0", "-S", "2001:db8::1", NULL}; + TEST_ASSERT(options_parse_args(&options, 5, arguments) == OPR_SUCCESS); +} + +int main(void) { + TEST_RUN(test_parses_outbound_interface); + TEST_RUN(test_rejects_interface_with_internal_user_drop); + TEST_RUN(test_allows_interface_with_proxy); + TEST_RUN(test_interface_rejects_source_interface_or_hostname); + TEST_RUN(test_interface_accepts_ipv6_source); + return test_summary(); +} diff --git a/tests/unit/test_outbound_interface.c b/tests/unit/test_outbound_interface.c new file mode 100644 index 0000000..e2fa87c --- /dev/null +++ b/tests/unit/test_outbound_interface.c @@ -0,0 +1,91 @@ +#include +#include +#include +#include +#include + +#include "test_harness.h" + +static int setsockopt_calls; +static int setsockopt_result; +static int captured_socket; +static int captured_level; +static int captured_option; +static char captured_interface[IFNAMSIZ]; + +#ifndef SO_BINDTODEVICE +#define SO_BINDTODEVICE 25 +#endif + +static int test_setsockopt(int socket_fd, int level, int option, + const void *value, socklen_t value_length) { + setsockopt_calls++; + captured_socket = socket_fd; + captured_level = level; + captured_option = option; + const size_t copy_length = value_length < sizeof(captured_interface) + ? value_length : sizeof(captured_interface); + memcpy(captured_interface, value, copy_length); + if (setsockopt_result != 0) { + errno = EPERM; + } + return setsockopt_result; +} + +#define setsockopt test_setsockopt +#define OUTBOUND_INTERFACE_TEST +#include "../../src/outbound_interface.c" +#undef OUTBOUND_INTERFACE_TEST +#undef setsockopt + +static void setUp(void) { + setsockopt_calls = 0; + setsockopt_result = 0; + captured_socket = -1; + captured_level = -1; + captured_option = -1; + memset(captured_interface, 0, sizeof(captured_interface)); +} + +static void tearDown(void) { +} + +static void test_rejects_empty_interface(void) { + TEST_ASSERT(outbound_interface_bind_socket(7, "") == -1); + TEST_ASSERT(errno == EINVAL); + TEST_ASSERT(setsockopt_calls == 0); +} + +static void test_rejects_overlong_interface(void) { + char interface_name[IFNAMSIZ + 1]; + memset(interface_name, 'a', sizeof(interface_name)); + interface_name[sizeof(interface_name) - 1] = '\0'; + + TEST_ASSERT(outbound_interface_bind_socket(7, interface_name) == -1); + TEST_ASSERT(errno == ENAMETOOLONG); + TEST_ASSERT(setsockopt_calls == 0); +} + +static void test_binds_socket_to_exact_interface(void) { + TEST_ASSERT(outbound_interface_bind_socket(7, "wg0") == 0); + TEST_ASSERT(setsockopt_calls == 1); + TEST_ASSERT(captured_socket == 7); + TEST_ASSERT(captured_level == SOL_SOCKET); + TEST_ASSERT(captured_option == SO_BINDTODEVICE); + TEST_ASSERT(strcmp(captured_interface, "wg0") == 0); +} + +static void test_propagates_bind_permission_failure(void) { + setsockopt_result = -1; + TEST_ASSERT(outbound_interface_bind_socket(7, "wg0") == -1); + TEST_ASSERT(errno == EPERM); + TEST_ASSERT(setsockopt_calls == 1); +} + +int main(void) { + TEST_RUN(test_rejects_empty_interface); + TEST_RUN(test_rejects_overlong_interface); + TEST_RUN(test_binds_socket_to_exact_interface); + TEST_RUN(test_propagates_bind_permission_failure); + return test_summary(); +} From 055e3a7cea69c1c662737dece7d390b71082594e Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Sun, 4 Oct 2026 21:29:26 +1100 Subject: [PATCH 09/10] options: avoid implicit group reset for non-root startup --- src/options.c | 2 +- tests/unit/test_options.c | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/options.c b/src/options.c index 3e7fd30..b3e2830 100644 --- a/src/options.c +++ b/src/options.c @@ -167,7 +167,7 @@ enum OptionsParseResult options_parse_args(struct Options *opt, int argc, char * return OPR_OPTION_ERROR; } opt->uid = p->pw_uid; - if (!opt->group) { + if (!opt->group && geteuid() == 0) { opt->gid = p->pw_gid; } } diff --git a/tests/unit/test_options.c b/tests/unit/test_options.c index 211eccc..a51cb10 100644 --- a/tests/unit/test_options.c +++ b/tests/unit/test_options.c @@ -3,6 +3,12 @@ #include "test_harness.h" +static uid_t effective_uid; + +static uid_t test_geteuid(void) { + return effective_uid; +} + static uid_t test_getuid(void) { return 1000; } @@ -17,17 +23,20 @@ static struct passwd *test_getpwnam(const char __attribute__((unused)) *name) { } #define getuid test_getuid +#define geteuid test_geteuid #define getpwnam test_getpwnam #define OUTBOUND_INTERFACE_TEST #include "../../src/options.c" #undef OUTBOUND_INTERFACE_TEST #undef getpwnam #undef getuid +#undef geteuid static options_t options; static void setUp(void) { options_init(&options); + effective_uid = 1000; optind = 1; #ifdef __APPLE__ optreset = 1; @@ -77,11 +86,28 @@ static void test_interface_accepts_ipv6_source(void) { TEST_ASSERT(options_parse_args(&options, 5, arguments) == OPR_SUCCESS); } +static void test_nonroot_user_does_not_request_group_reset(void) { + char *arguments[] = {"https_dns_proxy", "-u", "proxy", NULL}; + TEST_ASSERT(options_parse_args(&options, 3, arguments) == OPR_SUCCESS); + TEST_ASSERT(options.uid == 1000); + TEST_ASSERT(options.gid == (gid_t)-1); +} + +static void test_root_user_uses_primary_group(void) { + effective_uid = 0; + char *arguments[] = {"https_dns_proxy", "-u", "proxy", NULL}; + TEST_ASSERT(options_parse_args(&options, 3, arguments) == OPR_SUCCESS); + TEST_ASSERT(options.uid == 1000); + TEST_ASSERT(options.gid == 1000); +} + int main(void) { TEST_RUN(test_parses_outbound_interface); TEST_RUN(test_rejects_interface_with_internal_user_drop); TEST_RUN(test_allows_interface_with_proxy); TEST_RUN(test_interface_rejects_source_interface_or_hostname); TEST_RUN(test_interface_accepts_ipv6_source); + TEST_RUN(test_nonroot_user_does_not_request_group_reset); + TEST_RUN(test_root_user_uses_primary_group); return test_summary(); } From 69a20659db44bee12e841e8196139c5f72c26b02 Mon Sep 17 00:00:00 2001 From: Aaron Drew Date: Mon, 19 Oct 2026 18:21:14 +1100 Subject: [PATCH 10/10] Address review feedback and cap shutdown at five seconds Bound the shutdown grace period so stalled HTTPS requests cannot delay exit. Allow responses to finish during the grace period, then cancel outstanding requests before freeing the listeners and proxy state. Simplify TCP parsing and request completion while preserving safe ownership. Add regression tests for slow clients, callback accounting, and shutdown. --- CMakeLists.txt | 15 ++ src/dns_listener.h | 3 + src/dns_listener_tcp.c | 107 +++++------ src/dns_listener_udp.c | 6 - src/doh_proxy.c | 10 +- src/https_client.c | 20 +- src/main.c | 25 ++- tests/integration/test_tcp_lifecycle.py | 240 ++++++++++++++++++++++++ tests/robot/DnsTcpClient.py | 21 +++ tests/robot/functional_tests.robot | 14 +- tests/unit/test_dns_listener_tcp.c | 77 ++++++++ tests/unit/test_dns_poller.c | 4 +- tests/unit/test_doh_proxy_resolver.c | 29 +++ tests/unit/test_https_client_limits.c | 47 +++++ 14 files changed, 534 insertions(+), 84 deletions(-) create mode 100644 tests/integration/test_tcp_lifecycle.py diff --git a/CMakeLists.txt b/CMakeLists.txt index b3385ee..f1e6bb2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -323,6 +323,21 @@ if(BUILD_TESTING) else() message(STATUS "python3 found: ${PYTHON3_EXE}") + find_program(OPENSSL_EXE NAMES openssl) + if(OPENSSL_EXE) + add_test(NAME tcp_lifecycle + COMMAND ${PYTHON3_EXE} ${CMAKE_SOURCE_DIR}/tests/integration/test_tcp_lifecycle.py + --binary $ --openssl ${OPENSSL_EXE}) + set_tests_properties(tcp_lifecycle PROPERTIES LABELS integration TIMEOUT 90) + if(VALGRIND_EXE) + add_test(NAME tcp_lifecycle_valgrind + COMMAND ${PYTHON3_EXE} ${CMAKE_SOURCE_DIR}/tests/integration/test_tcp_lifecycle.py + --binary $ --openssl ${OPENSSL_EXE} + --valgrind ${VALGRIND_EXE}) + set_tests_properties(tcp_lifecycle_valgrind PROPERTIES LABELS memcheck TIMEOUT 120) + endif() + endif() + # Robot framework tests add_test(NAME robot COMMAND ${PYTHON3_EXE} -m robot.run functional_tests.robot WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/tests/robot) diff --git a/src/dns_listener.h b/src/dns_listener.h index 4aa11ec..bc35c14 100644 --- a/src/dns_listener.h +++ b/src/dns_listener.h @@ -13,6 +13,8 @@ typedef struct dns_listener dns_listener_t; +enum { DNS_SHUTDOWN_TIMEOUT_S = 5 }; + // Transport classification, exposed for callers that need to bin metrics or // log per-transport — not used for dispatch (dispatch is via the function // pointers on dns_listener itself). @@ -37,6 +39,7 @@ struct dns_listener { char *dns_resp, size_t dns_resp_len); // Release any transport-level capacity reserved for this request. The // proxy core calls this exactly once whether the request succeeds or fails. + // May be NULL for transports that do not reserve per-request capacity. void (*request_complete)(dns_listener_t *self, struct sockaddr *raddr, uint64_t connection_id); // Stop accepting new requests. Existing per-client state (TCP) is retained // so any in-flight DoH responses can still be delivered during graceful diff --git a/src/dns_listener_tcp.c b/src/dns_listener_tcp.c index d6a3785..27bec68 100644 --- a/src/dns_listener_tcp.c +++ b/src/dns_listener_tcp.c @@ -31,7 +31,6 @@ enum { LISTEN_BACKLOG = 5, IDLE_TIMEOUT_S = 120, // "two minutes" according to RFC1035 4.2.2 - SHUTDOWN_TIMEOUT_S = 15, TCP_DNS_MAX_PAYLOAD = UINT16_MAX, TCP_DNS_MAX_FRAME = TCP_DNS_MAX_PAYLOAD + sizeof(uint16_t), TCP_CLIENT_REQUEST_LIMIT = 16, @@ -155,25 +154,22 @@ static int get_dns_request(struct tcp_client_s *client, } // check if whole request is available - uint16_t network_req_size = 0; - memcpy(&network_req_size, client->input_buffer, sizeof(network_req_size)); - *req_size = ntohs(network_req_size); + memcpy(req_size, client->input_buffer, sizeof(*req_size)); + *req_size = ntohs(*req_size); const size_t data_size = sizeof(uint16_t) + (size_t)*req_size; if (data_size > client->input_buffer_used) { return 0; // Partial request } - if (*req_size < DNS_HEADER_LENGTH) { - client->input_buffer_used -= (uint32_t)data_size; - memmove(client->input_buffer, client->input_buffer + data_size, client->input_buffer_used); - *dns_req = NULL; - return 1; - } - // copy whole request - *dns_req = (char *)malloc(*req_size); // freed when DoH request completes - if (*dns_req == NULL) { - FLOG_CLIENT("Out of mem"); + // Extract the frame; the caller validates the DNS header. Avoid malloc(0) + // for empty frames, since a NULL result is not an allocation failure. + *dns_req = NULL; + if (*req_size != 0) { + *dns_req = (char *)malloc(*req_size); // freed when DoH request completes + if (*dns_req == NULL) { + FLOG_CLIENT("Out of mem"); + } + memcpy(*dns_req, client->input_buffer + sizeof(uint16_t), *req_size); } - memcpy(*dns_req, client->input_buffer + sizeof(uint16_t), *req_size); // move down data of next request(s) if any client->input_buffer_used -= (uint32_t)data_size; memmove(client->input_buffer, client->input_buffer + data_size, client->input_buffer_used); @@ -226,7 +222,7 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, return; } const size_t read_size = remaining < sizeof(buf) ? remaining : sizeof(buf); - ssize_t len = recv(w->fd, buf, read_size, 0); + const ssize_t len = recv(w->fd, buf, read_size, 0); if (len == 0) { client->read_eof = 1; ev_io_stop(client->d->loop, &client->read_watcher); @@ -239,51 +235,47 @@ static void read_cb(struct ev_loop __attribute__((unused)) *loop, if (errno == ECONNRESET) { DLOG_CLIENT("TCP client closed connection"); } else if (errno == EAGAIN || errno == EWOULDBLOCK) { - len = 0; + return; // Buffered requests were already dispatched before recv(). } else { WLOG_CLIENT("Read error: %s (%d), dropping client", strerror(errno), errno); } - if (len < 0) { - remove_client(client); - return; - } + remove_client(client); + return; } - if (len > 0) { - // Append data into input buffer - // Check for integer overflow and maximum message size - if ((size_t)len > TCP_DNS_MAX_FRAME - client->input_buffer_used) { - WLOG_CLIENT("Request too large, dropping client"); - remove_client(client); - return; + // Append data into input buffer + // Check for integer overflow and maximum message size + if ((size_t)len > TCP_DNS_MAX_FRAME - client->input_buffer_used) { + WLOG_CLIENT("Request too large, dropping client"); + remove_client(client); + return; + } + const uint32_t free_space = client->input_buffer_size - client->input_buffer_used; + const uint32_t needed_space = client->input_buffer_used + (uint32_t)len; + // Limit buffer size to prevent memory exhaustion attacks + if (needed_space > TCP_DNS_MAX_FRAME) { + WLOG_CLIENT("Request too large, dropping client"); + remove_client(client); + return; + } + DLOG_CLIENT("Received %d byte, free: %u", len, free_space); + if (free_space < len) { + for (client->input_buffer_size = 64; // lower value does not make much sense + client->input_buffer_size < needed_space; + client->input_buffer_size *= 2) { } - const uint32_t free_space = client->input_buffer_size - client->input_buffer_used; - const uint32_t needed_space = client->input_buffer_used + (uint32_t)len; - // Limit buffer size to prevent memory exhaustion attacks - if (needed_space > TCP_DNS_MAX_FRAME) { - WLOG_CLIENT("Request too large, dropping client"); - remove_client(client); - return; + if (client->input_buffer_size > TCP_DNS_MAX_FRAME) { + client->input_buffer_size = TCP_DNS_MAX_FRAME; } - DLOG_CLIENT("Received %d byte, free: %u", len, free_space); - if (free_space < len) { - for (client->input_buffer_size = 64; // lower value does not make much sense - client->input_buffer_size < needed_space; - client->input_buffer_size *= 2) { - } - if (client->input_buffer_size > TCP_DNS_MAX_FRAME) { - client->input_buffer_size = TCP_DNS_MAX_FRAME; - } - DLOG_CLIENT("Resize input buffer to %u", client->input_buffer_size); - client->input_buffer = (char *) realloc((void*) client->input_buffer, // NOLINT(bugprone-suspicious-realloc-usage) if realloc fails, program stops - client->input_buffer_size); - if (client->input_buffer == NULL) { - FLOG_CLIENT("Out of mem"); - } + DLOG_CLIENT("Resize input buffer to %u", client->input_buffer_size); + client->input_buffer = (char *) realloc((void*) client->input_buffer, // NOLINT(bugprone-suspicious-realloc-usage) if realloc fails, program stops + client->input_buffer_size); + if (client->input_buffer == NULL) { + FLOG_CLIENT("Out of mem"); } - memcpy(client->input_buffer + client->input_buffer_used, buf, (size_t)len); - client->input_buffer_used = needed_space; } + memcpy(client->input_buffer + client->input_buffer_used, buf, (size_t)len); + client->input_buffer_used = needed_space; (void)dispatch_client_requests(client); } @@ -503,7 +495,9 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr, WLOG("Malformed response received, invalid length: %zu", resp_len); return; } - const uint16_t response_id = ntohs(*((uint16_t*)resp)); + uint16_t response_id; + memcpy(&response_id, resp, sizeof(response_id)); + response_id = ntohs(response_id); // find client data struct tcp_client_s *client = find_client(d, raddr, connection_id); @@ -513,7 +507,7 @@ static void tcp_respond(dns_listener_t *self, struct sockaddr *raddr, } const size_t frame_len = sizeof(uint16_t) + resp_len; - if (frame_len > TCP_CLIENT_OUTPUT_LIMIT - client->output_bytes) { + if (frame_len > (TCP_CLIENT_OUTPUT_LIMIT - client->output_bytes)) { WLOG_CLIENT("Output queue limit exceeded, dropping client"); remove_client(client); return; @@ -568,7 +562,7 @@ static void tcp_stop(dns_listener_t *self) { remove_client(client); } else { ev_timer_stop(d->loop, &client->timer_watcher); - ev_timer_set(&client->timer_watcher, SHUTDOWN_TIMEOUT_S, 0); + ev_timer_set(&client->timer_watcher, DNS_SHUTDOWN_TIMEOUT_S, 0); ev_timer_start(d->loop, &client->timer_watcher); } client = next; @@ -577,6 +571,9 @@ static void tcp_stop(dns_listener_t *self) { static void tcp_destroy(dns_listener_t *self) { dns_listener_tcp_t *d = (dns_listener_tcp_t *)self; + while (d->clients != NULL) { + remove_client(d->clients); + } close(d->sock); free(d); } diff --git a/src/dns_listener_udp.c b/src/dns_listener_udp.c index 49dad2e..26c6efe 100644 --- a/src/dns_listener_udp.c +++ b/src/dns_listener_udp.c @@ -103,11 +103,6 @@ static void udp_respond(dns_listener_t *self, struct sockaddr *raddr, } } -static void udp_request_complete(dns_listener_t __attribute__((unused)) *self, - struct sockaddr __attribute__((unused)) *raddr, - uint64_t __attribute__((unused)) connection_id) { -} - static void udp_stop(dns_listener_t *self) { dns_listener_udp_t *d = (dns_listener_udp_t *)self; ev_io_stop(d->loop, &d->watcher); @@ -127,7 +122,6 @@ dns_listener_t * dns_udp_listener_create(struct ev_loop *loop, FLOG("Out of mem"); } d->base.respond = udp_respond; - d->base.request_complete = udp_request_complete; d->base.stop = udp_stop; d->base.destroy = udp_destroy; d->base.transport = DNS_TRANSPORT_UDP; diff --git a/src/doh_proxy.c b/src/doh_proxy.c index 4ce084f..48db19c 100644 --- a/src/doh_proxy.c +++ b/src/doh_proxy.c @@ -208,8 +208,10 @@ static void doh_response_cb(void *data, char *buf, size_t buflen) { } } - req->listener->request_complete(req->listener, (struct sockaddr*)&req->raddr, - req->connection_id); + if (req->listener->request_complete != NULL) { + req->listener->request_complete(req->listener, (struct sockaddr*)&req->raddr, + req->connection_id); + } free((void*)req->dns_req); free(req); } @@ -224,7 +226,9 @@ void doh_proxy_handle_request(void *ctx, dns_listener_t *listener, if (p->awaiting_bootstrap) { WLOG("%04hX: Query received before bootstrapping is completed, discarding.", req_id); - listener->request_complete(listener, raddr, connection_id); + if (listener->request_complete != NULL) { + listener->request_complete(listener, raddr, connection_id); + } free(dns_req); return; } diff --git a/src/https_client.c b/src/https_client.c index b4a9c7e..cd76043 100644 --- a/src/https_client.c +++ b/src/https_client.c @@ -557,6 +557,17 @@ static void https_fetch_ctx_cleanup(https_client_t *client, if (code != CURLM_OK) { FLOG_REQ("curl_multi_remove_handle error %d: %s", code, curl_multi_strerror(code)); } + // Release the slot before notifying the caller, which may submit another + // request. Detach the context as well so the list and count stay consistent. + if (prev) { + prev->next = ctx->next; + } else { + client->fetches = ctx->next; + } + if (client->fetch_count == 0) { + FLOG_REQ("HTTPS fetch count underflow"); + } + client->fetch_count--; int drop_reply = 0; if (curl_result_code < 0) { WLOG_REQ("Request was aborted"); @@ -574,15 +585,6 @@ static void https_fetch_ctx_cleanup(https_client_t *client, ctx->cb(ctx->cb_data, ctx->buf, ctx->buflen); curl_easy_cleanup(ctx->curl); free(ctx->buf); - if (prev) { - prev->next = ctx->next; - } else { - client->fetches = ctx->next; - } - if (client->fetch_count == 0) { - FLOG_REQ("HTTPS fetch count underflow"); - } - client->fetch_count--; free(ctx); } diff --git a/src/main.c b/src/main.c index ccbbfc7..5b8c155 100644 --- a/src/main.c +++ b/src/main.c @@ -101,6 +101,16 @@ static void sigpipe_cb(struct ev_loop __attribute__((__unused__)) *loop, ELOG("Received SIGPIPE. Ignoring."); } +static void shutdown_timeout_cb(struct ev_loop *loop, + ev_timer __attribute__((__unused__)) *w, + int __attribute__((__unused__)) revents) { + // A one-shot timer is stopped before its callback. Balance the ev_unref() + // used below to allow an otherwise idle shutdown to finish immediately. + ev_ref(loop); + ILOG("Shutdown grace period expired, aborting outstanding requests"); + ev_break(loop, EVBREAK_ALL); +} + static void systemd_notify_ready(void) { #if HAS_LIBSYSTEMD == 1 static uint8_t called_once = 0; @@ -397,17 +407,26 @@ int main(int argc, char *argv[]) { } stat_stop(&stat); + ev_timer shutdown_timer; + ev_timer_init(&shutdown_timer, shutdown_timeout_cb, DNS_SHUTDOWN_TIMEOUT_S, 0); + ev_timer_start(loop, &shutdown_timer); + ev_unref(loop); DLOG("re-entering loop"); ev_run(loop, 0); + if (ev_is_active(&shutdown_timer)) { + ev_ref(loop); + ev_timer_stop(loop, &shutdown_timer); + } DLOG("loop finished all events"); + // Aborting transfers invokes completion callbacks. Keep the listeners and + // proxy alive until those callbacks have released their request state. + // The proxy's CURLOPT_RESOLVE list must also outlive the curl easy handles. + https_client_cleanup(&https_client); udp_listener->destroy(udp_listener); if (tcp_listener != NULL) { tcp_listener->destroy(tcp_listener); } - // The CURLOPT_RESOLVE list owned by the proxy must outlive in-flight curl - // easy handles, which is why https_client_cleanup runs first. - https_client_cleanup(&https_client); doh_proxy_destroy(proxy); stat_cleanup(&stat); diff --git a/tests/integration/test_tcp_lifecycle.py b/tests/integration/test_tcp_lifecycle.py new file mode 100644 index 0000000..5e5845a --- /dev/null +++ b/tests/integration/test_tcp_lifecycle.py @@ -0,0 +1,240 @@ +"""Exercise real TCP clients and shutdown against a local TLS DoH server.""" + +import argparse +import http.server +import os +from pathlib import Path +import signal +import socket +import ssl +import struct +import subprocess +import tempfile +import threading +import time +import unittest + + +def query(identifier): + return struct.pack("!6H", identifier, 0x0100, 1, 0, 0, 0) + ( + b"\x07example\x04test\x00\x00\x01\x00\x01" + ) + + +def answer(request): + # A valid DNS response with EDNS padding, large enough to exercise framing. + question = request[12:] + padding = 4096 - 12 - len(question) - 15 + return ( + struct.pack("!6H", struct.unpack("!H", request[:2])[0], 0x8180, 1, 0, 0, 1) + + question + + b"\x00" + + struct.pack("!HHIH", 41, 4096, 0, padding + 4) + + struct.pack("!HH", 12, padding) + + bytes(padding) + ) + + +def receive_exact(client, length): + data = bytearray() + while len(data) < length: + part = client.recv(length - len(data)) + if not part: + raise AssertionError("TCP stream ended in the middle of a frame") + data.extend(part) + return bytes(data) + + +def receive_frame(client): + length = struct.unpack("!H", receive_exact(client, 2))[0] + return receive_exact(client, length) + + +class LocalDohServer(http.server.ThreadingHTTPServer): + daemon_threads = True + + def __init__(self, context): + super().__init__(("127.0.0.1", 0), DohHandler) + self.socket = context.wrap_socket(self.socket, server_side=True) + self.received = threading.Event() + self.release = threading.Event() + self.release.set() + + +class DohHandler(http.server.BaseHTTPRequestHandler): + protocol_version = "HTTP/1.1" + + def do_POST(self): + request = self.rfile.read(int(self.headers["Content-Length"])) + self.server.received.set() + if not self.server.release.wait(15): + return + response = answer(request) + try: + self.send_response(200) + self.send_header("Content-Type", "application/dns-message") + self.send_header("Content-Length", str(len(response))) + self.end_headers() + self.wfile.write(response) + except (BrokenPipeError, ConnectionResetError, ssl.SSLError): + pass # The shutdown-deadline tests deliberately cancel the transfer. + + def log_message(self, *args): + pass + + +class TcpLifecycleTests(unittest.TestCase): + @classmethod + def setUpClass(cls): + cls.files = tempfile.TemporaryDirectory(prefix="hdp-tcp-lifecycle-") + cls.root = Path(cls.files.name) + cls.cert = cls.root / "cert.pem" + key = cls.root / "key.pem" + subprocess.run([ + OPTIONS.openssl, "req", "-x509", "-newkey", "rsa:2048", "-nodes", + "-keyout", str(key), "-out", str(cls.cert), "-days", "1", + "-subj", "/CN=127.0.0.1", "-addext", "subjectAltName=IP:127.0.0.1", + ], check=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL) + cls.context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) + cls.context.load_cert_chain(cls.cert, key) + + @classmethod + def tearDownClass(cls): + cls.files.cleanup() + + def setUp(self): + self.server = LocalDohServer(self.context) + self.server_thread = threading.Thread(target=self.server.serve_forever) + self.server_thread.start() + self.clients = [] + self.process = None + self.log = tempfile.TemporaryFile(mode="w+b") + self.memory_log = self.root / (self.id().rsplit(".", 1)[-1] + ".valgrind") + + def tearDown(self): + if self.process is not None and self.process.poll() is None: + self.process.kill() + self.process.wait() + for client in self.clients: + client.close() + self.server.release.set() + self.server.shutdown() + self.server.server_close() + self.server_thread.join() + self.log.seek(0) + output = self.log.read().decode(errors="replace") + self.log.close() + if self.process is not None: + memory_output = self.memory_log.read_text() if self.memory_log.exists() else "" + self.assertEqual(self.process.returncode, 0, output + memory_output) + + def start_proxy(self): + with socket.socket() as reservation: + reservation.bind(("127.0.0.1", 0)) + self.port = reservation.getsockname()[1] + command = [str(OPTIONS.binary), "-a", "127.0.0.1", "-p", str(self.port), + "-r", f"https://127.0.0.1:{self.server.server_port}/dns-query", + "-C", str(self.cert), "-x", "-4", "-vvv"] + if OPTIONS.valgrind: + command = [OPTIONS.valgrind, "--error-exitcode=99", "--leak-check=full", + "--show-leak-kinds=definite,indirect", + "--errors-for-leak-kinds=definite,indirect", + f"--log-file={self.memory_log}"] + command + environment = os.environ.copy() + for name in ("HTTP_PROXY", "HTTPS_PROXY", "ALL_PROXY", "http_proxy", + "https_proxy", "all_proxy"): + environment.pop(name, None) + environment["NO_PROXY"] = "127.0.0.1" + self.process = subprocess.Popen(command, stdout=self.log, stderr=self.log, + env=environment) + deadline = time.monotonic() + 15 + while time.monotonic() < deadline: + if self.process.poll() is not None: + self.fail("Proxy exited during startup") + try: + return self.connect() + except ConnectionRefusedError: + time.sleep(0.02) + self.fail("Proxy did not start listening") + + def connect(self): + client = socket.create_connection(("127.0.0.1", self.port), timeout=10) + self.clients.append(client) + return client + + def stop_proxy(self): + start = time.monotonic() + self.process.send_signal(signal.SIGINT) + self.process.wait(timeout=7) + return time.monotonic() - start + + def test_idle_shutdown_does_not_wait_for_deadline(self): + self.start_proxy() + self.assertLess(self.stop_proxy(), 2) + + def test_shutdown_delivers_response_that_finishes_within_grace(self): + self.server.release.clear() + client = self.start_proxy() + request = query(123) + client.sendall(struct.pack("!H", len(request)) + request) + self.assertTrue(self.server.received.wait(10)) + self.process.send_signal(signal.SIGINT) + # Keep the upstream response pending until the proxy is draining. + # This must fail if tcp_stop() immediately drops every connection. + release = threading.Timer(0.2, self.server.release.set) + release.start() + try: + self.assertEqual(receive_frame(client), answer(request)) + finally: + release.cancel() + release.join() + self.assertEqual(client.recv(1), b"") + self.process.wait(timeout=2) + + def check_shutdown_deadline(self, tcp): + self.server.release.clear() + client = self.start_proxy() + request = query(456) + if tcp: + client.sendall(struct.pack("!H", len(request)) + request) + else: + udp = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + self.clients.append(udp) + udp.sendto(request, ("127.0.0.1", self.port)) + self.assertTrue(self.server.received.wait(10)) + elapsed = self.stop_proxy() + self.assertGreater(elapsed, 4) + self.assertLess(elapsed, 6.5) + + def test_shutdown_bounds_outstanding_tcp_fetch(self): + self.check_shutdown_deadline(tcp=True) + + def test_shutdown_bounds_outstanding_udp_fetch(self): + self.check_shutdown_deadline(tcp=False) + + def test_slow_reader_does_not_stall_another_client(self): + slow = self.start_proxy() + slow.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 4096) + requests = [query(identifier) for identifier in range(24)] + slow.sendall(b"".join(struct.pack("!H", len(req)) + req for req in requests)) + self.assertTrue(self.server.received.wait(10)) + time.sleep(0.2) # Let responses accumulate while this client does not read. + fast = self.connect() + request = query(1000) + start = time.monotonic() + fast.sendall(struct.pack("!H", len(request)) + request) + self.assertEqual(receive_frame(fast), answer(request)) + self.assertLess(time.monotonic() - start, 2) + slow.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 131072) + responses = [receive_frame(slow) for _ in requests] + self.assertEqual(sorted(responses), sorted(answer(req) for req in requests)) + self.assertLess(self.stop_proxy(), 2) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + parser.add_argument("--binary", type=Path, required=True) + parser.add_argument("--openssl", required=True) + parser.add_argument("--valgrind") + OPTIONS = parser.parse_args() + unittest.main(argv=[__file__], verbosity=2) diff --git a/tests/robot/DnsTcpClient.py b/tests/robot/DnsTcpClient.py index 35ba0b2..db07d1f 100644 --- a/tests/robot/DnsTcpClient.py +++ b/tests/robot/DnsTcpClient.py @@ -1,4 +1,6 @@ import socket +import time +from pathlib import Path class DnsTcpClient: @@ -15,6 +17,25 @@ class DnsTcpClient: def __init__(self): self.client_socket = None + def wait_for_proxy_ready(self, process, logfile, timeout=15): + deadline = time.monotonic() + float(timeout) + ready_markers = ( + "Received new DNS server IP", "DNS polling disabled.", + "Systemd notify succeeded", "NOTIFY_SOCKET not set", + "Systemd notify skipped", + ) + while time.monotonic() < deadline: + if process.poll() is not None: + raise AssertionError("Proxy exited before it became ready") + try: + output = Path(logfile).read_text(errors="replace") + if any(marker in output for marker in ready_markers): + return + except FileNotFoundError: + pass + time.sleep(0.05) + raise AssertionError(f"Proxy did not finish bootstrapping within {timeout}s") + def open_tcp_client_connection(self, host, port): try: self.client_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) diff --git a/tests/robot/functional_tests.robot b/tests/robot/functional_tests.robot index 8595ea3..145aaec 100644 --- a/tests/robot/functional_tests.robot +++ b/tests/robot/functional_tests.robot @@ -29,13 +29,14 @@ Start Proxy [Arguments] @{args} @{default_args} = Create List -v -v -v -4 -p ${PORT} @{proces_args} = Combine Lists ${default_args} ${args} + ${proxy_log} = Set Variable ${TEMPDIR}${/}https-dns-proxy-${PORT}.log ${proxy} = Start Process ${BINARY_PATH} @{proces_args} - ... stderr=STDOUT alias=proxy + ... stdout=${proxy_log} stderr=STDOUT alias=proxy Set Test Variable ${proxy} Set Test Variable ${dig_timeout} 2 Set Test Variable ${dig_retry} 0 - Sleep 0.5 - Is Process Running ${proxy} + Wait For Proxy Ready ${proxy} ${proxy_log} + Process Should Be Running ${proxy} Common Test Setup Start Proxy With Valgrind @@ -45,13 +46,14 @@ Start Proxy With Valgrind ... --show-leak-kinds=all --track-origins=yes --keep-stacktraces=alloc-and-free ... ${BINARY_PATH} -v -v -v -F 100 -4 -p ${PORT} # using flight recorder with smallest possible buffer size to test memory leak @{proces_args} = Combine Lists ${default_args} ${args} + ${proxy_log} = Set Variable ${TEMPDIR}${/}https-dns-proxy-${PORT}.log ${proxy} = Start Process valgrind @{proces_args} - ... stderr=STDOUT alias=proxy + ... stdout=${proxy_log} stderr=STDOUT alias=proxy Set Test Variable ${proxy} Set Test Variable ${dig_timeout} 10 Set Test Variable ${dig_retry} 2 - Sleep 6 # wait for valgrind to fire up the proxy - Is Process Running ${proxy} + Wait For Proxy Ready ${proxy} ${proxy_log} + Process Should Be Running ${proxy} Common Test Setup Stop Proxy diff --git a/tests/unit/test_dns_listener_tcp.c b/tests/unit/test_dns_listener_tcp.c index 5cacdff..8e89cdd 100644 --- a/tests/unit/test_dns_listener_tcp.c +++ b/tests/unit/test_dns_listener_tcp.c @@ -182,6 +182,23 @@ static void test_resume_drains_buffer_before_reading_socket(void) { TEST_ASSERT(memcmp(unread, frame, sizeof(frame)) == 0); } +static void test_dispatches_buffered_request_when_socket_would_block(void) { + TEST_ASSERT(fcntl(sockets[0], F_SETFL, O_NONBLOCK) == 0); + const uint16_t length = htons(DNS_HEADER_LENGTH); + memcpy(client.input_buffer, &length, sizeof(length)); + client.input_buffer_used = sizeof(length) + DNS_HEADER_LENGTH; + unsigned requests = 0; + listener.cb = consume_request; + listener.cb_data = &requests; + + read_cb(listener.loop, &client.read_watcher, EV_READ); + + TEST_ASSERT(requests == 1); + TEST_ASSERT(client.pending_requests == 1); + TEST_ASSERT(client.input_buffer_used == 0); + TEST_ASSERT(listener.clients == &client); +} + static void test_read_respects_remaining_input_capacity(void) { client.input_buffer_size = TCP_DNS_MAX_FRAME; client.input_buffer = realloc(client.input_buffer, client.input_buffer_size); @@ -300,6 +317,59 @@ static struct tcp_client_s *heap_client(void) { return owned; } +static void test_dispatch_rejects_short_request(void) { + struct tcp_client_s *owned = heap_client(); + TEST_ASSERT(owned != NULL); + const uint16_t length = htons(DNS_HEADER_LENGTH - 1); + memcpy(owned->input_buffer, &length, sizeof(length)); + owned->input_buffer_used = sizeof(length) + DNS_HEADER_LENGTH - 1; + + const int result = dispatch_client_requests(owned); + if (listener.clients == NULL) { + sockets[0] = -1; + } + + TEST_ASSERT(result == -1); + TEST_ASSERT(listener.clients == NULL); +} + +static void test_output_limit_drops_client_and_frees_queue(void) { + struct tcp_client_s *owned = heap_client(); + TEST_ASSERT(owned != NULL); + TEST_ASSERT(fcntl(sockets[0], F_SETFL, O_NONBLOCK) == 0); + char fill[4096] = {0}; + while (send(sockets[0], fill, sizeof(fill), MSG_NOSIGNAL) > 0) { + } + TEST_ASSERT(errno == EAGAIN || errno == EWOULDBLOCK); + char response[UINT16_MAX] = {0}; + tcp_respond(&listener.base, (struct sockaddr *)&owned->raddr, + owned->id, NULL, 0, response, sizeof(response)); + TEST_ASSERT(owned->output_head != NULL); + TEST_ASSERT(owned->output_bytes == TCP_DNS_MAX_FRAME); + + tcp_respond(&listener.base, (struct sockaddr *)&owned->raddr, + owned->id, NULL, 0, response, sizeof(response)); + if (listener.clients == NULL) { + sockets[0] = -1; + } + TEST_ASSERT(listener.clients == NULL); + TEST_ASSERT(listener.client_count == 0); +} + +static void test_failed_send_drops_client(void) { + struct tcp_client_s *owned = heap_client(); + TEST_ASSERT(owned != NULL); + TEST_ASSERT(shutdown(sockets[0], SHUT_WR) == 0); + char response[DNS_HEADER_LENGTH] = {0}; + tcp_respond(&listener.base, (struct sockaddr *)&owned->raddr, + owned->id, NULL, 0, response, sizeof(response)); + if (listener.clients == NULL) { + sockets[0] = -1; + } + TEST_ASSERT(listener.clients == NULL); + TEST_ASSERT(listener.client_count == 0); +} + static void check_queued_response_drains(int stopping) { struct tcp_client_s *owned = heap_client(); TEST_ASSERT(owned != NULL); @@ -356,6 +426,9 @@ static void test_shutdown_waits_for_pending_response(void) { TEST_ASSERT(!ev_is_active(&owned->read_watcher)); TEST_ASSERT(!ev_is_active(&listener.accept_watcher)); const ev_tstamp deadline = owned->timer_watcher.at; + const ev_tstamp remaining = ev_timer_remaining(listener.loop, &owned->timer_watcher); + TEST_ASSERT(remaining >= DNS_SHUTDOWN_TIMEOUT_S - 0.1 && + remaining <= DNS_SHUTDOWN_TIMEOUT_S + 0.1); char response[DNS_HEADER_LENGTH] = {0}; tcp_respond(&listener.base, (struct sockaddr *)&owned->raddr, owned->id, NULL, 0, response, sizeof(response)); @@ -438,9 +511,13 @@ int main(void) { TEST_RUN(test_large_partial_request_grows_without_exiting); TEST_RUN(test_request_completion_resumes_client_reads); TEST_RUN(test_resume_drains_buffer_before_reading_socket); + TEST_RUN(test_dispatches_buffered_request_when_socket_would_block); + TEST_RUN(test_dispatch_rejects_short_request); TEST_RUN(test_read_respects_remaining_input_capacity); TEST_RUN(test_old_connection_cannot_complete_or_respond_to_new_client); TEST_RUN(test_queues_response_without_blocking_event_loop); + TEST_RUN(test_output_limit_drops_client_and_frees_queue); + TEST_RUN(test_failed_send_drops_client); TEST_RUN(test_half_close_keeps_queued_response_until_drained); TEST_RUN(test_shutdown_keeps_queued_response_until_drained); TEST_RUN(test_shutdown_waits_for_pending_response); diff --git a/tests/unit/test_dns_poller.c b/tests/unit/test_dns_poller.c index 6dc59db..6de76ce 100644 --- a/tests/unit/test_dns_poller.c +++ b/tests/unit/test_dns_poller.c @@ -132,10 +132,10 @@ static void test_query_cache_disabled(void) { int optmask = 0; memset(&saved, 0, sizeof(saved)); TEST_ASSERT(ares_save_options(poller.ares, &saved, &optmask) == ARES_SUCCESS); - const int cache_option_set = (optmask & ARES_OPT_QUERY_CACHE) != 0; const unsigned int max_ttl = saved.qcache_max_ttl; ares_destroy_options(&saved); - TEST_ASSERT(cache_option_set); + // c-ares before 1.31 omits ARES_OPT_QUERY_CACHE when the cache is disabled. + // Newer releases retain it with a zero TTL; both represent a disabled cache. TEST_ASSERT(max_ttl == 0); #endif } diff --git a/tests/unit/test_doh_proxy_resolver.c b/tests/unit/test_doh_proxy_resolver.c index 1383751..341d078 100644 --- a/tests/unit/test_doh_proxy_resolver.c +++ b/tests/unit/test_doh_proxy_resolver.c @@ -163,10 +163,39 @@ static void test_preserves_connection_identity_through_proxy_callbacks(void) { } } +static void test_listener_without_completion_callback(void) { + dns_listener_t listener = { + .respond = record_response, + .transport = DNS_TRANSPORT_UDP + }; + struct sockaddr_in address = {.sin_family = AF_INET}; + for (unsigned attempt = 0; attempt < 3; attempt++) { + char *request = calloc(1, DNS_HEADER_LENGTH); + TEST_ASSERT(request != NULL); + pending_response_cb = NULL; + if (attempt == 2) { + doh_proxy_await_bootstrap(proxy, NULL); + } + doh_proxy_handle_request(proxy, &listener, (struct sockaddr *)&address, + 0, request, DNS_HEADER_LENGTH); + if (attempt == 2) { + TEST_ASSERT(pending_response_cb == NULL); + } else { + TEST_ASSERT(pending_response_cb != NULL); + char response[DNS_HEADER_LENGTH] = {0}; + pending_response_cb(pending_response_data, attempt == 0 ? response : NULL, + attempt == 0 ? sizeof(response) : 0); + } + TEST_ASSERT(response_calls == 1); + TEST_ASSERT(completion_calls == 0); + } +} + int main(void) { TEST_RUN(test_new_address_triggers_resolver_update); TEST_RUN(test_removed_address_triggers_resolver_update); TEST_RUN(test_exact_address_match_after_prefix_match); TEST_RUN(test_preserves_connection_identity_through_proxy_callbacks); + TEST_RUN(test_listener_without_completion_callback); return test_summary(); } diff --git a/tests/unit/test_https_client_limits.c b/tests/unit/test_https_client_limits.c index 8b41617..0400b11 100644 --- a/tests/unit/test_https_client_limits.c +++ b/tests/unit/test_https_client_limits.c @@ -77,6 +77,52 @@ static void test_rejects_requests_above_fetch_limit(void) { TEST_ASSERT(callback_state.failures == 1); } +typedef struct { + size_t fetch_count; + size_t listed_fetches; +} replacement_state_t; + +static void submit_replacement_request(void *data, char *buf, size_t buflen) { + (void)buf; + (void)buflen; + replacement_state_t *state = data; + state->fetch_count = client.fetch_count; + for (struct https_fetch_ctx *ctx = client.fetches; ctx != NULL; ctx = ctx->next) { + state->listed_fetches++; + } + static const char request[DNS_HEADER_LENGTH] = {0}; + https_client_fetch(&client, "https://127.0.0.1/dns-query", + request, sizeof(request), NULL, UINT16_MAX, + response_cb, &callback_state); +} + +static void test_completion_callback_can_replace_request_at_fetch_limit(void) { + static const char request[DNS_HEADER_LENGTH] = {0}; + for (size_t i = 0; i < HTTPS_FETCH_LIMIT; i++) { + https_client_fetch(&client, "https://127.0.0.1/dns-query", + request, sizeof(request), NULL, (uint16_t)i, + response_cb, &callback_state); + } + TEST_ASSERT(client.fetch_count == HTTPS_FETCH_LIMIT); + replacement_state_t state = {0}; + struct https_fetch_ctx *prev = client.fetches; + struct https_fetch_ctx *completed = prev->next; + completed->cb = submit_replacement_request; + completed->cb_data = &state; + + https_fetch_ctx_cleanup(&client, prev, completed, -1); + + TEST_ASSERT(state.fetch_count == HTTPS_FETCH_LIMIT - 1); + TEST_ASSERT(state.listed_fetches == HTTPS_FETCH_LIMIT - 1); + TEST_ASSERT(callback_state.calls == 0); + TEST_ASSERT(client.fetch_count == HTTPS_FETCH_LIMIT); + size_t listed_fetches = 0; + for (struct https_fetch_ctx *ctx = client.fetches; ctx != NULL; ctx = ctx->next) { + listed_fetches++; + } + TEST_ASSERT(listed_fetches == client.fetch_count); +} + static void test_socket_watchers_grow_and_support_fd_zero(void) { CURL *fake_easy_handle = (CURL *)(uintptr_t)1; enum { WATCHER_COUNT = 16 }; @@ -125,6 +171,7 @@ static void test_rejects_socket_when_interface_binding_fails(void) { int main(void) { TEST_RUN(test_rejects_requests_above_fetch_limit); + TEST_RUN(test_completion_callback_can_replace_request_at_fetch_limit); TEST_RUN(test_socket_watchers_grow_and_support_fd_zero); TEST_RUN(test_aborted_multi_action_schedules_client_reset); TEST_RUN(test_rejects_socket_when_interface_binding_fails);