From da3f305fb403c516279e13ef4360fd0b7d27c183 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Wed, 12 Aug 2026 11:44:55 -0400 Subject: [PATCH 1/2] Publish a handover connection only after releasing its mutex --- vendor/httpz.patch | 78 +++++++++++++++++++++++++++++-------- vendor/httpz/src/worker.zig | 45 ++++++++++++++++----- 2 files changed, 97 insertions(+), 26 deletions(-) diff --git a/vendor/httpz.patch b/vendor/httpz.patch index 14a32fb..13f719c 100644 --- a/vendor/httpz.patch +++ b/vendor/httpz.patch @@ -60,21 +60,58 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig .keepalive_list = .{}, .buffer_pool = buffer_pool, .conn_mem_pool = conn_mem_pool, -@@ -675,6 +685,13 @@ +@@ -668,13 +678,37 @@ + fn swapList(self: *Self, conn: *Conn(WSH), new_state: HTTPConn.State) void { + const io = self.io; + const http_conn = conn.protocol.http; +- http_conn._mut.lockUncancelable(io); +- defer http_conn._mut.unlock(io); +- ++ { ++ http_conn._mut.lockUncancelable(io); ++ defer http_conn._mut.unlock(io); ++ self.swapListLocked(conn, http_conn, new_state); ++ } ++ ++ // Publishing to handover_list hands this conn to the event-loop ++ // thread, which may release it (releaseHandover) before this call ++ // even returns. So it MUST be the last thing that touches conn or ++ // http_conn: doing it inside the critical section above meant the ++ // deferred _mut.unlock() then wrote into an HTTPConn already ++ // returned to the pool or destroyed, which panics in Mutex.unlock ++ // with "switch on corrupt value" (reproduced) and is a silent ++ // write to recycled memory in ReleaseFast. The caller's ++ // loop.signal() afterwards deliberately touches neither. ++ if (new_state == .handover) self.handover_list.insert(io, conn); ++ } ++ ++ // Everything that needs http_conn._mut held. Split out so the handover ++ // publish can happen strictly after the mutex is released. ++ fn swapListLocked(self: *Self, conn: *Conn(WSH), http_conn: *HTTPConn, new_state: HTTPConn.State) void { ++ const io = self.io; + switch (http_conn._state) { .active => self.active_list.remove(io, conn), .keepalive => self.keepalive_list.remove(io, conn), .request => self.request_list.remove(conn), -+ // Unreachable in practice: both callers (run()'s parse-error -+ // path and accept()'s errdefer) only ever hold .request or -+ // .keepalive conns, and .handover conns are released through -+ // releaseHandover instead. Left as a plain remove rather than -+ // `unreachable` so that a wrong analysis degrades to a stale -+ // list entry instead of aborting a live relay. Do NOT route a -+ // handover-snapshot node here; see releaseHandover for why. ++ // Unreachable in practice: run()'s .recv handler skips a conn ++ // already in .handover, and processHTTPData only ever arrives ++ // here from .active. Left as a plain remove rather than ++ // `unreachable` so a wrong analysis degrades to a stale list ++ // entry instead of aborting a live relay. .handover => self.handover_list.remove(io, conn), } -@@ -782,7 +799,15 @@ +@@ -684,7 +718,8 @@ + .active => self.active_list.insert(io, conn), + .keepalive => self.keepalive_list.insert(io, conn), + .request => self.request_list.insert(conn), +- .handover => self.handover_list.insert(io, conn), ++ // Published by the caller once _mut is released; see swapList. ++ .handover => {}, + } + } + +@@ -782,7 +817,15 @@ while (c) |conn| { c = conn.next; @@ -91,7 +128,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig switch (http_conn.handover) { .close, .unknown => { closed_bool.* = true; -@@ -793,7 +818,7 @@ +@@ -793,7 +836,7 @@ // can deliver a .recv for the freed Conn. loop.remove(conn); conn.close(); @@ -100,7 +137,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig }, .disown => { // When res.disown() was called, we immediately removed -@@ -802,14 +827,14 @@ +@@ -802,14 +845,14 @@ // before we get back here. // https://github.com/karlseguin/http.zig/issues/129#issuecomment-3031411404 closed_bool.* = true; @@ -117,7 +154,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig continue; } -@@ -817,18 +842,70 @@ +@@ -817,18 +860,70 @@ const hc: *ws.HandlerConn(WSH) = @ptrCast(@alignCast(ptr)); conn.protocol = .{ .websocket = hc }; @@ -189,7 +226,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig } // Entry-point of our thread pool. `thread_buf` is a thread-specific buffer -@@ -885,10 +962,10 @@ +@@ -885,10 +980,10 @@ if (success == false) { ws_conn.close(.{ .code = 4997, .reason = "wsz" }) catch {}; self.websocket.cleanupConn(hc); @@ -202,7 +239,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig } else { // Release `processing` before re-arming. With EPOLLONESHOT, re-arming // while still holding `processing` loses a read that arrives in the -@@ -903,10 +980,60 @@ +@@ -903,20 +998,77 @@ log.debug("({f}) failed to add read event monitor: {}", .{ ws_conn.address, err }); ws_conn.close(.{ .code = 4998, .reason = "wsz" }) catch {}; self.websocket.cleanupConn(hc); @@ -263,7 +300,16 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig fn disown(self: *Self, conn: *Conn(WSH)) void { const io = self.io; const http_conn = conn.protocol.http; -@@ -916,7 +1043,7 @@ + switch (http_conn._state) { + .request => self.request_list.remove(conn), ++ // Unreachable in practice: both callers (run()'s parse-error ++ // path and accept()'s errdefer) only ever hold .request or ++ // .keepalive conns, and .handover conns are released through ++ // releaseHandover instead. Left as a plain remove rather than ++ // `unreachable` so a wrong analysis degrades to a stale list ++ // entry instead of aborting a live relay. Do NOT route a ++ // handover-snapshot node here; see releaseHandover for why. + .handover => self.handover_list.remove(io, conn), .keepalive => self.keepalive_list.remove(io, conn), .active => unreachable, } @@ -272,7 +318,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig self.http_conn_pool.release(http_conn); self.conn_mem_pool.destroy(conn); } -@@ -995,7 +1122,7 @@ +@@ -995,7 +1147,7 @@ while (conn) |c| { conn = c.next; c.close(); diff --git a/vendor/httpz/src/worker.zig b/vendor/httpz/src/worker.zig index ba7633f..fe18e5b 100644 --- a/vendor/httpz/src/worker.zig +++ b/vendor/httpz/src/worker.zig @@ -678,20 +678,37 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type { fn swapList(self: *Self, conn: *Conn(WSH), new_state: HTTPConn.State) void { const io = self.io; const http_conn = conn.protocol.http; - http_conn._mut.lockUncancelable(io); - defer http_conn._mut.unlock(io); + { + http_conn._mut.lockUncancelable(io); + defer http_conn._mut.unlock(io); + self.swapListLocked(conn, http_conn, new_state); + } + // Publishing to handover_list hands this conn to the event-loop + // thread, which may release it (releaseHandover) before this call + // even returns. So it MUST be the last thing that touches conn or + // http_conn: doing it inside the critical section above meant the + // deferred _mut.unlock() then wrote into an HTTPConn already + // returned to the pool or destroyed, which panics in Mutex.unlock + // with "switch on corrupt value" (reproduced) and is a silent + // write to recycled memory in ReleaseFast. The caller's + // loop.signal() afterwards deliberately touches neither. + if (new_state == .handover) self.handover_list.insert(io, conn); + } + + // Everything that needs http_conn._mut held. Split out so the handover + // publish can happen strictly after the mutex is released. + fn swapListLocked(self: *Self, conn: *Conn(WSH), http_conn: *HTTPConn, new_state: HTTPConn.State) void { + const io = self.io; switch (http_conn._state) { .active => self.active_list.remove(io, conn), .keepalive => self.keepalive_list.remove(io, conn), .request => self.request_list.remove(conn), - // Unreachable in practice: both callers (run()'s parse-error - // path and accept()'s errdefer) only ever hold .request or - // .keepalive conns, and .handover conns are released through - // releaseHandover instead. Left as a plain remove rather than - // `unreachable` so that a wrong analysis degrades to a stale - // list entry instead of aborting a live relay. Do NOT route a - // handover-snapshot node here; see releaseHandover for why. + // Unreachable in practice: run()'s .recv handler skips a conn + // already in .handover, and processHTTPData only ever arrives + // here from .active. Left as a plain remove rather than + // `unreachable` so a wrong analysis degrades to a stale list + // entry instead of aborting a live relay. .handover => self.handover_list.remove(io, conn), } @@ -701,7 +718,8 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type { .active => self.active_list.insert(io, conn), .keepalive => self.keepalive_list.insert(io, conn), .request => self.request_list.insert(conn), - .handover => self.handover_list.insert(io, conn), + // Published by the caller once _mut is released; see swapList. + .handover => {}, } } @@ -1039,6 +1057,13 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type { const http_conn = conn.protocol.http; switch (http_conn._state) { .request => self.request_list.remove(conn), + // Unreachable in practice: both callers (run()'s parse-error + // path and accept()'s errdefer) only ever hold .request or + // .keepalive conns, and .handover conns are released through + // releaseHandover instead. Left as a plain remove rather than + // `unreachable` so a wrong analysis degrades to a stale list + // entry instead of aborting a live relay. Do NOT route a + // handover-snapshot node here; see releaseHandover for why. .handover => self.handover_list.remove(io, conn), .keepalive => self.keepalive_list.remove(io, conn), .active => unreachable, From b789bfc8f8bc2179abe8647a7ed84efe031109db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Wed, 12 Aug 2026 12:07:07 -0400 Subject: [PATCH 2/2] Wait for the lock holder before recycling an HTTPConn --- vendor/httpz.patch | 95 ++++++++++++++++++++++++++++++------- vendor/httpz/src/worker.zig | 60 +++++++++++++++++++---- 2 files changed, 127 insertions(+), 28 deletions(-) diff --git a/vendor/httpz.patch b/vendor/httpz.patch index 13f719c..06e16da 100644 --- a/vendor/httpz.patch +++ b/vendor/httpz.patch @@ -60,7 +60,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig .keepalive_list = .{}, .buffer_pool = buffer_pool, .conn_mem_pool = conn_mem_pool, -@@ -668,13 +678,37 @@ +@@ -668,14 +678,46 @@ fn swapList(self: *Self, conn: *Conn(WSH), new_state: HTTPConn.State) void { const io = self.io; const http_conn = conn.protocol.http; @@ -93,15 +93,25 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig .active => self.active_list.remove(io, conn), .keepalive => self.keepalive_list.remove(io, conn), .request => self.request_list.remove(conn), +- .handover => self.handover_list.remove(io, conn), + // Unreachable in practice: run()'s .recv handler skips a conn + // already in .handover, and processHTTPData only ever arrives -+ // here from .active. Left as a plain remove rather than -+ // `unreachable` so a wrong analysis degrades to a stale list -+ // entry instead of aborting a live relay. - .handover => self.handover_list.remove(io, conn), ++ // here from .active. ++ // ++ // Logged rather than removed, because remove() is the dangerous ++ // option: on a non-member it rewrites the live list's head/tail ++ // from stale prev/next, which is exactly the connection-loss and ++ // core-pinning bug releaseHandover documents. That is not a ++ // "stale entry" degradation, so doing nothing is strictly safer, ++ // and the log makes a wrong analysis observable. ++ .handover => { ++ log.err("swapList reached the unreachable .handover state; not touching any list", .{}); ++ return; ++ }, } -@@ -684,7 +718,8 @@ + http_conn.setState(new_state); +@@ -684,7 +726,8 @@ .active => self.active_list.insert(io, conn), .keepalive => self.keepalive_list.insert(io, conn), .request => self.request_list.insert(conn), @@ -111,7 +121,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig } } -@@ -782,7 +817,15 @@ +@@ -782,7 +825,15 @@ while (c) |conn| { c = conn.next; @@ -128,7 +138,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig switch (http_conn.handover) { .close, .unknown => { closed_bool.* = true; -@@ -793,7 +836,7 @@ +@@ -793,7 +844,7 @@ // can deliver a .recv for the freed Conn. loop.remove(conn); conn.close(); @@ -137,7 +147,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig }, .disown => { // When res.disown() was called, we immediately removed -@@ -802,14 +845,14 @@ +@@ -802,14 +853,14 @@ // before we get back here. // https://github.com/karlseguin/http.zig/issues/129#issuecomment-3031411404 closed_bool.* = true; @@ -154,7 +164,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig continue; } -@@ -817,18 +860,70 @@ +@@ -817,18 +868,70 @@ const hc: *ws.HandlerConn(WSH) = @ptrCast(@alignCast(ptr)); conn.protocol = .{ .websocket = hc }; @@ -226,7 +236,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig } // Entry-point of our thread pool. `thread_buf` is a thread-specific buffer -@@ -885,10 +980,10 @@ +@@ -885,10 +988,10 @@ if (success == false) { ws_conn.close(.{ .code = 4997, .reason = "wsz" }) catch {}; self.websocket.cleanupConn(hc); @@ -239,7 +249,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig } else { // Release `processing` before re-arming. With EPOLLONESHOT, re-arming // while still holding `processing` loses a read that arrives in the -@@ -903,20 +998,77 @@ +@@ -903,20 +1006,77 @@ log.debug("({f}) failed to add read event monitor: {}", .{ ws_conn.address, err }); ws_conn.close(.{ .code = 4998, .reason = "wsz" }) catch {}; self.websocket.cleanupConn(hc); @@ -302,14 +312,15 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig const http_conn = conn.protocol.http; switch (http_conn._state) { .request => self.request_list.remove(conn), +- .handover => self.handover_list.remove(io, conn), + // Unreachable in practice: both callers (run()'s parse-error + // path and accept()'s errdefer) only ever hold .request or + // .keepalive conns, and .handover conns are released through -+ // releaseHandover instead. Left as a plain remove rather than -+ // `unreachable` so a wrong analysis degrades to a stale list -+ // entry instead of aborting a live relay. Do NOT route a -+ // handover-snapshot node here; see releaseHandover for why. - .handover => self.handover_list.remove(io, conn), ++ // releaseHandover instead. Do NOT route a handover-snapshot node ++ // here: remove() on a non-member rewrites the live list's ++ // head/tail from stale prev/next, the connection-loss bug ++ // releaseHandover documents, so this arm removes nothing. ++ .handover => log.err("disown reached the unreachable .handover state; not touching any list", .{}), .keepalive => self.keepalive_list.remove(io, conn), .active => unreachable, } @@ -318,12 +329,60 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig self.http_conn_pool.release(http_conn); self.conn_mem_pool.destroy(conn); } -@@ -995,7 +1147,7 @@ +@@ -995,7 +1155,8 @@ while (conn) |c| { conn = c.next; c.close(); - self.len -= 1; + self.releaseSlot(); ++ self.http_conn_pool.release(c.protocol.http); self.conn_mem_pool.destroy(c); } +@@ -1509,6 +1670,38 @@ + } + + fn release(self: *HTTPConnPool, conn: *HTTPConn) void { ++ const io = self.io; ++ ++ // Recycling an HTTPConn while a thread still holds its lock is a ++ // use-after-free: that thread's later _mut.unlock writes into an object ++ // returned to this pool or destroyed. Reproduced as a panic in ++ // Mutex.unlock ("switch on corrupt value"); in ReleaseFast there is no ++ // check, and since accept() does not re-init _mut, a fresh connection ++ // can inherit a stolen lock and end up with two threads inside ++ // swapListLocked for one conn. ++ // ++ // swapList holds _mut across its keepalive and request inserts, both of ++ // which publish the conn to the event-loop thread, so that thread can ++ // reach a free before the worker's deferred unlock lands. It gets there ++ // by three routes (disown, closeList, releaseHandover) and fencing them ++ // individually already missed one, so the wait lives here instead: this ++ // is the single point every free funnels through. ++ // ++ // Waiting for the lock is what makes it safe; the log is what makes the ++ // ordering violation attributable, since this race is not reachable from ++ // a unit test. Deadlock-free because every caller reaches this holding ++ // no list mutex, while the worker's order is _mut then list.mut, and ++ // because this runs before self.lock() below. ++ // Debug, not error: the wait below makes this safe, so it is an expected ++ // and handled interleaving rather than a fault. Under a deliberately ++ // widened window it fired 403 times across 1800 connections, so at error ++ // level it would be pure noise in production. ++ if (conn._mut.state.load(.monotonic) != .unlocked) { ++ log.debug("HTTPConn released while its lock was still held; waiting for the holder", .{}); ++ } ++ conn._mut.lockUncancelable(io); ++ conn._mut.unlock(io); ++ + const conns = self.conns; + self.lock(); + const available = self.available; +@@ -1516,7 +1709,6 @@ + self.unlock(); + conn.deinit(self.allocator); + +- const io = self.io; + self.http_mem_pool_mut.lockUncancelable(io); + self.http_mem_pool.destroy(conn); + self.http_mem_pool_mut.unlock(io); diff --git a/vendor/httpz/src/worker.zig b/vendor/httpz/src/worker.zig index fe18e5b..402094a 100644 --- a/vendor/httpz/src/worker.zig +++ b/vendor/httpz/src/worker.zig @@ -706,10 +706,18 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type { .request => self.request_list.remove(conn), // Unreachable in practice: run()'s .recv handler skips a conn // already in .handover, and processHTTPData only ever arrives - // here from .active. Left as a plain remove rather than - // `unreachable` so a wrong analysis degrades to a stale list - // entry instead of aborting a live relay. - .handover => self.handover_list.remove(io, conn), + // here from .active. + // + // Logged rather than removed, because remove() is the dangerous + // option: on a non-member it rewrites the live list's head/tail + // from stale prev/next, which is exactly the connection-loss and + // core-pinning bug releaseHandover documents. That is not a + // "stale entry" degradation, so doing nothing is strictly safer, + // and the log makes a wrong analysis observable. + .handover => { + log.err("swapList reached the unreachable .handover state; not touching any list", .{}); + return; + }, } http_conn.setState(new_state); @@ -1060,11 +1068,11 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type { // Unreachable in practice: both callers (run()'s parse-error // path and accept()'s errdefer) only ever hold .request or // .keepalive conns, and .handover conns are released through - // releaseHandover instead. Left as a plain remove rather than - // `unreachable` so a wrong analysis degrades to a stale list - // entry instead of aborting a live relay. Do NOT route a - // handover-snapshot node here; see releaseHandover for why. - .handover => self.handover_list.remove(io, conn), + // releaseHandover instead. Do NOT route a handover-snapshot node + // here: remove() on a non-member rewrites the live list's + // head/tail from stale prev/next, the connection-loss bug + // releaseHandover documents, so this arm removes nothing. + .handover => log.err("disown reached the unreachable .handover state; not touching any list", .{}), .keepalive => self.keepalive_list.remove(io, conn), .active => unreachable, } @@ -1148,6 +1156,7 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type { conn = c.next; c.close(); self.releaseSlot(); + self.http_conn_pool.release(c.protocol.http); self.conn_mem_pool.destroy(c); } @@ -1661,6 +1670,38 @@ const HTTPConnPool = struct { } fn release(self: *HTTPConnPool, conn: *HTTPConn) void { + const io = self.io; + + // Recycling an HTTPConn while a thread still holds its lock is a + // use-after-free: that thread's later _mut.unlock writes into an object + // returned to this pool or destroyed. Reproduced as a panic in + // Mutex.unlock ("switch on corrupt value"); in ReleaseFast there is no + // check, and since accept() does not re-init _mut, a fresh connection + // can inherit a stolen lock and end up with two threads inside + // swapListLocked for one conn. + // + // swapList holds _mut across its keepalive and request inserts, both of + // which publish the conn to the event-loop thread, so that thread can + // reach a free before the worker's deferred unlock lands. It gets there + // by three routes (disown, closeList, releaseHandover) and fencing them + // individually already missed one, so the wait lives here instead: this + // is the single point every free funnels through. + // + // Waiting for the lock is what makes it safe; the log is what makes the + // ordering violation attributable, since this race is not reachable from + // a unit test. Deadlock-free because every caller reaches this holding + // no list mutex, while the worker's order is _mut then list.mut, and + // because this runs before self.lock() below. + // Debug, not error: the wait below makes this safe, so it is an expected + // and handled interleaving rather than a fault. Under a deliberately + // widened window it fired 403 times across 1800 connections, so at error + // level it would be pure noise in production. + if (conn._mut.state.load(.monotonic) != .unlocked) { + log.debug("HTTPConn released while its lock was still held; waiting for the holder", .{}); + } + conn._mut.lockUncancelable(io); + conn._mut.unlock(io); + const conns = self.conns; self.lock(); const available = self.available; @@ -1668,7 +1709,6 @@ const HTTPConnPool = struct { self.unlock(); conn.deinit(self.allocator); - const io = self.io; self.http_mem_pool_mut.lockUncancelable(io); self.http_mem_pool.destroy(conn); self.http_mem_pool_mut.unlock(io);