Harden DNS proxy lifecycles and add outbound interface binding by aarond10 · Pull Request #214 · aarond10/https_dns_proxy

aarond10 · 2026-10-05T21:23:47Z

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.

aarond10 · 2026-10-05T21:28:58Z

@baranyaib90, the bulk of these are AI generated. I've reviewed them and they look reasonable and I'm very happy with the better test coverage we have here now. Do you have any thoughts before a squash and merge?

baranyaib90 · 2026-10-06T06:26:47Z

Hi @aarond10, I'm kinda afraid already of the review. Last time there were plenty bugs in the pack.
Please give me a few weeks to check, before you merge. Thanks.

aarond10 · 2026-10-06T09:35:04Z

No rush! The last set I trusted way too much and didn't check carefully. I hope I've learned a bit better since then how to go about these. Please let me know what you think.

…

On Tue, 6 Oct 2026 at 17:27, baranyaib90 ***@***.***> wrote: *baranyaib90* left a comment (aarond10/https_dns_proxy#214) <#214 (comment)> Hi @aarond10 <https://github.com/aarond10>, I'm kinda afraid already of the review. Last time there were plenty bugs in the pack. Please give me a few weeks to check, before you merge. Thanks. — Reply to this email directly, view it on GitHub <#214?email_source=notifications&email_token=AABTOXSBLFWZLNGYBIVNLED5SSGDZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBRGA3DSMRYGQZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-6010692842>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABTOXRDNJTOKOGLWX2UC2T5SSGDZAVCNFSNUABEKJSXA33TNF2G64TZHM2TOMJSHA2TGOJ3JFZXG5LFHM2TOMJZGEYDMMJSGGQXMAQ> . Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS <https://github.com/notifications/mobile/ios/AABTOXTVNGJDORD6BBLGIAL5SSGDZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBRGA3DSMRYGQZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y> and Android <https://github.com/notifications/mobile/android/AABTOXUJ56TWUW7A5NPKLGL5SSGDZA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTMMBRGA3DSMRYGQZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>. Download it today! You are receiving this because you were mentioned.Message ID: ***@***.***>

baranyaib90

I have checked the changes, quite many. I was not that thorough, I hope that you are using an advanced AI model and not a cheap one ;)
I'm mostly OK with them.

baranyaib90 · 2026-10-17T17:38:37Z

  // 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;


network_req_size is unnecessary, previous one-liner to set *req_size will be now safe enough

Removed the extra temporary variable. The prefix is copied directly into *req_size with memcpy, followed by ntohs. I've kept memcpy rather than restoring the pointer-cast read.

baranyaib90 · 2026-10-17T17:41:30Z

  if (data_size > client->input_buffer_used) {
    return 0;  // Partial request
  }
+  if (*req_size < DNS_HEADER_LENGTH) {


I think that this whole condition is unnecessary:

upper layer will drop client properly

the actual purpose of this function is, to amound N bytes of data according to the number of the first 2 bytes. validating the extracted data is not needed here.

Agreed. The code now leaves DNS-header validation to dispatch_client_requests() and consumes the frame in one place. Extraction only special-cases a zero-length payload to avoid malloc(0), whose NULL result would otherwise be treated as a fatal allocation failure. A regression test checks that a short frame still causes the caller to drop the client.

baranyaib90 · 2026-10-17T19:45:45Z

      DLOG_CLIENT("TCP client closed connection");
    } else if (errno == EAGAIN || errno == EWOULDBLOCK) {
-      return;
+      len = 0;


why was return removed?

Restored the early return. In the final implementation, buffered requests are dispatched before recv(), so returning on EAGAIN/EWOULDBLOCK is safe. Added a test with a complete buffered request and an empty nonblocking socket to cover this path.

baranyaib90 · 2026-10-17T19:46:03Z

-      if (client->input_buffer_size > TCP_DNS_MAX_PAYLOAD) {
-        FLOG_CLIENT("Unrealistic input buffer size: %u", client->input_buffer_size);
-      }
+  if (len > 0) {


no need for this dumb condition if return is put back

Removed the len > 0 wrapper. EOF and read errors now return before the append logic, so only positive reads reach it.

baranyaib90 · 2026-10-17T19:54:28Z

    }
  }

