[leoparente]

Summary

Follow-up to #805: problems in the dnstap TCP and Unix-socket servers found while fixing it.

Disconnected clients were never released. Each client's session was keyed by its fd and erased from the client's close handler. By then the handle is closing, uv_fileno() fails, and uvw's fd() returns an uninitialized value, so the session and the client handle it owns stayed in the map. An erase that happened to hit a live client's fd dropped that client's session instead, and its next read dereferenced a null session. Sessions are now keyed by their client handle. The stream reports its connected clients in its info as dnstap.clients, which is how the tests see a disconnected client being released.

A second Unix input took over the first one's socket. Before binding, the Unix server removed whatever was at its socket path. A second dnstap input on the same path unlinked the first server's socket and took its new clients, while the first kept running unreachable. A regular file at the path was deleted too. Now the server first checks what is at the path:

At the socket path	Before	Now
Nothing	binds	binds
A socket nobody listens on (a server crashed or stopped)	removed, binds	removed, binds
A socket a server listens on	removed, takes the path	address already in use
A regular file or other non-socket	deleted	the path exists and is not a socket, file kept
A stale socket that cannot be removed	the remove's error	unable to remove the stale socket there: <reason>
A socket the check cannot connect to, e.g. one left by a pktvisord that ran as root	removed if the directory allowed it	unable to tell whether a server listens on the socket there: <reason>, file kept

The check is a non-blocking connect: only "connection refused" means stale. The last row is a behaviour change: a root-owned socket that a non-root pktvisord cannot connect to looks the same whether a server is behind it or not, so it is kept and the error says why. Remove it by hand if nothing uses it.

Clients were dropped when a frame spanned two reads. The frame-stream session threw, and the client was dropped, whenever a socket read ended inside a frame's 4-byte length, or inside a control frame after its escape code had been consumed. Reads can end anywhere, so a busy client was eventually dropped. The session now waits for the rest of the frame, and consumes a control frame only once all of it has arrived.

A new session could inherit the previous one's state. _is_bidir had no initial value. Now that a disconnected client's session is actually freed, the next session can be built in the same memory: on glibc it then started as bidirectional, and a unidirectional client's START was refused as out of order. It now starts false, as in WinFrameSession.h.

Tests

All 25 ctest targets pass on macOS (Release). The new dnstap tests:

• Sessions: a TCP server reports its connected clients; clients that disconnect are released while one that stays is kept. The release test failed on develop: the count only grew.
• Socket path: a socket another server listens on is refused, and the path is still that server's socket, by inode; a datagram server's socket is refused; a path that is not a socket is refused and left alone; a socket left by a stream or datagram server that stopped is replaced; a path that fills sun_path is handled; a stale socket that cannot be removed, and one that cannot be checked, are reported. The last two skip as root, so the Linux CI job skips them and the macOS job runs them.
• Frame stream: a bidirectional stream fed one byte at a time delivers all its frames, which failed on develop at the first byte; a unidirectional session built in memory a bidirectional one used starts clean, which failed on develop.

All of them except the two stale-socket replacements, which keep develop's behaviour, failed on develop or before their fix. The two client-count tests use ports 15355 and 15356, because 5355 is LLMNR, which systemd-resolved can hold.

Not changed here

• On macOS, a live server whose backlog is full refuses connections like a stale one, so a second input would remove its socket. Linux returns EAGAIN, which is treated as live.
• Two inputs starting within the same microseconds can still race on a stale socket.
• Older frame-stream problems: a bidirectional STOP never gets a FINISH back, so an fstrm writer waits on close; _decode_control_frame leaks its fstrm_control on its error paths; erasing the front of the buffer after every frame is quadratic for reads full of small frames.
• The note in fix(inputs): fail a flow or dnstap listener that cannot bind instead of running without it #805 that a failed accept() stops the server was wrong: in libuv 1.48 the listening socket stays watched after a failed uv_accept. Only a client handle that cannot be created stops it, and uv_tcp_init/uv_pipe_init do not fail in practice.

🤖 Generated with Claude Code

[leoparente]

@codex review

[leoparente]

/ai-review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-29T22:06:50.912867Z	afc246e	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[nbl-ai-review]

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net-positive fix: resolves the stale-session/fd-reuse bug, the Unix-socket-takeover bug, and the frame-reassembly drops, backed by tests that fail on develop and pass here. Two low-severity robustness notes below, neither blocking.

Findings

• [Low] [correctness] probe_unix_socket ignores fcntl's return value — src/inputs/dnstap/DnstapInputStream.cpp:52 — ::fcntl(fd, F_SETFL, O_NONBLOCK)'s result is discarded; if it fails, fd stays blocking, so connect() to a live server with a full backlog can hang start() instead of returning the intended "live" signal. Check the return value and treat a failed fcntl the same as the existing "unable to tell" probe-failure path.
• [Low] [security] Check-then-bind race on the Unix socket path (author-disclosed) — src/inputs/dnstap/DnstapInputStream.cpp:487 — probe_unix_socket()/remove() run before uvw's bind, so two inputs racing to start on the same path within the same window can both pass the stale check. The PR description already calls this out under "Not changed here" as an accepted limitation given the trusted, co-located deployment model, so no action is required now; worth revisiting with an atomic bind-or-fail primitive if the socket path is ever derived from less-trusted config.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 2 comment(s) · 0 prior AI review(s) · usage: $1.32 · 1769k in (91% cached) · 18.1k out}

[nbl-ai-review]

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net-positive fix: client sessions are now keyed by handle instead of fd, the Unix-socket takeover bug is closed with a proper stale-socket probe, frame-stream reads correctly await partial frames, and _is_bidir gets a safe initial value. No new issues found.

Resolved since last review

• src/inputs/dnstap/DnstapInputStream.cpp:52 — probe_unix_socket ignoring fcntl's return value is fixed: a failed fcntl now closes the socket and routes through the existing "unable to tell whether a server listens on the socket there" probe-failure path instead of proceeding with a blocking socket.

Previously raised — still open

• None.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 10 comment(s) · 2 prior AI review(s) · 1/1 thread(s) resolved · usage: $1.1 · 1506k in (92% cached) · 15k out}

Previous review · 2026-10-29 · 40eab19

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net-positive fix: client sessions are now keyed by handle instead of fd (closing the stale-session/null-deref bug), the Unix-socket takeover is closed with a proper stale-socket probe, and frame-stream reads correctly await partial frames. One low-severity robustness gap remains in the new probe.

Findings

• [Low] [correctness] probe_unix_socket ignores fcntl's return value — src/inputs/dnstap/DnstapInputStream.cpp:52 — ::fcntl(fd, F_SETFL, O_NONBLOCK)'s result is discarded. If it fails, fd stays blocking, so a subsequent connect() to a live server whose accept backlog is full can block indefinitely in start() instead of returning EAGAIN/EWOULDBLOCK as the surrounding logic assumes. Check the return value and, on failure, route through the existing "unable to tell whether a server listens on the socket there" probe-failure path instead of proceeding with a blocking socket.

Previously raised — still open

• None. The only other finding from the prior review (TOCTOU window between symlink_status/probe/remove() on the Unix socket path, DnstapInputStream.cpp:489) was explicitly declined by the author in the PR conversation: the window predates this change (narrower than develop's unconditional remove()), a vanished file resolves cleanly via ENOENT, and fully closing it would require a lock file alongside the socket. Not re-raised; would need to be restated if the socket path were ever derived from a less-trusted, non-operator-controlled directory.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 6 comment(s) · 2 prior AI review(s) · 1/1 thread(s) resolved · usage: $1.14 · 1556k in (93% cached) · 20.3k out}

Previous review · 2026-10-29 · 40eab19

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net-positive fix: client sessions are now keyed by handle instead of fd, the Unix-socket takeover bug is closed with a proper stale-socket probe, frame-stream reads correctly await partial frames, and _is_bidir gets a safe initial value. No correctness issues found; one low-severity, low-likelihood security note.

Findings

• [Low] [security] TOCTOU window between stat/probe and remove() of the Unix socket path — src/inputs/dnstap/DnstapInputStream.cpp:489 — symlink_status, probe_unix_socket, and std::filesystem::remove each independently re-resolve socket_path (lines 483-498). If another local principal can write to the socket's parent directory, it could swap the path's contents between these calls (CWE-367), potentially defeating the new "already in use" protection. remove() unlinks by name rather than following a symlink to arbitrary file content, so this cannot be escalated into deleting unrelated files. Not actionable unless the socket directory can be non-operator-controlled; if so, worth a one-line comment documenting the trust assumption.

Nits — none beyond the above.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 3 comment(s) · 0 prior AI review(s) · usage: $0.81 · 982k in (92% cached) · 17k out}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 40eab1911c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[leoparente]

On the AI review's finding "TOCTOU window between stat/probe and remove() of the Unix socket path": declined.

The window is the few microseconds between symlink_status, the probe, remove and the bind. The race needs two dnstap inputs on the same path starting at the same moment, or a socket file being replaced in that window. Closing it fully would take a lock file next to the socket. On develop the remove was unconditional, so this window is narrower than before, not new. A file that vanishes in the window is handled: the probe's ENOENT goes on to the bind.

[leoparente]

@codex review

[leoparente]

/ai-review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 40eab1911c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[leoparente]

On the AI review's finding "probe_unix_socket ignores fcntl's return value, risking a blocking connect() hang": fixed in afc246e. If fcntl(F_SETFL, O_NONBLOCK) fails, the probe now closes its socket and returns that errno. The start then fails with "unable to tell whether a server listens on the socket there: " and the file is kept, instead of connecting on a blocking socket. There is no test, because fcntl cannot be made to fail on a fresh socket. All 25 ctest targets pass.

[leoparente]

@codex review

[leoparente]

/ai-review

[nbl-ai-review]

🔁 AI Code Review updated for afc246ed — see the review · 1 resolved · 0 open (run)

[github-actions]

LCOV of commit afc246e during Debug Builds #268

  lines......: 86.2% (18578 of 21554 lines)
  functions..: 76.4% (1870 of 2448 functions)
  branches...: no data found

Files changed coverage rate: n/a

Full coverage report

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 👍

Reviewed commit: afc246edd8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".