AdguardTeam / AdguardTeam/dnsproxy
udpPacketLoop abandons its socket on a read error — UDP listener dies permanently, without closing the socket and (for net.ErrClosed) without a log
- Vorherrschende Sprache
- Go
- Sterne
- 3.3k
- Forks
- 343
- PR-Merge-Kennzahlen
- Keine gemergten PRs in 30 T.
Beschreibung
## Summary
`Proxy.udpPacketLoop` exits the read loop on **any** read error and never closes
the socket. The goroutine is gone, nothing restarts it, and `serveListeners`
spawns each loop exactly once. The socket stays bound. From that moment the
proxy accepts no further UDP queries on it, and for one of the two error classes
it says nothing at all.
Verified present in **v0.84.2** (latest at time of writing) and in v0.81.4.
## The code
`proxy/serverudp.go`:
```go
for p.isStarted() {
n, localIP, remoteAddr, err := proxynetutil.UDPRead(conn, b, p.udpOOBSize)
if n > 0 {
// ...
}
if err != nil {
logUDPConnError(err, conn, p.logger)
break // <- loop ends here, forever; conn is never closed
}
}
```
```go
func logUDPConnError(err error, conn *net.UDPConn, l *slog.Logger) {
if errors.Is(err, net.ErrClosed) {
l.Debug("udp connection closed", "addr", conn.LocalAddr()) // <- invisible by default
} else {
l.Error("reading from udp", slogutil.KeyError, err)
}
}
```
## Why this is worse than it looks
Three properties compound:
1. **No recovery.** `serveListeners` starts one goroutine per listener and there
is no supervision. A loop that returns is not replaced for the process
lifetime.
2. **The socket is not closed.** It remains bound, so the kernel keeps queueing
datagrams into a receive buffer that no longer has a reader. Once the buffer
fills, every subsequent datagram is dropped in the kernel. The client sees
silence and retries until it times out.
3. **It can be completely silent.** The `net.ErrClosed` branch logs at **Debug**,
which is off by default (AdGuardHome ships `verbose: false`). Nothing in
`serverudp.go` increments a counter or exports a metric, so no health check,
dashboard or log can distinguish "listener dead" from "quiet period".
The result is a daemon that reports itself healthy while serving no UDP DNS.
## Impact on AdGuardHome
For a typical AdGuardHome install, plain UDP `:53` is **the primary path** — it is
what every LAN client uses. If that listener dies, DNS stops for the whole
network, while the web UI, DoH, DoT and DoQ all keep working and the service
stays `active (running)`. The user sees "the internet is broken", the dashboard
shows nothing wrong, and a restart fixes it with no explanation in the log.
## How we found it
We run a fork that opens several `SO_REUSEPORT` sockets per listen address
instead of one, so the loops shard across cores. That turned a total outage into
a *partial* one and made it survive for days: three of four listeners were dead,
so roughly three quarters of plain-DNS queries were black-holed while a quarter
still answered. Encrypted transports were unaffected throughout.
Measured on the live resolver before the restart:
- `bpftrace -e 'tracepoint:syscalls:sys_enter_recvmsg /pid==$AGH/ { @fd[args->fd] = count(); }'`
showed reads on **one** fd; the other three listening fds never appeared.
- `ss -lunpm 'sport = :53'` showed three sockets with `Recv-Q` pinned at exactly
`rb` (8 MiB) and their `d=` drop counters climbing, while the fourth sat at 0.
- `/proc/net/snmp`: `RcvbufErrors` 1,332,054 of 2,251,838 `InDatagrams` (**37%**).
- `dig @127.0.0.1` answered **4 of 12**.
- Nothing whatsoever in the journal — no `reading from udp`, no
`acquiring semaphore`, no listener lifecycle line after startup.
Note the single-socket upstream case is *louder* than ours (all UDP stops at
once) but equally unexplained, and equally unrecoverable without a restart.
## What we could not determine
**We cannot say which error triggered it**, because nothing was logged — which is
itself the core of this report. What we can rule out:
- Not semaphore exhaustion. `requestsSema` is shared across all loops, and the
surviving loop kept acquiring and releasing slots throughout, so slots were
available. The `break` after `reqSema.Acquire` also logs at Error, and no such
line exists.
- Not shutdown. `p.isStarted()` is the same flag for every loop and the surviving
loop kept running.
- Not a dropped log line. `journalctl` showed no rate-limiting or suppression for
the unit.
That leaves the read-error `break`, most likely via the `net.ErrClosed` branch
that logs at Debug. A user cannot be expected to diagnose this, which is the
point: **the failure mode is unobservable by construction.**
## Suggested fix
Distinguish the error classes, and make a listener's death both survivable and
visible:
- **Timeouts** (`net.Error` with `Timeout()`) — continue; they are not fatal to
the socket.
- **`net.ErrClosed`** — return; that is shutdown closing us deliberately.
- **Anything else** — `Close()` the socket and reopen a replacement on the same
address. Closing matters independently of the reopen: it removes the dead
socket from the `SO_REUSEPORT` group so the kernel stops delivering datagrams
to a reader that will never come. With a single socket it at least surfaces the
failure as a connection refusal rather than a silent black hole.
And regardless of the recovery policy: **log the exit at Error unconditionally
and count it.** A path that stops serving queries should never be quiet.
We implemented exactly this in our fork if it is useful as a starting point —
a pure `udpLoopActionFor(err, started) -> continue|retire|stop` predicate with a
table test, plus `retireUDPListener` / `swapUDPListener`:
https://github.com/Ozy-666/dnsproxy/commit/2aa166a
Happy to open a PR against `master` if you would like it in this shape.
## Reproduction
We did not reproduce the *triggering* error — it occurred naturally after ~17h of
production traffic. The consequence, however, is reproducible directly and is
what makes the bug severe: a bound UDP socket with no reader silently discards
everything once its receive buffer fills.
```bash
# bind, never read, watch the kernel drop
python3 - <<'EOF' &
import socket, time
s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
s.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 4096)
s.bind(("127.0.0.1", 15353))
time.sleep(30)
EOF
sleep 1
python3 -c 'import socket;s=socket.socket(socket.AF_INET,socket.SOCK_DGRAM);[s.sendto(b"x"*512,("127.0.0.1",15353)) for _ in range(50000)]'
ss -lunpm 'sport = :15353' # Recv-Q pinned at rb, d= climbing
```
To exercise the code path itself, injecting an error from `proxynetutil.UDPRead`
is enough to show the loop never comes back and the socket is never closed.
Beitragsleitfaden
Für dieses Repository ist kein Beitragsleitfaden indexiert
Bewertung
Dieses Issue wurde noch nicht bewertet.