Commit 974b431
committed
Fix: Put stores pooled connection after shutdown starts
=== Classification
Race condition; severity: medium; confidence: certain.
=== Affected Locations
- `dnscrypt-proxy/udp_conn_pool.go:94`
- `dnscrypt-proxy/udp_conn_pool.go:132`
- `dnscrypt-proxy/proxy.go:636`
- `dnscrypt-proxy/main.go:145`
=== Summary
`Put(addr, conn)` checks `closed` before acquiring the shard lock, then appends to `shard.conns[addrStr]`. If `Close()` sets `closed=1` and drains the shard while `Put()` is blocked, `Put()` later resumes and stores a live connection after shutdown cleanup has completed. This violates the pool's closed-and-drained invariant and leaves a reusable UDP socket open after `Close()` returns.
=== Provenance
Reproduced from a verified finding and confirmed with a focused local Go test. The finding originated from Swival Security Scanner (https://swival.dev).
=== Preconditions
`Put()` runs concurrently with `Close()` on the same UDP connection pool.
=== Proof
A caller-controlled `conn` reaches `Put(addr, conn)`. `Put()` reads `closed==0` before taking the shard lock. Concurrently, `Close()` stores `closed=1`, locks shards, closes pooled descriptors, and deletes shard entries. If `Put()` is delayed until after `Close()` finishes that shard, it resumes, never rechecks `closed`, and appends a new `pooledConn` at `dnscrypt-proxy/udp_conn_pool.go:132`.
This was reproduced with a focused Go test using an address whose `String()` call was intentionally slow to widen the interleaving window. The test observed that:
- `Close()` returned
- `Stats()` reported a remaining pooled connection
- the inserted `*net.UDPConn` remained writable, proving it was not closed
The path is reachable in normal operation because request handling returns sockets through `proxy.udpConnPool.Put(...)` at `dnscrypt-proxy/proxy.go:636`, while shutdown invokes `udpConnPool.Close()` at `dnscrypt-proxy/main.go:145`.
=== Why This Is A Real Bug
The race leaves a post-shutdown socket resident in the pool after `Close()` has completed. That causes a resource leak and permits reuse of a connection from an object that callers reasonably treat as closed. The reproduced state directly contradicts the shutdown contract enforced by `Close()`.
=== Fix Requirement
Recheck the pool `closed` state while holding the shard lock in `Put()`. If shutdown has started, do not append; close the incoming `conn` instead.
=== Patch Rationale
The patch closes the TOCTOU gap by validating `closed` under the same shard lock that protects `shard.conns`. This makes `Put()` and `Close()` mutually consistent for the append/drain decision, ensuring no connection can be inserted after shutdown begins. Closing the rejected `conn` preserves the pool's drained invariant and avoids leaking the descriptor.
=== Residual Risk
None.
=== Patch
Patched in `012-put-can-store-a-connection-after-shutdown-starts.patch`.1 parent f3615b5 commit 974b431
1 file changed
+5
-0
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
123 | 123 | | |
124 | 124 | | |
125 | 125 | | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
126 | 131 | | |
127 | 132 | | |
128 | 133 | | |
| |||
0 commit comments