Skip to content

feat: async DNS resolver (hdns) + integrate into evpp clients#855

Merged
ithewei merged 17 commits into
masterfrom
dns
Jul 23, 2026
Merged

feat: async DNS resolver (hdns) + integrate into evpp clients#855
ithewei merged 17 commits into
masterfrom
dns

Conversation

@ithewei

@ithewei ithewei commented Jul 23, 2026

Copy link
Copy Markdown
Owner

Summary

Add a self-contained, non-blocking, native UDP async DNS resolver that runs
entirely inside the hloop event loop, and integrate it into the evpp client
connect path so hostname resolution no longer blocks the loop with a synchronous
getaddrinfo().

Approach follows libevent's evdns (event-driven UDP), not libuv's thread-pool
offload, and adds no third-party dependency (independent from the demo
protocol/dns.c).

What's included

Core resolver — event/hdns.{h,c} (compiled by default, no build flag)

  • Async A / AAAA queries, results merged (IPv4 first).
  • Nameservers from /etc/resolv.conf (Unix) / GetAdaptersAddresses (Windows),
    universal fallback to 8.8.8.8.
  • Loads /etc/hosts; numeric-IP fast path; per-attempt timeout + retry +
    multi-nameserver rotation (IPv4/IPv6 sockets); DNS name-compression parsing.
  • TTL-respecting per-loop cache (positive + negative).
  • hdns_t is an hevent_t subclass (like htimer_t/hio_t); C API returns a
    hdns_t* handle, callback receives the handle, hdns_cancel(hdns_t*).

C++ handle model — evpp/Event.h, EventLoop.h

  • EventLoop::resolveDns()/cancelDns() mirror setTimer()/killTimer() with a
    DnsID -> query map, making the handle use-after-free-proof (a stale id
    no-ops), same pattern as TimerID.

Client integration

  • TcpClientEventLoopTmpl resolves hostnames asynchronously on first connect and
    every reconnect; numeric IPs keep the sync fast path. WebSocketClient and any
    other TcpClientTmpl subclass inherit this for free (no per-client glue).
  • AsyncHttpClient uses EventLoop::resolveDns on its hostname path.
  • First-attempt resolve failure still notifies onConnection with a valid
    (NULL-io) channel; Channel::error() surfaces the reason (ERR_DNS_RESOLVE
    added to herr.h) and is null-safe.

Build / examples / tests / docs

  • Wired into Makefile, CMake and Bazel; iphlpapi linked on Windows.
  • examples/host.c (async DNS lookup tool, nc/curl/wget style).
  • unittest/hdns_test.c (mock nameserver, wire round-trip, cache, cancel,
    NXDOMAIN), unittest/tcpclient_dns_test.cpp (connect+reconnect, destroy-mid-
    resolve lifetime, resolve-failure notification), unittest/hdns_benchmark.c
    (getaddrinfo vs async comparison).
  • docs/cn/hdns.md + README index; docs/PLAN.md marks async DNS done.

Testing

All DNS tests pass under both Makefile and CMake; real-network resolve verified;
benchmark shows async concurrent resolution is far faster than sequential
blocking and, more importantly, never stalls the event loop.

Notes / scope

  • First version intentionally excludes: search domains / ndots, TCP fallback on
    truncation, nameserver health-checking, and raw record types (SRV/TXT/MX...).
    These are documented as future enhancements.
  • AddressSanitizer could not run in the dev environment (a trivial ASan binary
    hangs at startup), so UAF-freedom is argued by the DnsID design plus the
    non-ASan lifetime regression test.

ithewei added 11 commits July 22, 2026 14:36
Add a native, non-blocking UDP DNS resolver that runs entirely inside the
hloop event loop, replacing blocking getaddrinfo on the async connect path.
Self-contained (no thirdparty dep, independent from protocol/dns.c).

- event/hdns.{h,c}: async A/AAAA queries, DNS wire codec with name
  compression, resolv.conf / hosts parsing, Windows GetAdaptersAddresses,
  8.8.8.8 fallback, per-attempt timeout + retry + nameserver rotation,
  numeric-IP fast path, cancelable query handle, TTL-respecting per-loop
  cache (positive + negative). Callbacks never fire re-entrantly.
- event/hloop.c, hevent.h: per-loop resolver ownership, freed in cleanup.
- evpp/TcpClient.h: reconnect re-resolves hostnames asynchronously instead
  of blocking the loop; numeric IPs and initial connect keep the fast path.
