Publish a handover connection only after releasing its mutex

[kwsantiago]

Fixes a use-after-free of a connection's mutex that aborts the relay. Pre-existing and identical upstream; unchanged by the recent handover-list work.

The bug

swapList runs on a thread-pool thread and holds http_conn._mut across its whole body, including the final handover_list.insert. That insert is the publish: it makes the connection visible to the event-loop thread, which can snapshot the handover list and release the connection (releaseHandover returns the HTTPConn to its pool or destroys it) at any moment afterwards.

handover_list.insert releases the list mutex before swapList's deferred _mut.unlock runs. So:

worker:  handover_list.insert(conn) returns, list mutex released
loop:    snapshot sees conn, releaseHandover pools/destroys http_conn
worker:  http_conn._mut.unlock(io)      <- writes into an object it no longer owns

Nothing synchronizes those: processSignal never takes _mut. Reaching the window needs the loop already awake on some other connection's signal, since this connection's own signal is sent after swapList returns. With several thread-pool threads that is routine.

Reproduced, not inferred

Compiled a probe into HTTPConnPool.release that reads conn._mut.state and reports when the pool takes ownership of a still-locked mutex, then widened the window with a 2ms delay between the insert and the unlock to make the interleaving reliable. Load was 2000 interleaved WebSocket upgrades and Connection: close requests.

	probe hits	panics	relay
before	24	yes	aborted, core dumped
after	0	0	alive

The panic lands exactly where the analysis predicts, in Mutex.unlock:

thread N panic: switch on corrupt value
  std/Io.zig:1642 in swapList          <- switch (m.state.swap(.unlocked, .release))
  vendor/httpz/src/worker.zig:956 in processHTTPData
      self.swapList(conn, .handover);

"switch on corrupt value" is the mutex memory no longer holding a valid State, because the HTTPConn was recycled underneath it. In ReleaseSafe that is an abort, so a remote peer driving handover churn can restart the relay; in ReleaseFast (the release build) there is no check and it is a silent write into recycled memory.

Honest limit on this evidence: the 2ms delay is what makes it reproducible. It proves the mechanism and the consequence, not the natural rate. An earlier run without the delay produced zero hits, but its load generator was too slow to create meaningful handover churn, so that is not evidence of rarity either.

The fix

Publishing is now the last thing that touches the connection, and happens after _mut is released. The locked region moves into swapListLocked; swapList releases the mutex and only then inserts into handover_list. The caller's following loop.signal() deliberately touches neither conn nor http_conn, so it stays safe.

The gap this opens, between the unlock and the insert, is harmless: the connection is in no list and its state is already .handover, which is exactly what run()'s .recv handler checks for and skips.

Also here

Corrects a comment I misplaced in #181. It was meant for disown() but a first-match replacement put it on swapList, where its reasoning ("both callers: run()'s parse-error path and accept()'s errdefer") describes disown's callers, not swapList's. Both switches now carry the reasoning that actually applies to them.

Verification

zig build test 65/65, handover-leak test 6/6, protocol 45/45, concurrency 2/2 at 200 connections, verify-vendored-httpz.sh clean, zero panics or corruption in any relay log.

Review follow-up: the first fix was incomplete

Both reviews independently found that moving the handover publish fixed only one of two paths, and my own repro confirmed it. swapList also publishes .keepalive (and .request) from inside the critical section, and the event-loop thread reaches those through disown() and closeList(), neither of which takes _mut. So the same use-after-free survived.

Reproduced it the same way, with http_keepalive_timeout_s = 0 so every keepalive conn expires on insert: 57 probe hits and the relay aborted, same panic, now from the .keepalive call site.

Hoisting the keepalive insert the way the handover one was hoisted is not a valid fix, and both reviews said so independently. run()'s .recv skips a .handover conn but not a .keepalive one, so a gap with .keepalive state and no list membership would let a pipelined request drive keepalive_list.remove on a non-member, nulling head and tail and wiping the live list. That trades a rare use-after-free for a reachable list wipe.

So the wait goes where every free funnels through: HTTPConnPool.release takes and releases _mut before recycling, which makes it impossible for a call site to miss. I know that matters because I first fenced only closeList and the relay still crashed through disown.

Verified with both windows widened and mixed load including pipelined parse errors, which is what drives the disown path:

	violations	panics	relay
fence in closeList only	5	1	aborted
fence in release()	403 caught and waited on	0	alive

The 403 is the fence doing its job under a deliberately widened window; it is logged at debug rather than error, since the wait makes it an expected and handled interleaving rather than a fault.

One result in this series was invalid and I am not counting it: an intermediate run reported a crash, but the build had failed on a shadowed io binding and the test had silently run the previous binary. The runs above are gated on the build succeeding.

Also corrected here, per both reviews: my "degrades to a stale list entry" rationale on the impossible .handover arms was false. List.remove on a non-member rewrites the live list's head and tail from stale links, which is the connection-loss and core-pinning bug fixed in #181, not a benign stale entry. Those arms now log and touch no list, which is what actually degrades safely.

Re-verified on the final tree: zig build test 65/65, handover-leak 6/6, protocol 45/45, concurrency 2/2, vendor integrity clean, zero panics.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a4fbb37c-f52f-49ad-9005-105c1fcd0d73

📥 Commits

Reviewing files that changed from the base of the PR and between 42ef4fb and da3f305.

📒 Files selected for processing (2)

• vendor/httpz.patch
• vendor/httpz/src/worker.zig

---

Walkthrough

The patch adds public connection-count APIs and changes worker handover synchronization. It also adds dedicated detached HTTP and WebSocket cleanup paths, with guarded slot release during normal cleanup and shutdown.

Changes

Connection lifecycle

Layer / File(s)	Summary
Handover synchronization vendor/httpz.patch, vendor/httpz/src/worker.zig	Connection list mutations remain mutex-protected. Handover publication now occurs after the mutex is released.
WebSocket and detached cleanup vendor/httpz.patch, vendor/httpz/src/worker.zig	Detached HTTP connections use releaseHandover. WebSocket teardown transfers wrappers to the event-loop thread and cleans failed registrations.
Slot accounting and public counts vendor/httpz.patch	Slot release uses guarded releaseSlot calls, including shutdown cleanup. Public connectionCount() APIs return the atomic accepted-connection count.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• privkeyio/wisp#166: Shares connection slot reclamation and connectionCount() API changes.
• privkeyio/wisp#180: Shares handover, WebSocket cleanup, and connection lifecycle changes.
• privkeyio/wisp#181: Expands detached-connection release and handover list management.

Poem

A rabbit checks the handover line,
While locked lists settle into time.
WebSocket wrappers hop away,
And guarded slots close out the day.
Counts now tell the connections true.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: publishing handover connections only after releasing the connection mutex.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/swaplist-mutex-uaf

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}