+  req->listener->request_complete(req->listener, (struct sockaddr*)&req->raddr);


now in case of successful request this looks up the TCP client again. it would be nice to modify this callback to failed_request.
because respond callback could do the "request_complete" tasks inside it and failed_request could do the "request_complete" stuff if respond cb. was not called

I'd keep the shared request_complete callback so success and every failure case use one completion path. It makes releasing pending-request capacity explicit even for malformed replies, mismatched IDs, upstream failures, and bootstrap rejection. This does mean a second client lookup after a successful reply, but I'd prefer that small cost to splitting completion accounting between respond() and a separate failure callback.

baranyaib90 · 2026-10-17T20:04:08Z

    if (old_addr_list) {
      old_addr_list += strlen(port_colon);
-      if (!addr_list_reduced(addr_list, old_addr_list)) {
+      if (!addr_list_reduced(old_addr_list, addr_list) &&


again: add comment from other PR

This explanatory comment is now included through the merged #215. The rebase keeps that implementation rather than replacing it with the overlapping version from this PR.

baranyaib90 · 2026-10-17T20:12:27Z

  }
 }

+static void udp_request_complete(dns_listener_t __attribute__((unused)) *self,


instead of empty function, thic callback could be NULL and only called when non-NULL

Removed the empty UDP callback and documented request_complete as optional. Both the normal completion path and the early bootstrap-rejection path check for NULL. Added coverage for success, upstream failure, and bootstrap rejection without a completion callback.

baranyaib90 · 2026-10-17T20:18:48Z

  return 1;
 }

+static int dispatch_client_requests(struct tcp_client_s *client) {


to be honest: I would rather not review this code.
I understand the intention and I'm OK with it. when I was writing the send logic, it was good enough.
if the proxy is not under extreme load, the blocking send of mine would not block in 99% of cases, thanks to 1MB TCP output buffer from modern kernels.

I would like to ask you to review this commit thoroughly with high-end AI model, like claude-sonet-5 (1M ctx) or at least with latest sonnet. not with cheap dummy free models.

Added more validation rather than relying on kernel buffering. Deterministic unit tests exercise a full send buffer, queued frame ordering, queue-limit removal, failed sends, half-closes, and shutdown. Five end-to-end tests use a local TLS DoH server to cover idle exit, completion during graceful shutdown, stalled TCP/UDP requests, and a slow-reading client alongside a healthy client.

The complete Linux CTest suite passes, including the existing functional tests and privileged interface-binding test. All eight unit targets and the TLS lifecycle scenarios also pass under Valgrind, and the Linux unit/integration tests pass with ASan/UBSan. An independent code review found an unaligned response-ID load, which is now decoded with memcpy; the final review found no remaining blockers.

baranyaib90 · 2026-10-17T20:21:22Z


  // 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


thankfully we are 1 threaded, so was not a problem

Agreed that callbacks execute on one thread, so there is no concurrent callback race here. I've kept the conservative ordering: remove and clean up the outstanding curl handles before freeing their old resolver list. That follows the CURLOPT_RESOLVE lifetime contract and avoids depending on whether curl touches the list during reset. It is a small ordering change and makes the ownership clear.

baranyaib90 · 2026-10-17T20:26:01Z

  size_t fetch_count;

  ev_timer timer;
-  ev_io io_events[HTTPS_SOCKET_LIMIT];


there were no issues with the static array, but OK. linked list would not take that much time to go through

Keeping this change. Curl's socket callback can include internal descriptors in addition to its connection sockets, so the connection limit does not guarantee a sufficient watcher count. The old free-slot marker also used descriptor 0, which is a valid descriptor. Allocating stable watcher nodes as needed handles both cases without moving watchers that libev retains pointers to.

baranyaib90 · 2026-10-18T12:14:38Z

-  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,


15s is too much. normally DNS clients won't ever wait this much for a request.
at least lower to 5s, but my honest opinion is to drop everything without graceful shutdown.
DNS is unreliable. clients should just retry (with different resolver)

Reduced the grace period to five seconds. Reading the shutdown path showed that changing only the TCP timer would still leave longer HTTPS requests or reset timers keeping the process alive, so there is now a shared five-second drain deadline. Idle shutdown still returns immediately, and responses that finish within the grace period are delivered. At the deadline, HTTPS transfers are cancelled while the listeners and proxy remain alive for completion callbacks, then remaining TCP queues are freed.

End-to-end tests verify graceful completion and a five-second bound with stalled TCP and UDP requests. These paths also pass Valgrind and ASan/UBSan.

baranyaib90 · 2026-10-18T12:16:26Z

-  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) {


minor: add parenthesis

Added parentheses around TCP_CLIENT_OUTPUT_LIMIT - client->output_bytes.

baranyaib90 · 2026-10-18T12:20:05Z

+  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);


minor optimalization: could save some memory copy if this function would take over the resp pointer instead of copying the data and let upper layer release.

Agreed to leave the buffer-ownership optimization for a separate PR; this change is already large enough. The TCP queue keeps the copy it currently needs, since the HTTPS layer frees its response buffer when the callback returns. There is no ownership-transfer API change in this PR.

baranyaib90 · 2026-10-18T12:31:18Z

generic comment: next time with AI you could generate the commit message like:

title
max. 10 line chapter about the change for humans
rest: detailed reasoning: what/why/how changed

aarond10

Thanks for the review. Addressed the parser/read-path and request-accounting suggestions, kept a shared completion path, and bounded graceful shutdown to five seconds. Resolver-list lifetime and dynamic watcher changes remain; buffer-ownership optimization is deferred. #215 was merged first and this PR rebased onto it.

Validation: the complete 13-target Linux CTest suite passes, including all ten Robot functional cases, privileged interface binding, and TLS lifecycle tests. All eight unit targets and the TLS lifecycle tests pass under Valgrind; the Linux unit/integration suite passes ASan/UBSan. An independent code review found no remaining blockers after the response-ID alignment fix.

aarond10 · 2026-10-19T06:01:46Z

  // 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;


Removed the extra temporary variable. The prefix is copied directly into *req_size with memcpy, followed by ntohs. I've kept memcpy rather than restoring the pointer-cast read.

aarond10 · 2026-10-19T06:01:47Z

  if (data_size > client->input_buffer_used) {
    return 0;  // Partial request
  }
+  if (*req_size < DNS_HEADER_LENGTH) {


Agreed. The code now leaves DNS-header validation to dispatch_client_requests() and consumes the frame in one place. Extraction only special-cases a zero-length payload to avoid malloc(0), whose NULL result would otherwise be treated as a fatal allocation failure. A regression test checks that a short frame still causes the caller to drop the client.

aarond10 · 2026-10-19T06:01:48Z

      DLOG_CLIENT("TCP client closed connection");
    } else if (errno == EAGAIN || errno == EWOULDBLOCK) {
-      return;
+      len = 0;


Restored the early return. In the final implementation, buffered requests are dispatched before recv(), so returning on EAGAIN/EWOULDBLOCK is safe. Added a test with a complete buffered request and an empty nonblocking socket to cover this path.

aarond10 · 2026-10-19T06:01:50Z

-      if (client->input_buffer_size > TCP_DNS_MAX_PAYLOAD) {
-        FLOG_CLIENT("Unrealistic input buffer size: %u", client->input_buffer_size);
-      }
+  if (len > 0) {


Removed the len > 0 wrapper. EOF and read errors now return before the append logic, so only positive reads reach it.

aarond10 · 2026-10-19T06:01:51Z

  } else {
    client->fetches = ctx->next;
  }
+  if (client->fetch_count == 0) {


Moved the list unlink and fetch_count decrement to just after curl_multi_remove_handle(), before the completion callback. This also lets the callback submit a replacement request when the client was at its fetch limit. The new regression test fails with the old ordering and passes with this change.

aarond10 · 2026-10-19T07:21:33Z

  return 1;
 }

+static int dispatch_client_requests(struct tcp_client_s *client) {


Added more validation rather than relying on kernel buffering. Deterministic unit tests exercise a full send buffer, queued frame ordering, queue-limit removal, failed sends, half-closes, and shutdown. Five end-to-end tests use a local TLS DoH server to cover idle exit, completion during graceful shutdown, stalled TCP/UDP requests, and a slow-reading client alongside a healthy client.

The complete Linux CTest suite passes, including the existing functional tests and privileged interface-binding test. All eight unit targets and the TLS lifecycle scenarios also pass under Valgrind, and the Linux unit/integration tests pass with ASan/UBSan. An independent code review found an unaligned response-ID load, which is now decoded with memcpy; the final review found no remaining blockers.

aarond10 · 2026-10-19T07:21:34Z


  // 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


Agreed that callbacks execute on one thread, so there is no concurrent callback race here. I've kept the conservative ordering: remove and clean up the outstanding curl handles before freeing their old resolver list. That follows the CURLOPT_RESOLVE lifetime contract and avoids depending on whether curl touches the list during reset. It is a small ordering change and makes the ownership clear.

aarond10 · 2026-10-19T07:21:35Z

  size_t fetch_count;

  ev_timer timer;
-  ev_io io_events[HTTPS_SOCKET_LIMIT];


Keeping this change. Curl's socket callback can include internal descriptors in addition to its connection sockets, so the connection limit does not guarantee a sufficient watcher count. The old free-slot marker also used descriptor 0, which is a valid descriptor. Allocating stable watcher nodes as needed handles both cases without moving watchers that libev retains pointers to.

aarond10 · 2026-10-19T07:21:37Z

-  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,


Reduced the grace period to five seconds. Reading the shutdown path showed that changing only the TCP timer would still leave longer HTTPS requests or reset timers keeping the process alive, so there is now a shared five-second drain deadline. Idle shutdown still returns immediately, and responses that finish within the grace period are delivered. At the deadline, HTTPS transfers are cancelled while the listeners and proxy remain alive for completion callbacks, then remaining TCP queues are freed.

End-to-end tests verify graceful completion and a five-second bound with stalled TCP and UDP requests. These paths also pass Valgrind and ASan/UBSan.

aarond10 · 2026-10-19T07:21:38Z

+  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);


Agreed to leave the buffer-ownership optimization for a separate PR; this change is already large enough. The TCP queue keeps the copy it currently needs, since the HTTPS layer frees its response buffer when the callback returns. There is no ownership-transfer API change in this PR.

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.

aarond10 marked this pull request as ready for review October 5, 2026 21:27

emgeorrk mentioned this pull request Oct 16, 2026

Fix resolver list getting stuck after a partial bootstrap result #215

Merged

baranyaib90 reviewed Oct 17, 2026

View reviewed changes

baranyaib90 reviewed Oct 18, 2026

View reviewed changes

aarond10 added 9 commits October 19, 2026 16:25

dns_tcp: prevent frame length overflow

83f6b72

https_client: bound in-flight requests

1fa4e76

options: drop to user's primary group

2746cdd

doh_proxy: detect newly added resolver addresses

d33431a

dns_tcp: queue responses without blocking

63de734

doh_proxy: retain resolve list through client reset

d7540a2

https_client: allocate socket watchers on demand

b4dd4d4

Bind HTTPS and bootstrap DNS sockets to an outbound interface

c437451

options: avoid implicit group reset for non-root startup

055e3a7

aarond10 force-pushed the review/dns-proxy-hardening branch from c574e4b to 055e3a7 Compare October 19, 2026 05:26

aarond10 commented Oct 19, 2026

View reviewed changes

aarond10 force-pushed the review/dns-proxy-hardening branch from b0299d9 to 94b5045 Compare October 19, 2026 09:28

aarond10 force-pushed the review/dns-proxy-hardening branch from 94b5045 to 69a2065 Compare October 19, 2026 10:24

aarond10 merged commit e83e0b6 into master Oct 19, 2026
4 checks passed

Conversation

aarond10 commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aarond10 commented Oct 5, 2026

Uh oh!

baranyaib90 commented Oct 6, 2026

Uh oh!

aarond10 commented Oct 6, 2026 via email

Uh oh!

baranyaib90 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baranyaib90 commented Oct 18, 2026

Uh oh!

aarond10 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarond10 commented Oct 5, 2026 •

edited

Loading