- examples: hdns_example, hdns_benchmark (getaddrinfo vs async comparison).
- unittest: hdns_test (mock nameserver, wire round-trip, cache, cancel,
  NXDOMAIN), tcpclient_dns_test (hostname connect + async reconnect).
- build: Makefile / CMake / Bazel wiring, install header, iphlpapi on win.
- docs/cn/hdns.md + README index; mark async DNS done in docs/PLAN.md.
AsyncHttpClient::doTask ran in the loop thread but resolved hostnames with
blocking getaddrinfo (via sockaddr_set_ipport), stalling the event loop.

- Split doTask: numeric IP / UDS keep the synchronous fast path; hostnames
  are resolved asynchronously via hdns_resolve, then continue in
  doTaskWithAddr from the callback. The loop is never blocked.
- Track in-flight DnsContext handles; cancel them and free contexts in the
  destructor to avoid leaks / dangling callbacks.
- unittest/asynchttp_dns_test: async HTTP request to a hostname URL
  (localhost) against a local server, resolved through hdns.
- build wiring (Makefile / CMake / unittest.sh); update docs/cn/hdns.md.
Move async hostname resolution fully into the TcpClientEventLoopTmpl base so
every client that inherits from TcpClientTmpl (TcpClient, WebSocketClient, and
any future XXXClient) gets non-blocking DNS for free — no per-client
onDnsResolved glue.

- startConnect() now async-resolves hostnames on BOTH first connect and every
  reconnect (previously reconnect-only), superseding the special case.
- createsocket(port, host) no longer blocks on getaddrinfo for hostnames: it
  records host/port and defers resolution to startConnect(); numeric IPs / UDS
  keep the synchronous fast path and the connfd contract.
- onDnsResolved: on resolve failure with no usable prior address, drive the
  normal failure/reconnect callback path instead of connecting to a zero addr.
- WebSocketClient needs zero changes; it inherits the behavior. Its previous
  blocking createsocket() in open() (also hit on redirect in the loop thread)
  is now non-blocking via the base.
- unittest/websocket_dns_test: ws://localhost connect + echo, proving the
  generic base handles WebSocket with no WS-specific DNS code.
- build wiring (Makefile / CMake / unittest.sh); update docs/cn/hdns.md.
The return value differs between numeric-IP and hostname targets, which looked
inconsistent. Document the three-state contract explicitly (matching the
>0/=0/<0 convention already used by examples/protorpc):

  >0  connfd  - socket created immediately (numeric IP, or UDS when port < 0)
  =0  pending - hostname; socket creation deferred until async DNS resolves
                (not an error; real fd available later via channel->fd())
  <0  error

Also explain why the =0 case is unavoidable: a socket's address family
(AF_INET vs AF_INET6) must be known before socket(), but for a hostname the
family is only known after DNS returns an A/AAAA record, so the socket is
created post-resolution in startConnectWithAddr() -- same order as the standard
getaddrinfo connect loop. Comment-only change; no behavior change.
- AsyncHttpClient dtor use-after-free: an owned loop uses HLOOP_FLAG_AUTO_FREE,
  so EventLoopThread::stop() runs hloop_free() -> hdns_resolver_free(), which
  already frees every in-flight hdns_query_t. The dtor then called
  hdns_cancel() on those freed pointers. Now the dtor only frees its own
  DnsContext shells and never touches the (already-freed) query handles.

- IPv6 nameserver transport: the resolver used a single AF_INET UDP socket, so
  sendto() to an IPv6 nameserver (resolv.conf "nameserver ::1", Windows IPv6
  DNS) always failed and timed out. Create the send/recv socket lazily per
  nameserver family (io4/io6) and pick the matching one per query.

- Concurrent-query txid collisions: txids were random (hv_rand), so two
  in-flight queries could share a txid and get responses demuxed to the wrong
  query. Use a per-resolver monotonic counter (seeded randomly), stepping by 2
  since each query uses base (A) and base+1 (AAAA).

- Response validation: reject packets without the QR (response) bit set, in
  addition to the existing txid + rcode checks.

All DNS unit/integration tests pass (hdns_test, tcpclient/asynchttp/websocket
_dns_test) under both Makefile and CMake; real-network resolve verified.
The C handle was a raw hdns_query_t* held by clients, which was fragile: when
the loop tore down (hloop_free -> hdns_resolver_free) it freed in-flight
queries while a client still held the pointer, and a resolve callback could
fire into a destroyed client -- both use-after-free / double-free hazards.

