Skip to content

Publish a handover connection only after releasing its mutex #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kwsantiago merged 2 commits into from
Aug 12, 2026
Merged

Publish a handover connection only after releasing its mutex #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da3f305
Publish a handover connection only after releasing its mutex
kwsantiago Aug 12, 2026
b789bfc
Wait for the lock holder before recycling an HTTPConn
kwsantiago Aug 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 122 additions & 17 deletions vendor/httpz.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,21 +60,68 @@ 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,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;
- 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.
.handover => self.handover_list.remove(io, 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.
+ //
+ // 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;
+ },
}

@@ -782,7 +799,15 @@
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),
- .handover => self.handover_list.insert(io, conn),
+ // Published by the caller once _mut is released; see swapList.
+ .handover => {},
}
}

@@ -782,7 +825,15 @@

while (c) |conn| {
c = conn.next;
Expand All @@ -91,7 +138,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 +844,7 @@
// can deliver a .recv for the freed Conn.
loop.remove(conn);
conn.close();
Expand All @@ -100,7 +147,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 +853,14 @@
// before we get back here.
// https://github.com/karlseguin/http.zig/issues/129#issuecomment-3031411404
closed_bool.* = true;
Expand All @@ -117,7 +164,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
continue;
}

@@ -817,18 +842,70 @@
@@ -817,18 +868,70 @@

const hc: *ws.HandlerConn(WSH) = @ptrCast(@alignCast(ptr));
conn.protocol = .{ .websocket = hc };
Expand Down Expand Up @@ -189,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 +962,10 @@
@@ -885,10 +988,10 @@
if (success == false) {
ws_conn.close(.{ .code = 4997, .reason = "wsz" }) catch {};
self.websocket.cleanupConn(hc);
Expand All @@ -202,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,10 +980,60 @@
@@ -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);
Expand Down Expand Up @@ -263,7 +310,17 @@ 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),
- .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. 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,
}
Expand All @@ -272,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 +1122,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);
91 changes: 78 additions & 13 deletions vendor/httpz/src/worker.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,21 +678,46 @@ 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.
.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.
//
// 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);
Expand All @@ -701,7 +726,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 => {},
}
}

Expand Down Expand Up @@ -1039,7 +1065,14 @@ 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),
.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. 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,
}
Expand Down Expand Up @@ -1123,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);
}
Expand Down Expand Up @@ -1636,14 +1670,45 @@ 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;
if (available == conns.len) {
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);
Expand Down