diff --git a/docs/leak-findings.md b/docs/leak-findings.md new file mode 100644 index 000000000..752a38bcd --- /dev/null +++ b/docs/leak-findings.md @@ -0,0 +1,162 @@ +# SMP server leak findings + +From `bench/MemBench.hs` extended with a proxy plus relay topology and a transport that adds +latency and drops replies. + +Two leaks on the proxy path, both client reachable. Two related bugs. TLS/TCP stack clean. + +--- + +## Leak 1: forwarded commands never removed on timeout + +### Issue + +Entries go into `sentCommands` in `mkTransmission_` (`Client.hs:1418`). The only removal is in +`processMsg` (`Client.hs:706`), which runs when a reply arrives. `getResponse` (`Client.hs:1383`) +handles the timeout but does not receive the map, so it cannot delete. + +The session does not drop either: that needs 15 minutes of total silence, and the proxy never +pings. Each entry holds the forwarded command, 16226 bytes. + +`proxytmo 448`: `proxy_sentCommands` goes 64, 128, 192, 256, 320, 384, 448. Monotonic. +About 20 KiB per entry. + +### Impact + +20 KiB per unanswered forward, held for the life of the relay session (days). + +`PRXY` is unauthenticated unless `newQueueBasicAuth` is set (`Server.hs:1534`), and it names an +arbitrary destination. A client can supply a relay that accepts and stays silent. Answering some +requests and dropping others keeps the session alive, since any reply resets the drop counters. + +100k stuck commands is 2 GiB. No rate cap, see Bug 3. + +Also fires with no attacker: any round trip above 30 seconds. + +### Fix + +```haskell +Nothing -> do + TM.delete corrId sentCommands -- new + modifyTVar' timeoutErrorCount (+ 1) $> Left PCEResponseTimeout +``` + +Pass `sentCommands` and the request's `corrId` into `getResponse`. Double delete is harmless. + +Same leak with no timeout at `Client.hs:1366` and `1368`, where the request is inserted before +an early error return. + +--- + +## Leak 2: failed relay connects never cleared + +### Issue + +A failed connect is cached in `smpClients` as `Left (error, expiry)`, removed only on a later +lookup of the same server (`Client/Agent.hs:250`, `:411`). Nothing sweeps on a timer. + +The address comes from the client via `PRXY`. Host, port and key hash are arbitrary, so distinct +keys are effectively unlimited. + +`proxychurn 300`: `proxy_smpClients = 300`, none removed. A 1000 run settles at ~19 KiB per +entry, created in about 1 second. + +### Impact + +19 KiB per address, never freed while the process runs. + +About 19 MiB/s when the address refuses immediately. An address that blackholes instead waits +out the 45 second connect timeout, which throttles it heavily. + +Same unauthenticated `PRXY` as Leak 1. + +### Fix + +Sweep the map on a timer, dropping entries past their expiry. The timestamp is already stored. + +--- + +## Bug 3: proxy concurrency limit is inert + +### Issue + +`Server.hs:1590`: + +```haskell +bracket_ wait signal . forkClient clnt label $ action +``` + +`.` binds tighter than `$`, so `signal` runs when the thread starts, not when it finishes. Only +forking is limited. + +### Impact + +No memory cost. Removes the cap on how fast Leak 1 grows, and `procThreads` reads near zero at +any load. + +### Fix + +```haskell +wait >> forkClient clnt label (action `finally` signal) +``` + +This enables the limit for the first time. Default is 32, and `wait` blocks the client's whole +command loop when hit, so check the value first. + +--- + +## Bug 4: stale endThreads entry when a command finishes fast + +### Issue + +`forkClient` (`Server.hs:1480`) registers the thread after `forkIO`. If the action finishes +first, its delete misses and the insert is never undone. + +100k forks: 20% stale at `-N1`, 12% at `-N4`, 0% when the action blocks 1ms. Real callers +(`PFWD`, `PRXY`, `RSLV`) wait on the network. Reachable at speed via an oversized `PFWD` that +fails the block size check without IO. + +### Impact + +About 320 bytes per entry, freed on disconnect. 10k fast failing commands on one connection is +roughly 640 KB. Minor. The real cost is that `endThreads` no longer distinguishes stuck commands +from counter error. + +### Fix + +```haskell +atomically $ modifyTVar' endThreads $ IM.insert tId Nothing -- before forkIO +atomically $ modifyTVar' endThreads $ IM.adjust (const (Just w)) tId +``` + +`adjust` is a no-op if the action already removed the key. + +--- + +## Clean + +200 connections opened at once, closed, then measured again: + +| test | peak per conn | after 25s | +| ------------------------------------- | ------------- | --------- | +| TCP connect, never start TLS | 48.2 KiB | 0.31 KiB | +| TLS done, no SMP handshake | 203.1 KiB | 0.71 KiB | +| Handshake done, one byte, then quiet | 264.6 KiB | 0.87 KiB | + +All recovered. Also clean: 400 connect/disconnect rounds, and steady forwarding at 50ms each way. + +At +5s the middle two still read ~120 KiB per connection, which looks like a 24 MiB leak but is +`gracefulClose` waiting up to 5s per connection. Falling means reclaimed, flat above baseline +means leaked. + +Peaks still matter. 200 abandoned half open connections hold ~40 MiB for ~25s, unauthenticated. +A client that finishes the handshake then sends one byte holds ~265 KiB for as long as it stays +connected: no read timeout, `transportTimeout` is hardcoded `Nothing` (`Transport/Server.hs:104`). + +## Not reproduced + +Empty session variable leak. After 100 rounds `proxysess` ends with `proxy_smpClients=0` and +`proxy_smpSessions=0`, checked before and after the 45 second connect timeout. `bracketOnError` +in `withGetSessVar` drops the empty entry. + +The ~108 KiB per round it shows is harness overhead, not a server finding.