Redesign the handle ownership to mirror how EventLoop already handles timers
(TimerID -> htimer_t), which is the codebase's established UAF-proof pattern:

- event/hdns.{h,c}: rename hdns_query_t -> hdns_t and make struct hdns_s a
  subclass of hevent_t (HEVENT_FIELDS, like htimer_s/hio_s), so it carries an
  event_id and can use hevent_set_id/hevent_id. The C API keeps returning a
  hdns_t* (same raw-pointer contract as htimer_t) and hdns_cancel(hdns_t*)
  frees immediately. hdns_cb now passes the hdns_t* handle to the callback
  (like htimer_cb/hio_cb) so a completion handler can recover its id.
- evpp/Event.h + EventLoop.h: add DnsID (uint64) and a DnsID -> DnsQuery map on
  EventLoop, with resolveDns()/cancelDns() mirroring setTimer()/killTimer().
  The DnsID rides in the query's event_id; onDnsResolved erases the map entry
  before invoking the C++ callback. A stale DnsID simply misses the map, so
  cancelDns() is a safe no-op -- clients hold an id, never a raw pointer.
- evpp/TcpClient.h: hold a DnsID; resolve/cancel via EventLoop. Removes the
  per-class static onDnsResolved glue (now a lambda).
- http/client/AsyncHttpClient.{h,cpp}: use EventLoop::resolveDns; drop the
  DnsContext tracking set and its destructor cleanup (loop owns lifetime now).
- unittest/dns_lifetime_test: regression that destroys a TcpClient while a
  hostname resolve is in flight (the exact old UAF scenario); wired into
  Makefile / CMake / unittest.sh.
- update C tests/examples to the new hdns_cb signature; refresh docs/cn/hdns.md.

All DNS tests pass (hdns_test, tcpclient/asynchttp/websocket/dns_lifetime) under
Makefile and CMake; real-network resolve verified. NOTE: AddressSanitizer could
not run in this environment (even a trivial ASan binary hangs at startup), so
UAF-freedom is argued by the id-map design + the non-ASan lifetime test.
With async resolution the hostname is resolved before the socket/channel
exists. If that first resolve failed, the notification was guarded by
`if (onConnection && channel)` -- but channel was still NULL, so the callback
was silently dropped and a client without reconnect never learned the connect
failed (it looked stuck "connecting").

Fix: if no channel exists yet, create one wrapping a NULL hio_t. Channel's
methods all guard against a null io (isConnected()==false, peeraddr()=="",
write/close no-op), so the user receives a valid, non-connected channel that
matches the connect-failure contract -- without fabricating a real socket.

Also factor the shared "notify disconnect via onConnection, then reconnect if
configured" logic into notifyDisconnectThenReconnect(), used by both the
onclose path and the DNS-failure path (onDnsResolveFailed). This removes the
duplicated block and ensures the DNS-failure path also snapshots the reconnect
flag into a local BEFORE the user callback -- onConnection may delete the
client, so no member is touched afterwards (a user destroying the object must
first call setReconnect(NULL), matching the existing onclose contract).

- unittest/dns_resolvefail_test: TcpClient to an unresolvable .invalid host
  with no reconnect must still get onConnection(disconnected); the test also
  calls channel->peeraddr() to confirm a NULL-io channel is safe to use.
- wired into Makefile / CMake / unittest.sh; doc note in docs/cn/hdns.md.

All DNS tests pass (Makefile + CMake), no regression to connect/reconnect.
Let onConnection handlers tell apart *why* a connection failed (DNS resolve
failure vs. connect timeout/refused vs. other) through channel->error().

- herr.h: add ERR_DNS_RESOLVE (-1022), next to the socket syscall errors.
- Channel: add an error_ member with setError()/error(). error() returns the
  explicitly-set upper-layer error if any, else falls back to hio_error(io_),
  and is null-safe (a NULL-io channel no longer crashes error()).
- TcpClient::onDnsResolveFailed sets channel->setError(ERR_DNS_RESOLVE) before
  notifying, so a DNS failure is distinguishable from an IO-layer failure
  (which continues to surface the underlying errno/herr via hio_error).
- dns_resolvefail_test now asserts channel->error() == ERR_DNS_RESOLVE and
  prints hv_strerror() for it.
- doc note in docs/cn/hdns.md.

Usage in onConnection:
    if (!channel->isConnected()) {
        int err = channel->error();   // ERR_DNS_RESOLVE, ETIMEDOUT, ...
    }

All DNS tests pass (Makefile + CMake).
The DNS test set had grown to six files. Consolidate now that the feature is
stable:

