Conversation
added 7 commits
September 1, 2026 00:44
read_rr iterated range(d.count + d.rrsig_count) over an rrset's rdata. The addresses occupy the first d.count entries; the remaining d.rrsig_count are signatures. Taking the last 4 or 16 bytes of a signature yields bytes that pass inet_ntop and, being globally routable more often than not, pass ingress validation too and are installed into the PF table. The installer configures "module-config: validator python iterator", so every DNSSEC-signed answer leaked one address into the egress whitelist, chosen by whoever signed the zone. Iterate the address records only.
Four ways the client mishandled a firewall that was not answering. udp_transmit assigned msg only inside its retry loop, then logged it after, so UDP_RETRY: 0 raised NameError inside Unbound rather than simply sending nothing. udp_transmit_close never told the circuit breaker what happened, so in UDP mode the breaker never opened and every query kept paying the full ACK wait against a dead firewall. That is the stall the breaker exists to prevent. tcp_transmit_close continued into conn.recv() after connect or sendall had already failed, so the log showed a timeout waiting for an ACK that was never going to come and buried the actual connection error. The unknown-event path called logger(qstate), which dereferences qstate.return_msg.rep with no None check. The AttributeError propagated out of operate() before ext_state was set, so Unbound never learned the module had failed. The dump is now best effort and MODULE_ERROR is set regardless.
The sync functions delete on "present in the store, absent from Redis", but the Redis snapshot was taken in run() before the PF table and persist file were read. Any IP whitelisted in that window looked orphaned and was deleted from both, despite a live Redis key, blackholing the domain until it was resolved again - up to SCAN_PERIOD, 300s by default. Each sync now takes its own snapshot, and reads the store it may delete from before reading Redis. The reverse ordering can only retain an expired entry for one extra cycle, which is harmless; the previous ordering could drop a valid one. Neither sync distinguished a failed read from an empty store: a pfctl or Redis error was logged and then diffed against, so a partial read could delete live entries. They now skip the cycle instead. The scan loop wrapped every cycle in one try, so a single transient Redis error ended expiry for that address family until the daemon was restarted, and PF tables grew without bound. The loop now survives a failed cycle and retries at the next SCAN_PERIOD; only stop_event ends it. The TCP accept loop caught only socket_timeout, so ECONNABORTED from a peer going away, or EMFILE under fd pressure, escaped run() and killed the daemon. The UDP branch already handled both. ScanSync.join() advertised a 30s timeout and then called super().join() with no argument, so stop could block indefinitely on a wedged thread.
… datagrams IOCTL returned pfrio_nadd for every command, but the kernel counts adds and deletes in separate fields, so table_pop reported 0 on a successful delete. Its documented contract is a count with 0 meaning failure, so a working delete was indistinguishable from a failed one. The OpenBSD-gated test asserted "deleted >= 0", which documented the bug rather than catching it. Moving ACKDATA after validation stopped the listener answering an unvalidated datagram, but every refusal path still sent its reason string to the same unverified source address, so the reflector remained through a slightly narrower door. Over UDP only ACKUPDATE is now sent; a refused datagram gets silence. UDP's message-size ceiling is made explicit rather than silent. The receive buffer was 1400 bytes, so a larger datagram was truncated by the kernel, failed to decode, and the answer was simply never whitelisted. The loop now reads one byte past the ceiling and logs a rejection naming the cause and the remedy. Raising the buffer is deliberately not the fix: a datagram above the link MTU fragments and PF commonly drops fragments, so UDP cannot carry the large answers TCP handles via MAX_MESSAGE. The ceiling is documented in PROTOCOL.md and DECISIONS.md, including that it applies to the encoded message rather than the DNS answer, which is why it is reachable at around 20 uncompressed records.
One PFUI message carries one reply, so every address in it answers the same query name. read_rr stamped the identical string onto every record, which for a 24-address CDN answer meant sending it 24 times: 2021 bytes uncompressed against 940 with the field at message level. Compression hid nearly all of it on the wire (262 against 261), so the cost fell on uncompressed deployments and on the structure implying the values could differ per address, which they never could. Breaking, like the framing and kind changes before it, so client and server must be deployed together. PROTOCOL.md is now version 2, the message vectors are regenerated, and validate.extract returns (ip, ttl) with the server reading qname from the message once and storing it against each key as before, so the Redis hashes are unchanged.
Moving qname to the message level invites a reading it must not have: that a message is a container several replies could be merged into. Batching would look like a bandwidth optimisation and would destroy the property the design rests on, which is that an address reaches the PF table in the microseconds before the client connects to it. State it normatively instead. A client MUST send as soon as it has the reply's records, MUST NOT wait, batch or hold records back, and MUST NOT mix query names in one message - which is what makes a single message-level qname correct rather than merely convenient. Several addresses in one message means one reply that carried several records. The implementation already behaves this way: both read_rr call sites transmit on the next line and no state accumulates between replies. DECISIONS.md records that, and notes the one place work can wait - the server's bounded receiver pool, which delays a message that has already arrived and never delays sending one.
…ffers accept() does not carry over the listener's settings: Python returns the new socket in blocking mode with timeout None. The listener's SOCKET_TIMEOUT existed only to poll for SIGTERM, so every accepted connection had no timeout at all, recv() could block forever, and the socket_timeout branch in the read path was unreachable code. A peer that connected and sent nothing held a receiver slot permanently; twice MAX_WORKERS of those wedge the daemon into shedding everything, with no authentication needed to do it. Per-connection options move into _prepare_conn, which sets the timeout and also sets TCP_NODELAY on the accepted socket rather than trusting inheritance from the listener. That inheritance holds on Linux, where it was measured, but TCP_NODELAY lives in the tcpcb rather than the socket options on BSD and this is one line. SO_SNDBUF 0 does not mean "send immediately", which is what its comment claimed and what TCP_NODELAY actually does. The kernel clamps it to a minimum (4608 bytes when measured), and a small send buffer costs syscalls and blocking on a large message. It was also being set on a listening socket, where it does nothing. Removed from both ends, along with a 36 byte send buffer on the UDP listener sized for an ACK. SOCKET_BACKLOG defaulted to 5. With a connection per answer, a burst overflows the accept queue, the SYN is dropped, and the resolver waits for a retransmit - seconds, on the DNS critical path. Now 128. SOCKET_BUFFER's comment called it a maximum message length; it is the read chunk size, and the protocol sets the message ceiling. DECISIONS.md records why connection-per-query stays: a pythonmod plugin has no process of its own to hold a socket across queries, so a persistent connection needs a separate client daemon with the plugin talking to it over local IPC. Parallelising sends to multiple firewalls is restored as a TODO, since whether a thread per firewall per query beats serial round trips wants measuring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Perf and socket fixes