- Merge dns_lifetime_test + dns_resolvefail_test into tcpclient_dns_test.cpp as
  three test functions (connect+reconnect, destroy-mid-resolve lifetime,
  resolve-failure notification). One binary, same coverage.
- Remove asynchttp_dns_test and websocket_dns_test: the AsyncHttpClient DNS
  path is now exercised by examples/http_client_test.cpp (URL switched to
  http://localhost:... to hit the hostname resolve path), and WebSocketClient
  inherits the exact same base-class DNS path already covered by
  tcpclient_dns_test.
- Move hdns_benchmark from examples/ to unittest/ (it is a measurement/CI aid,
  not a usage example).
- Update Makefile / CMake / scripts/unittest.sh / examples CMake and docs.

All DNS tests pass under Makefile and CMake.
Align the DNS options struct name with libhv's existing convention, which uses
singular *_setting_t (unpack_setting_t, kcp_setting_t, reconn_setting_t) rather
than *_options_t / *_settings_t.

Pure rename of the type/struct (hdns_options_s/_t -> hdns_setting_s/_t); no
field or behavior change. Updates hdns.h/.c, EventLoop::resolveDns, TcpClient,
AsyncHttpClient, tests and docs.

All DNS tests pass under Makefile and CMake.
Give the async DNS demo a short, purpose-obvious command name in the spirit of
nc / curl / wget. `host` fits its behavior (resolve a name, print the addresses
with concise output) better than `dig` (verbose, admin-oriented), and avoids
colliding with the existing `nslookup` unittest (which exercises the legacy
protocol/dns.c demo).

- git mv examples/hdns_example.c -> examples/host.c; update its header/usage.
- Makefile (EXAMPLES list + build rule), examples/CMakeLists.txt, docs.

Builds and runs under Makefile and CMake.
Copilot AI review requested due to automatic review settings July 23, 2026 11:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a native, non-blocking UDP-based async DNS resolver (hdns) that runs inside hloop, and integrates it into the C++ evpp client connect flow (and AsyncHttpClient) so hostname resolution no longer blocks the event loop.

Changes:

  • Add core async DNS resolver implementation (event/hdns.{h,c}) with caching, retry/timeout, hosts lookup, and system nameserver discovery.
  • Add C++ handle-based DNS APIs (EventLoop::resolveDns/cancelDns with DnsID) and integrate async resolution into TcpClient and AsyncHttpClient.
  • Add tests/benchmarks and wire them into Makefile/CMake/scripts; add example and documentation updates.

Reviewed changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
unittest/tcpclient_dns_test.cpp Integration tests for evpp connect/reconnect, lifetime safety, and DNS-failure notification behavior.
unittest/hdns_test.c Deterministic unit tests for hdns using a mock UDP nameserver and cache/cancel/NXDOMAIN coverage.
unittest/hdns_benchmark.c Benchmark comparing blocking getaddrinfo() vs concurrent async hdns resolves.
unittest/CMakeLists.txt Adds new DNS unit test and benchmark targets (and evpp DNS integration test under WITH_EVPP).
scripts/unittest.sh Runs the new DNS unit tests when built.
Makefile.vars Installs/exports the new public header event/hdns.h.
Makefile Builds new example and DNS tests under make unittest and adds cleanup for evpp DNS test binary.
http/client/AsyncHttpClient.h Adds internal continuation helper and clarifies teardown behavior re: in-flight DNS.
http/client/AsyncHttpClient.cpp Replaces blocking host resolution with async DNS for hostname targets.
examples/http_client_test.cpp Switches test URL host to localhost (hostname path).
examples/host.c Adds an async DNS demo example using hdns_resolve.
examples/CMakeLists.txt Builds and links the new host example.
evpp/TcpClient.h Integrates async DNS into connect/reconnect path and surfaces DNS errors via Channel::error().
evpp/EventLoop.h Adds resolveDns/cancelDns APIs with DnsID -> DnsQuery tracking.
evpp/Event.h Introduces DnsID, DnsCallback, and DnsQuery holder types.
evpp/Channel.h Adds explicit upper-layer error storage and makes error() null-safe for NULL-io channels.
event/hloop.c Frees per-loop DNS resolver state during loop cleanup.
event/hevent.h Adds dns_resolver field to hloop_t and declares hdns_resolver_free.
event/hdns.h New public C API for async DNS resolution and cancellation.
event/hdns.c New async DNS resolver implementation (wire codec, config, per-loop resolver, cache, lifecycle).
docs/PLAN.md Marks async DNS as completed.
docs/cn/README.md Adds hdns documentation link in the Chinese docs index.
docs/cn/hdns.md Adds comprehensive Chinese documentation for hdns and the C++ integration model.
cmake/vars.cmake Adds event/hdns.h to exported/installable headers list.
BUILD.bazel Adds event/hdns.h to Bazel header lists.
base/herr.h Introduces ERR_DNS_RESOLVE error code for surfacing DNS failures to users.
Comments suppressed due to low confidence (1)

event/hdns.c:993

  • hdns_cancel() will double-free if it is (incorrectly) called from within the query's own callback: hdns__deliver() sets q->node.next/prev to NULL before invoking the callback, but hdns_cancel() still HV_FREE(q) unconditionally. Adding a guard makes this misuse fail-safe instead of crashing.
void hdns_cancel(hdns_t* q) {
    if (!q) return;
    if (q->node.next) list_del(&q->node);
    if (q->timer) { htimer_del(q->timer); q->timer = NULL; }
    if (q->defer_timer) { htimer_del(q->defer_timer); q->defer_timer = NULL; }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread evpp/TcpClient.h
Comment on lines +175 to +183
}
// else: resolve failed but keep the previous remote_addr; the
// connect attempt will fail and drive the normal reconnect path.
startConnectWithAddr();
}, &opt);
if (dns_id == INVALID_DNS_ID) {
// could not start async resolve; fall back to synchronous path
return startConnectWithAddr();
}
Comment thread evpp/TcpClient.h
Comment on lines +59 to +65
// @retval =0 pending: remote_host is a hostname, so socket creation is
// deferred until the address is resolved asynchronously in
// startConnect() (avoids blocking the event loop on getaddrinfo).
// This is NOT an error; the real fd is available later via
// channel->fd(). This matches the >0/=0/<0 convention used by
// other evpp clients (see examples/protorpc).
// @retval <0 error.
Comment thread event/hdns.h
Comment on lines +24 to +27
* - Cancelable query handle.
*
* @see examples/hdns_example.c
*/
Comment thread unittest/hdns_test.c
Comment on lines +166 to +168
int main() {
hloop_t* loop = hloop_new(HLOOP_FLAG_QUIT_WHEN_NO_ACTIVE_EVENTS == 0 ? 0 : 0);

Comment thread event/hdns.c
Comment on lines +667 to +671
// Detach from the resolver and stop timers, then invoke the callback while
// the handle is still alive (the cb receives it, e.g. to read hevent_id),
// and finally free it. The query is already removed from the in-flight list
// so a re-entrant hdns_cancel(q) from the callback cannot double-free.
if (q->node.next) { list_del(&q->node); q->node.next = q->node.prev = NULL; }
- TcpClient::startResolveThenConnect: if resolveDns() fails to start on a
  hostname first-attempt (remote_addr unset), report a DNS failure via
  onDnsResolveFailed() instead of falling back to socket(0, ...) which fails
  with an unrelated socket error.
- hdns_cancel(): add a re-entrancy guard (query->delivering) so an (API-
  forbidden) cancel from within the query's own callback fails safe instead of
  double-freeing; correct the misleading "cannot double-free" comment in
  hdns__deliver accordingly.
- createsocket(): drop the misleading examples/protorpc reference in the
  return-value doc (that example returns the value as a connected fd); just
  state callers treat >=0 as success, <0 as failure.
- hdns.h: fix stale @see examples/hdns_example.c -> examples/host.c.
- hdns_test.c: replace the nonsensical hloop_new(FLAG == 0 ? 0 : 0) leftover
  with hloop_new(0).

All DNS tests pass under Makefile and CMake.
Copilot AI review requested due to automatic review settings July 23, 2026 12:23
HEVENT_FIELDS ends with HEVENT_FLAGS (destroy/active/pending, each unsigned:1).
Declare hdns_s's detached/delivering as unsigned:1 immediately after it so they
share the same storage unit instead of using two separate ints, saving space.
No behavior change.

All DNS tests pass under Makefile and CMake.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

evpp/TcpClient.h:89

  • TcpClientEventLoopTmpl::createsocket uses return 0 to mean "pending hostname resolution", but createsocket(struct sockaddr*) can legally return a real socket fd of 0 on POSIX (and SOCKET 0 can also be valid). That makes the API ambiguous and contradicts the doc comment that says @retval >0 connfd.

Consider using an explicit non-fd sentinel (e.g. a distinct negative code like -EINPROGRESS / a library-defined HV_PENDING) and updating callers/tests to treat that sentinel as success, or otherwise guaranteeing that a successfully created fd is never 0.

    // NOTE: By default, not bind local port. If necessary, you can call bind() after createsocket().
    //
    // @retval >0  connfd: the socket was created immediately (remote_host is a
    //             numeric IP, or a Unix Domain Socket when remote_port < 0).
    // @retval =0  pending: remote_host is a hostname, so socket creation is
    //             deferred until the address is resolved asynchronously in
    //             startConnect() (avoids blocking the event loop on getaddrinfo).
    //             This is NOT an error; the real fd is available later via
    //             channel->fd(). Callers should treat >=0 as success and only
    //             <0 as failure.
    // @retval <0  error.
    //
    // Rationale for the =0 case: a socket's address family (AF_INET vs AF_INET6)
    // must be known before socket() is called, but for a hostname the family is
    // only known after DNS returns an A (IPv4) or AAAA (IPv6) record. So the
    // socket cannot be created up front and is created once resolution completes
    // in startConnectWithAddr() -- the same order as the standard getaddrinfo
    // connect loop.
    int createsocket(int remote_port, const char* remote_host = "127.0.0.1") {
        this->remote_host = remote_host;
        this->remote_port = remote_port;
        memset(&remote_addr, 0, sizeof(remote_addr));
        // numeric IP (or UDS: port < 0) -> resolve now (non-blocking) and
        // create the socket immediately, returning the connfd (>0).
        if (remote_port < 0 || is_ipaddr(remote_host)) {
            int ret = sockaddr_set_ipport(&remote_addr, remote_host, remote_port);
            if (ret != 0) {
                return NABS(ret);
            }
            return createsocket(&remote_addr.sa);
        }
        // hostname -> defer socket creation to async resolution in
        // startConnect(); return 0 (pending), not an error.
        return 0;
    }

Comment thread evpp/TcpClient.h
Comment on lines +210 to +223
// @internal: notify a disconnect via onConnection, then reconnect if set.
// NOTE: onConnection is a user callback that may delete this object, so we
// snapshot whether to reconnect into a local BEFORE the callback and touch
// no members afterwards (a user destroying the object must first call
// setReconnect(NULL)). Shared by the onclose path and DNS-failure path.
void notifyDisconnectThenReconnect() {
bool reconnect = reconn_setting != NULL;
if (onConnection) {
onConnection(channel);
}
if (reconnect) {
startReconnect();
}
}
Comment thread event/hdns.c
Comment on lines +964 to +977
// 3) network query: assign sub-queries + txids, and start on the next loop
// tick so this function returns the handle before any send/callback.
// Use a per-resolver monotonic counter (not random) so concurrent
// queries never share a txid and responses demux unambiguously.
uint16_t base = r->next_txid;
r->next_txid += 2; // A uses base, AAAA uses base+1
if (q->opt.family & HDNS_QUERY_A) {
q->sub[0].qtype = HDNS_TYPE_A;
q->sub[0].txid = base;
}
if (q->opt.family & HDNS_QUERY_AAAA) {
q->sub[1].qtype = HDNS_TYPE_AAAA;
q->sub[1].txid = (uint16_t)(base + 1);
}
Copilot AI review requested due to automatic review settings July 23, 2026 12:29

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

evpp/TcpClient.h:214

  • The comment above notifyDisconnectThenReconnect() says the code will “touch no members afterwards”, but the function calls startReconnect() after onConnection returns. This is misleading and can give a false sense of lifetime safety for users who call setReconnect(NULL) or destroy the client inside onConnection.
    // @internal: notify a disconnect via onConnection, then reconnect if set.
    // NOTE: onConnection is a user callback that may delete this object, so we
    // snapshot whether to reconnect into a local BEFORE the callback and touch
    // no members afterwards (a user destroying the object must first call
    // setReconnect(NULL)). Shared by the onclose path and DNS-failure path.

evpp/TcpClient.h:88

  • Using return value 0 to mean “pending hostname resolution” collides with a valid OS socket fd value (0 is possible if stdio fds are closed), and it also breaks call sites that treat the return value as the actual connfd (e.g. examples/protorpc/protorpc_client.cpp returns that fd to callers on success). Consider using a distinct sentinel that cannot be a real fd (e.g. a negative HV_* code like -EINPROGRESS), or adding a separate async-hostname API while keeping createsocket()’s “returns real fd” contract intact.
        // hostname -> defer socket creation to async resolution in
        // startConnect(); return 0 (pending), not an error.
        return 0;

Comment thread examples/host.c
Comment on lines +1 to +10
/*
* hdns_example — asynchronous DNS resolve demo.
*
* Usage: hdns_example [domain] [domain2 ...]
* e.g. hdns_example www.example.com github.com localhost 1.1.1.1
*
* Resolves each domain asynchronously through the event loop (no blocking
* getaddrinfo) and prints the resulting addresses. Quits when all queries
* have completed.
*/
Comment thread event/hdns.c
Comment on lines +823 to +827
list_for_each(node, &r->queries) {
hdns_t* q = list_entry(node, hdns_t, node);
for (int i = 0; i < 2; ++i) {
if (!q->sub[i].qtype || q->sub[i].done) continue;
if (q->sub[i].txid != txid) continue;
Address the second round of Copilot review comments on PR #855:

- hdns.c: the 16-bit txid counter (next_txid += 2) could wrap and collide with
  a still-in-flight sub-query in a long-running loop, mis-delivering a response.
  Allocate the (base, base+1) pair by probing the (tiny) in-flight set and
  skipping any txid currently in use, so wrap can no longer alias a live query.

- TcpClient.h: notifyDisconnectThenReconnect() calls startReconnect() (a member
  call) after the user onConnection callback, so destroying the client from
  within onConnection while reconnect is enabled would be a use-after-free. This
  is pre-existing, long-standing evpp onclose behavior (this method is a verbatim
  extraction of it), not introduced here; changing it to a lifetime-safe handle
  is out of scope for this PR. Make the comment state the contract honestly
  instead of over-claiming safety: disable reconnect (or defer destroy) before
  tearing down from onConnection.

All DNS tests pass under Makefile and CMake.
Copilot AI review requested due to automatic review settings July 23, 2026 12:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

evpp/TcpClient.h:61

  • The createsocket() return-value documentation is internally inconsistent and can be ambiguous in edge cases: it says the immediate connfd is ">0" but file descriptor 0 can be a valid socket fd if stdin/stdout/stderr are closed. Since 0 is also used as the "pending DNS" sentinel, callers should not rely on "0 means pending" unless they also know the host is a hostname (and the docs should spell this out).
    // @retval >0  connfd: the socket was created immediately (remote_host is a
    //             numeric IP, or a Unix Domain Socket when remote_port < 0).
    // @retval =0  pending: remote_host is a hostname, so socket creation is
    //             deferred until the address is resolved asynchronously in
    //             startConnect() (avoids blocking the event loop on getaddrinfo).

event/hdns.c:833

  • hdns__on_udp_read demuxes purely by transaction id and does not validate that the UDP response came from the nameserver that was queried. This allows off-path/local injection of spoofed DNS replies to the resolver's UDP socket (especially when an attacker can send to the ephemeral local port). At minimum, compare the sender address/port (hio_peeraddr) against the expected nameserver before accepting the packet.
            // parse this sub-query's answer
            uint32_t ttl = HDNS_CACHE_MAX_TTL;
            int rc = hdns__parse_response(pkt, readbytes, txid,
                                          q->addrs, HDNS_MAX_ADDRS,
                                          &q->naddrs, &ttl);

Comment thread evpp/TcpClient.h Outdated
Comment on lines +148 to +151
if (!remote_host.empty() && !is_ipaddr(remote_host.c_str())) {
return startResolveThenConnect();
}
return startConnectWithAddr();
…y txid

Third round of Copilot review on PR #855:

- TcpClient::startConnect only runs async DNS when remote_port >= 0. A Unix
  Domain Socket target (remote_port < 0) carries a filesystem path in
  remote_host and is not an IP, so the previous !is_ipaddr() check wrongly
  routed it to DNS resolution and broke UDS clients; remote_addr is already set
  by createsocket() for UDS.

- hdns__on_udp_read now validates the datagram source address (hio_peeraddr)
  against the nameserver the current attempt was sent to (new hdns_s.last_ns),
  dropping off-path spoofed/injected responses that guess the txid but not the
  source. Verified the mock-nameserver unit test and real-network resolves
  still pass (legitimate replies match).

- Revert the earlier txid free-pair probing: it was over-defensive for a
  scenario (16-bit wrap colliding with a still-in-flight query) that requires
  tens of thousands of resolves on one loop with an old query still pending.
  Restore the simple monotonic counter and just document the theoretical bound.

All DNS tests pass under Makefile and CMake.
Copilot AI review requested due to automatic review settings July 23, 2026 12:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Comment thread event/hdns.h
Comment on lines +109 to +112
* The callback is NEVER invoked re-entrantly inside this call: even for
* numeric-IP / hosts / cache hits, completion is posted to the next loop
* iteration. Therefore the returned handle is always valid to pass to
* hdns_cancel() until the callback fires.
Comment thread event/hdns.c
Comment on lines +834 to +839
// Only accept a response coming from the nameserver we queried.
// This drops off-path injected packets that happen to guess the
// txid but not the source address/port.
if (src && sockaddr_compare((sockaddr_u*)src, &q->last_ns) != 0) {
return;
}
…rantee

Fourth round of Copilot review on PR #855:

- hdns__on_udp_read: when a response's source address does not match the queried
  nameserver, `continue` to the next in-flight sub-query instead of `return`ing
  from the whole handler, so one rejected/spoofed datagram cannot prevent a
  legitimate match from being processed.

- hdns__defer_complete and the network-start path previously fell back to
  synchronous completion / send if htimer_add failed, contradicting the
  "callback is NEVER invoked re-entrantly" guarantee documented in hdns.h.
  htimer_add(timeout_ms=1) never returns NULL, so that fallback was unreachable
  dead code; remove it and always defer, so the guarantee holds literally (an
  unreachable alloc failure leaves the query to be cleaned up at loop teardown).

All DNS tests pass under Makefile and CMake.
Copilot AI review requested due to automatic review settings July 23, 2026 13:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

evpp/TcpClient.h:65

  • The createsocket() return-value contract is documented as ">0 connfd" for the immediate-socket path, but socket() can legally return fd==0 (e.g., if stdin is closed). This makes the documentation incorrect and also makes "0 == pending" ambiguous on POSIX. At minimum, update the comment to not claim ">0" and warn callers not to use the numeric return value to distinguish pending vs created; consider using a distinct negative sentinel (e.g., -EINPROGRESS) if you need an unambiguous pending indicator.
    // NOTE: By default, not bind local port. If necessary, you can call bind() after createsocket().
    //
    // @retval >0  connfd: the socket was created immediately (remote_host is a
    //             numeric IP, or a Unix Domain Socket when remote_port < 0).
    // @retval =0  pending: remote_host is a hostname, so socket creation is
    //             deferred until the address is resolved asynchronously in
    //             startConnect() (avoids blocking the event loop on getaddrinfo).
    //             This is NOT an error; the real fd is available later via
    //             channel->fd(). Callers should treat >=0 as success and only
    //             <0 as failure.
    // @retval <0  error.

Comment thread evpp/TcpClient.h
Comment on lines +165 to +171
dns_id = loop_->resolveDns(remote_host.c_str(),
[this](int status, int naddrs, const sockaddr_u* addrs) {
dns_id = INVALID_DNS_ID; // this query is done
if (status == HDNS_STATUS_OK && naddrs > 0) {
// adopt the first resolved address, keep the target port
remote_addr = addrs[0];
sockaddr_set_port(&remote_addr, remote_port);
Comment thread event/hdns.h
Comment on lines +20 to +23
* - Numeric-IP fast path (no query for literal IPv4/IPv6).
* - Per-query timeout, retry and multi-nameserver rotation.
* - DNS name compression parsing and CNAME chain following.
* - TTL-respecting per-loop cache (positive + negative).
Copilot AI review requested due to automatic review settings July 23, 2026 14:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

evpp/TcpClient.h:58

  • The createsocket() return-value docs say the immediate socket fd is ">0", but ::socket() can legally return 0. Since callers are told to treat ">=0" as success, the comment should match that to avoid confusion and mis-documentation.
    // @retval >0  connfd: the socket was created immediately (remote_host is a
    //             numeric IP, or a Unix Domain Socket when remote_port < 0).

evpp/TcpClient.h:78

  • This comment says the immediate createsocket() path returns a connfd ">0", but 0 is also a valid fd from ::socket(). Update the comment to avoid implying 0 always means the "pending" (hostname) case.
        // numeric IP (or UDS: port < 0) -> resolve now (non-blocking) and
        // create the socket immediately, returning the connfd (>0).

Comment thread examples/host.c
Comment on lines +25 to +27
char ip[SOCKADDR_STRLEN] = {0};
// cast away const: sockaddr_ip only reads
sockaddr_ip((sockaddr_u*)&result->addrs[i], ip, sizeof(ip));
@ithewei
ithewei merged commit 6bcfc50 into master Jul 23, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants